LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] mm/swap: Fix release_pages() when releasing devmap pages
@ 2019-05-24 17:36 ira.weiny
  2019-05-27 15:01 ` Michal Hocko
  0 siblings, 1 reply; 4+ messages in thread
From: ira.weiny @ 2019-05-24 17:36 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Michal Hocko, linux-mm, linux-kernel, John Hubbard, Ira Weiny,
	Jérôme Glisse, Dan Williams

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(-)

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] mm/swap: Fix release_pages() when releasing devmap pages
  2019-05-24 17:36 [PATCH v2] mm/swap: Fix release_pages() when releasing devmap pages ira.weiny
@ 2019-05-27 15:01 ` Michal Hocko
  2019-05-29  3:56   ` Ira Weiny
  0 siblings, 1 reply; 4+ messages in thread
From: Michal Hocko @ 2019-05-27 15:01 UTC (permalink / raw)
  To: ira.weiny
  Cc: Andrew Morton, linux-mm, linux-kernel, John Hubbard,
	Jérôme Glisse, Dan Williams

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
> 

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] mm/swap: Fix release_pages() when releasing devmap pages
  2019-05-27 15:01 ` Michal Hocko
@ 2019-05-29  3:56   ` Ira Weiny
  2019-06-04 12:35     ` Michal Hocko
  0 siblings, 1 reply; 4+ messages in thread
From: Ira Weiny @ 2019-05-29  3:56 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, linux-mm, linux-kernel, John Hubbard,
	Jérôme Glisse, Dan Williams

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH v2] mm/swap: Fix release_pages() when releasing devmap pages
  2019-05-29  3:56   ` Ira Weiny
@ 2019-06-04 12:35     ` Michal Hocko
  0 siblings, 0 replies; 4+ messages in thread
From: Michal Hocko @ 2019-06-04 12:35 UTC (permalink / raw)
  To: Ira Weiny
  Cc: Andrew Morton, linux-mm, linux-kernel, John Hubbard,
	Jérôme Glisse, Dan Williams

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!
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2019-06-04 12:35 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-24 17:36 [PATCH v2] mm/swap: Fix release_pages() when releasing devmap pages ira.weiny
2019-05-27 15:01 ` Michal Hocko
2019-05-29  3:56   ` Ira Weiny
2019-06-04 12:35     ` Michal Hocko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).