| Message ID | 20190605214922.17684-1-ira.weiny@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] mm/swap: Fix release_pages() when releasing devmap pages | expand |
Sorry for a late reply. On Wed 05-06-19 14:49:22, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > 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] This is much more clear than the previous version I've looked at. Thanks a lot! > > [1] https://lore.kernel.org/lkml/20190523172852.GA27175@iweiny-DESK2.sc.intel.com/ > > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Michal Hocko <mhocko@suse.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> Acked-by: Michal Hocko <mhocko@suse.com> > > --- > Changes from V3: > Update comment to the one provided by John > > 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..607c48229a1d 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; > + /* > + * 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(). > + */ > + if (put_devmap_managed_page(page)) > + continue; > } > > page = compound_head(page); > -- > 2.20.1 >
On Wed, Jun 05, 2019 at 02:49:22PM -0700, ira.weiny@intel.com wrote: > From: Ira Weiny <ira.weiny@intel.com> > > 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@iweiny-DESK2.sc.intel.com/ > > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Michal Hocko <mhocko@suse.com> > Reviewed-by: Dan Williams <dan.j.williams@intel.com> > Reviewed-by: John Hubbard <jhubbard@nvidia.com> > Signed-off-by: Ira Weiny <ira.weiny@intel.com> > > --- > Changes from V3: > Update comment to the one provided by John > > 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(-) Andrew, As per the discussion on the hmm thread I took this patch into the hmm.git as the conflict that was created with CH's rework was tricky - the resolution is simple, but keeping Ira's hunk instead of the delete is, IMHO, subtle. Regards, Jason
diff --git a/mm/swap.c b/mm/swap.c index 7ede3eddc12a..607c48229a1d 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; + /* + * 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(). + */ + if (put_devmap_managed_page(page)) + continue; } page = compound_head(page);