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 <jhubb...@nvidia.com> wrote: > >> > >> On 6/4/19 9:48 AM, ira.we...@intel.com wrote: > >>> From: Ira Weiny <ira.we...@intel.com> > >>> > ... > >>> 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 >