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

From: Ira Weiny <ira.weiny@intel.com>
To: Michal Hocko <mhocko@kernel.org>
Cc: "Andrew Morton" <akpm@linux-foundation.org>,
	linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	"John Hubbard" <jhubbard@nvidia.com>,
	"Jérôme Glisse" <jglisse@redhat.com>,
	"Dan Williams" <dan.j.williams@intel.com>
Subject: Re: [PATCH v2] mm/swap: Fix release_pages() when releasing devmap pages
Date: Tue, 28 May 2019 20:56:19 -0700	[thread overview]
Message-ID: <20190529035618.GA21745@iweiny-DESK2.sc.intel.com> (raw)
In-Reply-To: <20190527150107.GG1658@dhcp22.suse.cz>

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