diff mbox series

[v2] mm/swap: Fix release_pages() when releasing devmap pages

Message ID 20190524173656.8339-1-ira.weiny@intel.com (mailing list archive)
State New, archived
Headers show
Series [v2] mm/swap: Fix release_pages() when releasing devmap pages | expand

Commit Message

Ira Weiny May 24, 2019, 5:36 p.m. UTC 
From: Ira Weiny <ira.weiny@intel.com>

Device pages can be more than type MEMORY_DEVICE_PUBLIC.

Handle all device pages within release_pages()

This was found via code inspection while determining if release_pages()
and the new put_user_pages() could be interchangeable.

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 V1:
	Add comment clarifying that put_devmap_managed_page() can still
	fail.
	Add Reviewed-by tags.

 mm/swap.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

Comments

Michal Hocko May 27, 2019, 3:01 p.m. UTC | #1 
On Fri 24-05-19 10:36:56, ira.weiny@intel.com wrote:
> From: Ira Weiny <ira.weiny@intel.com>
> 
> Device pages can be more than type MEMORY_DEVICE_PUBLIC.
> 
> Handle all device pages within release_pages()
> 
> This was found via code inspection while determining if release_pages()
> and the new put_user_pages() could be interchangeable.

Please expand more about who is such a user and why does it use
release_pages rather than put_*page API. The above changelog doesn't
really help understanding what is the actual problem. I also do not
understand the fix and a failure mode from release_pages is just scary.
It is basically impossible to handle the error case. So what is going on
here?

> 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 V1:
> 	Add comment clarifying that put_devmap_managed_page() can still
> 	fail.
> 	Add Reviewed-by tags.
> 
>  mm/swap.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/swap.c b/mm/swap.c
> index 9d0432baddb0..f03b7b4bfb4f 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -740,15 +740,18 @@ 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 can still fail here and will
> +			 * therefore need put_page_testzero()
> +			 */
> +			if (put_devmap_managed_page(page))
> +				continue;
>  		}
>  
>  		page = compound_head(page);
> -- 
> 2.20.1
>
Ira Weiny May 29, 2019, 3:56 a.m. UTC | #2 
On Mon, May 27, 2019 at 05:01:07PM +0200, Michal Hocko wrote:
> On Fri 24-05-19 10:36:56, ira.weiny@intel.com wrote:
> > From: Ira Weiny <ira.weiny@intel.com>
> > 
> > Device pages can be more than type MEMORY_DEVICE_PUBLIC.
> > 
> > Handle all device pages within release_pages()
> > 
> > This was found via code inspection while determining if release_pages()
> > and the new put_user_pages() could be interchangeable.
> 
> Please expand more about who is such a user and why does it use
> release_pages rather than put_*page API.

Sorry for not being more clear.   The error was discovered while discussing a
proposal to change a use of release_pages() to put_user_pages()[1]

[1] https://lore.kernel.org/lkml/20190523172852.GA27175@iweiny-DESK2.sc.intel.com/

In that thread John was saying that release_pages() was functionally equivalent
to a loop around put_page().  He also suggested implementing put_user_pages()
by using release_pages().  On the surface they did not seem the same to me so I
did a deep dive to make sure they were and found this error.

>
> The above changelog doesn't
> really help understanding what is the actual problem. I also do not
> understand the fix and a failure mode from release_pages is just scary.

This is not failing release_pages().  The fix is that not all devmap pages are
"public" type.  So previous to this change devmap pages of other types would
not correctly be accounted for.

The discussion about put_devmap_managed_page() "failing" is not about it
failing directly but rather in how these pages should be accounted for.  Only
devmap pages which require pagemap ops (specifically page_free()) require
put_devmap_managed_page() processing.   Because of the optimized locking in
release_pages() the zone device check is required to release the lock even if
put_devmap_managed_page() does not handle the put.

> It is basically impossible to handle the error case. So what is going on
> here?

I think what has happened is the code in release_pages() and put_page()
diverged at some point.  I think it is worth a clean up in this area but I
don't see way to do it at the moment which would be any cleaner than what is
there.  So I've refrained from doing so.

Does this help?  Would you like to roll a V3 with some of this in the commit
message?

Ira

>
>
>
> 
> > 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 V1:
> > 	Add comment clarifying that put_devmap_managed_page() can still
> > 	fail.
> > 	Add Reviewed-by tags.
> > 
> >  mm/swap.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> > diff --git a/mm/swap.c b/mm/swap.c
> > index 9d0432baddb0..f03b7b4bfb4f 100644
> > --- a/mm/swap.c
> > +++ b/mm/swap.c
> > @@ -740,15 +740,18 @@ 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 can still fail here and will
> > +			 * therefore need put_page_testzero()
> > +			 */
> > +			if (put_devmap_managed_page(page))
> > +				continue;
> >  		}
> >  
> >  		page = compound_head(page);
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs
Michal Hocko June 4, 2019, 12:35 p.m. UTC | #3 
On Tue 28-05-19 20:56:19, Ira Weiny wrote:
> Would you like to roll a V3 with some of this in the commit
> message?

Yes please. I will re-read the whole changelog and let's see whether I
can make more sense of it.

Thanks!
diff mbox series

Patch

diff --git a/mm/swap.c b/mm/swap.c
index 9d0432baddb0..f03b7b4bfb4f 100644
--- a/mm/swap.c
+++ b/mm/swap.c
@@ -740,15 +740,18 @@  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 can still fail here and will
+			 * therefore need put_page_testzero()
+			 */
+			if (put_devmap_managed_page(page))
+				continue;
 		}
 
 		page = compound_head(page);