On Tue, Jun 04, 2019 at 01:17:42PM -0700, John Hubbard wrote:
> On 6/4/19 1:11 PM, Dan Williams wrote:
> > On Tue, Jun 4, 2019 at 12:48 PM John Hubbard <jhubbard@xxxxxxxxxx> wrote:
> >>
> >> On 6/4/19 9:48 AM, ira.weiny@xxxxxxxxx wrote:
> >>> From: Ira Weiny <ira.weiny@xxxxxxxxx>
> >>>
> ...
> >>> diff --git a/mm/swap.c b/mm/swap.c
> >>> index 7ede3eddc12a..6d153ce4cb8c 100644
> >>> --- a/mm/swap.c
> >>> +++ b/mm/swap.c
> >>> @@ -740,15 +740,20 @@ void release_pages(struct page **pages, int nr)
> >>> if (is_huge_zero_page(page))
> >>> continue;
> >>>
> >>> - /* Device public page can not be huge page */
> >>> - if (is_device_public_page(page)) {
> >>> + if (is_zone_device_page(page)) {
> >>> if (locked_pgdat) {
> >>> spin_unlock_irqrestore(&locked_pgdat->lru_lock,
> >>> flags);
> >>> locked_pgdat = NULL;
> >>> }
> >>> - put_devmap_managed_page(page);
> >>> - continue;
> >>> + /*
> >>> + * Not all zone-device-pages require special
> >>> + * processing. Those pages return 'false' from
> >>> + * put_devmap_managed_page() expecting a call to
> >>> + * put_page_testzero()
> >>> + */
> >>
> >> Just a documentation tweak: how about:
> >>
> >> /*
> >> * ZONE_DEVICE pages that return 'false' from
> >> * put_devmap_managed_page() do not require special
> >> * processing, and instead, expect a call to
> >> * put_page_testzero().
> >> */
> >
> > Looks better to me, but maybe just go ahead and list those
> > expectations explicitly. Something like:
> >
> > /*
> > * put_devmap_managed_page() only handles
> > * ZONE_DEVICE (struct dev_pagemap managed)
> > * pages when the hosting dev_pagemap has the
> > * ->free() or ->fault() callback handlers
> > * implemented as indicated by
> > * dev_pagemap.type. Otherwise the expectation
> > * is to fall back to a plain decrement /
> > * put_page_testzero().
> > */
>
> I like it--but not here, because it's too much internal detail in a
> call site that doesn't use that level of detail. The call site looks
> at the return value, only.
>
> Let's instead put that blurb above (or in) the put_devmap_managed_page()
> routine itself. And leave the blurb that I wrote where it is. And then I
> think everything will have an appropriate level of detail in the right places.
I agree. This leaves it open that this handles any special processing which is
required.
FWIW the same call is made in put_page() and has no comment so perhaps we are
getting wrapped around the axle for no reason?
Frankly I questioned myself when I mentioned put_page_testzero() as well. But
I'm ok with Johns suggestion. My wording was a bit "rushed". Sorry about
that. I wanted to remove the word 'fail' from the comment because I think it
is what caught Michal's eye.
Ira
>
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
