Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 10 Jun 2021 17:15:51 -0700 (PDT)
From:   Hugh Dickins <hughd@google.com>
To:     Alistair Popple <apopple@nvidia.com>
cc:     Andrew Morton <akpm@linux-foundation.org>,
        Hugh Dickins <hughd@google.com>,
        "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>,
        Yang Shi <shy828301@gmail.com>,
        Wang Yugui <wangyugui@e16-tech.com>,
        Matthew Wilcox <willy@infradead.org>,
        Naoya Horiguchi <naoya.horiguchi@nec.com>,
        Ralph Campbell <rcampbell@nvidia.com>, Zi Yan <ziy@nvidia.com>,
        Miaohe Lin <linmiaohe@huawei.com>,
        Minchan Kim <minchan@kernel.org>, Jue Wang <juew@google.com>,
        Peter Xu <peterx@redhat.com>, Jan Kara <jack@suse.cz>,
        Shakeel Butt <shakeelb@google.com>,
        Oscar Salvador <osalvador@suse.de>, linux-mm@kvack.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 00/10] mm/thp: fix THP splitting unmap BUGs and
 related
In-Reply-To: <2014832.e7zRqyNrDn@nvdebian>
Message-ID: <b27e866-a06c-d32-20aa-3b16f58549@google.com>
References: <af88612-1473-2eaa-903-8d1a448b26@google.com> <20210610151505.d0124033e55bda07fa3d4408@linux-foundation.org> <2014832.e7zRqyNrDn@nvdebian>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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@google.com> 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@intel.com
> > 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