On 6/4/19 9:48 AM, ira.weiny@xxxxxxxxx wrote: > From: Ira Weiny <ira.weiny@xxxxxxxxx> > > release_pages() is an optimized version of a loop around put_page(). > Unfortunately for devmap pages the logic is not entirely correct in > release_pages(). This is because device pages can be more than type > MEMORY_DEVICE_PUBLIC. There are in fact 4 types, private, public, FS > DAX, and PCI P2PDMA. Some of these have specific needs to "put" the > page while others do not. > > This logic to handle any special needs is contained in > put_devmap_managed_page(). Therefore all devmap pages should be > processed by this function where we can contain the correct logic for a > page put. > > Handle all device type pages within release_pages() by calling > put_devmap_managed_page() on all devmap pages. If > put_devmap_managed_page() returns true the page has been put and we > continue with the next page. A false return of > put_devmap_managed_page() means the page did not require special > processing and should fall to "normal" processing. > > This was found via code inspection while determining if release_pages() > and the new put_user_pages() could be interchangeable.[1] > > [1] https://lore.kernel.org/lkml/20190523172852.GA27175@xxxxxxxxxxxxxxxxxxxxxxxxx/ > > Cc: Jérôme Glisse <jglisse@xxxxxxxxxx> > Cc: Michal Hocko <mhocko@xxxxxxxx> > Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx> > Reviewed-by: John Hubbard <jhubbard@xxxxxxxxxx> > Signed-off-by: Ira Weiny <ira.weiny@xxxxxxxxx> > > --- > Changes from V2: > Update changelog for more clarity as requested by Michal > Update comment WRT "failing" of put_devmap_managed_page() > > Changes from V1: > Add comment clarifying that put_devmap_managed_page() can still > fail. > Add Reviewed-by tags. > > mm/swap.c | 13 +++++++++---- > 1 file changed, 9 insertions(+), 4 deletions(-) > > 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(). */ thanks, -- John Hubbard NVIDIA > + if (put_devmap_managed_page(page)) > + continue; > } > > page = compound_head(page); >
