Re: [PATCH v2 00/10] mm/thp: fix THP splitting unmap BUGs and related

[Hugh Dickins]

On Fri, 11 Jun 2021, Alistair Popple wrote:
> On Friday, 11 June 2021 8:15:05 AM AEST Andrew Morton wrote:
> > On Tue, 8 Jun 2021 20:57:34 -0700 (PDT) Hugh Dickins <hughd@xxxxxxxxxx> wrote:
> > 
> > > These are against 5.13-rc5: expect mmotm conflicts with a couple of
> > > Alistair Popple's "Add support for SVM atomics in Nouveau" series:
> > > mm-remove-special-swap-entry-functions.patch
> > > mm-rmap-split-try_to_munlock-from-try_to_unmap.patch
> > 
> > I came unstuck at "mm/rmap: split migration into its own function".

Sorry about that, I hadn't yet gotten to trying my latest with mmotm.
And I think my previous mmotm-adjust.tar must have been incomplete;
and even if it were complete, would no longer apply properly anyway.

> > 
> > --- mm/huge_memory.c~mm-rmap-split-migration-into-its-own-function
> > +++ mm/huge_memory.c
> > @@ -2345,16 +2345,21 @@ void vma_adjust_trans_huge(struct vm_are
> > 
> >  static void unmap_page(struct page *page)
> >  {
> > -       enum ttu_flags ttu_flags = TTU_IGNORE_MLOCK |
> > -               TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD;
> > +       enum ttu_flags ttu_flags = TTU_RMAP_LOCKED | TTU_SPLIT_HUGE_PMD;
> >         bool unmap_success;
> > 
> >         VM_BUG_ON_PAGE(!PageHead(page), page);
> > 
> >         if (PageAnon(page))
> > -               ttu_flags |= TTU_SPLIT_FREEZE;
> > -
> > -       unmap_success = try_to_unmap(page, ttu_flags);
> > +               unmap_success = try_to_migrate(page, ttu_flags);
> > +       else
> > +               /*
> > +                * Don't install migration entries for file backed pages. This
> > +                * helps handle cases when i_size is in the middle of the page
> > +                * as there is no need to unmap pages beyond i_size manually.
> > +                */
> > +               unmap_success = try_to_unmap(page, ttu_flags |
> > +                                               TTU_IGNORE_MLOCK);
> >         VM_BUG_ON_PAGE(!unmap_success, page);
> >  }
> > 
> > 
> > Sigh.  I have a few todo's against Alastair's "Add support for SVM
> > atomics in Nouveau v9".  Including

Sigh shared!

> > 
> > https://lkml.kernel.org/r/20210525183710.fa2m2sbfixnhz7g5@revolver
> > https://lkml.kernel.org/r/20210604204934.sbspsmwdqdtmz73d@revolver
> > https://lkml.kernel.org/r/YK6mbf967dV0ljHn@t490s
> > https://lkml.kernel.org/r/2005328.bFqPmhE5MS@nvdebian
> > https://lkml.kernel.org/r/202105262107.LkxpsZsV-lkp@xxxxxxxxx
> > https://lkml.kernel.org/r/YK6hYGEx+XzeZELV@t490s
> > 
> > So I think I'll drop that series and shall ask for it to be redone
> > against this lot, please.

Thank you, Andrew: that's certainly easiest for you and for me:
and I think the right thing to do for now.

> > 
> 
> I believe v10 of the series posted earlier this week should address those
> todo's. I will double check though and resend based on top of mmotm. Thanks.

Sorry to give you the bother, Alistair: it's worked out as a bad moment
to rewrite swapops.h and rmap.c, I'm afraid.

And the only help I've had time to give you was pointing Peter at your
series - many thanks to Peter, and to Shakeel.

Several times I've been on the point of asking you to keep the familiar
migration_entry_to_page(), along with your new pfn_swap_entry_to_page();
but each time I've looked, seen that it's hard to retain it sensibly at
the same time as overdue cleanup of the device_private_entry_to_page()s.

So I guess I'm resigned to losing it; but there are at least three
bugs currently under discussion or fixes in flight, which border on
migration_entry_to_page() - Jann Horn's smaps syzbot bug, Xu Yu's
__migration_entry_wait() fix, my __split_huge_pmd_locked() fix
(and page_vma_mapped_walk() cleanup).

And regarding huge_memory.c's unmap_page(): I did not recognize the
"helps handle cases when i_size" comment you added there.  What I
ended up with (and thought was in mmotm-adjust.tar but seems not):

	/*
	 * Anon pages need migration entries to preserve them, but file
	 * pages can simply be left unmapped, then faulted back on demand.
	 * If that is ever changed (perhaps for mlock), update remap_page().
	 */
	if (PageAnon(page))
		try_to_migrate(page, ttu_flags);
	else
		try_to_unmap(page, ttu_flags | TTU_IGNORE_MLOCK);

with
	/* If try_to_migrate() is used on file, remove this check */
in remap_page() to replace the
	/* If TTU_SPLIT_FREEZE is ever extended to file, remove this check */
comment my series puts there (since you delete TTU_SPLIT_FREEZE altogether).

Hugh