[Intel-gfx] mm/huge_memory: do not clobber swp_entry_t during THP split

[Hugh Dickins]

On Tue, 25 Oct 2022, Mel Gorman wrote:

> Cc'ing Andrew for awareness. Andrew, this bug report is almost identical to
> the one Hugh already reported and fixed in "[PATCH] mm: prep_compound_tail()
> clear page->private". Nothing wrong with the patch AFAIK and only the last
> paragraph is relevant to you.
> 
> On Tue, Oct 25, 2022 at 09:50:14AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 24/10/2022 15:23, Mel Gorman wrote:
> > > On Mon, Oct 24, 2022 at 02:04:50PM +0100, Tvrtko Ursulin wrote:
> > > > 
> > > > Hi Mel, mm experts,
> > > > 
> > > > With 6.1-rc2 we started hitting the WARN_ON added in 71e2d666ef85 ("mm/huge_memory: do not clobber swp_entry_t during THP split") in i915 automated CI:
> > > > 
> > > 
> > > Thanks for the report. As shmem pages pages are allocated via vma_alloc_folio
> > > and are compound pages, can you try the following patch please?  If it
> > > still triggers, please post the new oops as it'll include the tail page
> > > information.
> > > 
> > > --8<--
> > > From: Hugh Dickins <hughd at google.com>
> > > Subject: [PATCH] mm: prep_compound_tail() clear page->private
> > > 
> > > Although page allocation always clears page->private in the first page
> > > or head page of an allocation, it has never made a point of clearing
> > > page->private in the tails (though 0 is often what is already there).
> > > 
> > > But now commit 71e2d666ef85 ("mm/huge_memory: do not clobber swp_entry_t
> > > during THP split") issues a warning when page_tail->private is found to
> > > be non-0 (unless it's swapcache).
> > > 
> > > Change that warning to dump page_tail (which also dumps head), instead
> > > of just the head: so far we have seen dead000000000122, dead000000000003,
> > > dead000000000001 or 0000000000000002 in the raw output for tail private.
> > > 
> > > We could just delete the warning, but today's consensus appears to want
> > > page->private to be 0, unless there's a good reason for it to be set:
> > > so now clear it in prep_compound_tail() (more general than just for THP;
> > > but not for high order allocation, which makes no pass down the tails).
> > > 
> > > Fixes: 71e2d666ef85 ("mm/huge_memory: do not clobber swp_entry_t during THP split")
> > > Signed-off-by: Hugh Dickins <hughd at google.com>
> > > Cc: Mel Gorman <mgorman at techsingularity.net>
> > > Cc: Matthew Wilcox (Oracle) <willy at infradead.org>
> > > Cc: <stable at vger.kernel.org>
> > > ---
> > >   mm/huge_memory.c | 2 +-
> > >   mm/page_alloc.c  | 1 +
> > >   2 files changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 03fc7e5edf07..561a42567477 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -2462,7 +2462,7 @@ static void __split_huge_page_tail(struct page *head, int tail,
> > >   	 * Fix up and warn once if private is unexpectedly set.
> > >   	 */
> > >   	if (!folio_test_swapcache(page_folio(head))) {
> > > -		VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, head);
> > > +		VM_WARN_ON_ONCE_PAGE(page_tail->private != 0, page_tail);
> > >   		page_tail->private = 0;
> > >   	}
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index b5a6c815ae28..218b28ee49ed 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -807,6 +807,7 @@ static void prep_compound_tail(struct page *head, int tail_idx)
> > >   	p->mapping = TAIL_MAPPING;
> > >   	set_compound_head(p, head);
> > > +	set_page_private(p, 0);
> > >   }
> > >   void prep_compound_page(struct page *page, unsigned int order)
> > 
> > The patch seems to fix our CI runs.
> 
> Thanks for letting me know.
> 
> > Is it considered final version?
> 
> AFAIK, yes.

AFAItooK, yes - modulo akpm's signoff and final SHA1.

> 
> > If so I
> > can temporarily put it in until it arrives via the next rc - assuming that
> > would be the flow from upstream pov?

The right thing for now is for GregKH to drop Mel's from 6.0.4:
I've just sent a mail asking for that (I would have asked yesterday,
but mistook that GregKH was not in Cc).

Of course Mel's fix is much more important than the harmless
(unless panic on warn) warning, but let's delay it a few more days,
it just flowed into stable too quickly.

Thanks Mel: I never knowingly hit the THP_SWAP issue which your patch
is fixing, but it now looks like it was also responsible for mysterious
occasional OOM kills that I had been chasing for weeks.

Hugh

> > 
> 
> I expect it to. It's currently in the akpm/mm.git branch
> mm/mm-hotfixes-unstable where I expect it to flow to mm/mm-hotfixes-stable
> in due course before sending to Linus. I can't make promises about the
> timing as that's determined by Andrew.
> 
> -- 
> Mel Gorman
> SUSE Labs