Re: [PATCH 2/2] mm/userfaultfd: don't lock anon_vma when performing UFFDIO_MOVE

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Sep 18, 2025 at 11:30:48PM -0700, Lokesh Gidra wrote:
> On Thu, Sep 18, 2025 at 5:38 AM Lorenzo Stoakes
> <lorenzo.stoakes@oracle.com> wrote:
> >
> > On Wed, Sep 17, 2025 at 10:51:35PM -0700, Lokesh Gidra wrote:
> > > Now that rmap_walk() is guaranteed to be called with the folio lock
> > > held, we can stop serializing on the src VMA anon_vma lock when moving
> > > an exclusive folio from a src VMA to a dst VMA in UFFDIO_MOVE ioctl.
> > >
> > > When moving a folio, we modify folio->mapping through
> > > folio_move_anon_rmap() and adjust folio->index accordingly. Doing that
> > > while we could have concurrent RMAP walks would be dangerous. Therefore,
> > > to avoid that, we had to acquire anon_vma of src VMA in write-mode. That
> > > meant that when multiple threads called UFFDIO_MOVE concurrently on
> > > distinct pages of the same src VMA, they would serialize on it, hurting
> > > scalability.
> > >
> > > In addition to avoiding the scalability bottleneck, this patch also
> > > simplifies the complicated lock dance that UFFDIO_MOVE has to go through
> > > between RCU, folio-lock, ptl, and anon_vma.
> > >
> > > folio_move_anon_rmap() already enforces that the folio is locked. So
> > > when we have the folio locked we can no longer race with concurrent
> > > rmap_walk() as used by folio_referenced() and hence the anon_vma lock
> >
> > And other rmap callers right?
> Right. Will fix it in the next version.

Thanks!

> >
> > > is no longer required.
> > >
> > > Note that this handling is now the same as for other
> > > folio_move_anon_rmap() users that also do not hold the anon_vma lock --
> > > namely COW reuse handling. These users never required the anon_vma lock
> > > as they are only moving the anon VMA closer to the anon_vma leaf of the
> > > VMA, for example, from an anon_vma root to a leaf of that root. rmap
> > > walks were always able to tolerate that scenario.
> >
> > Which users?
>
> The COW reusers, namely:
> do_wp_page()->wp_can_reuse_anon_folio()
> do_huge_pmd_wp_page()
> hugetlb_wp()

Right let's put this in the commit message is what I mean :)

>
> >
> > >
> > > CC: David Hildenbrand <david@redhat.com>
> > > CC: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > > CC: Peter Xu <peterx@redhat.com>
> > > CC: Suren Baghdasaryan <surenb@google.com>
> > > CC: Barry Song <baohua@kernel.org>
> > > Signed-off-by: Lokesh Gidra <lokeshgidra@google.com>
> > > ---
> > >  mm/huge_memory.c | 22 +----------------
> > >  mm/userfaultfd.c | 62 +++++++++---------------------------------------
> > >  2 files changed, 12 insertions(+), 72 deletions(-)
> > >
> > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > > index 5acca24bbabb..f444c142a8be 100644
> > > --- a/mm/huge_memory.c
> > > +++ b/mm/huge_memory.c
> > > @@ -2533,7 +2533,6 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
> > >       pmd_t _dst_pmd, src_pmdval;
> > >       struct page *src_page;
> > >       struct folio *src_folio;
> > > -     struct anon_vma *src_anon_vma;
> > >       spinlock_t *src_ptl, *dst_ptl;
> > >       pgtable_t src_pgtable;
> > >       struct mmu_notifier_range range;
> > > @@ -2582,23 +2581,9 @@ int move_pages_huge_pmd(struct mm_struct *mm, pmd_t *dst_pmd, pmd_t *src_pmd, pm
> > >                               src_addr + HPAGE_PMD_SIZE);
> > >       mmu_notifier_invalidate_range_start(&range);
> > >
> > > -     if (src_folio) {
> > > +     if (src_folio)
> > >               folio_lock(src_folio);
> > >
> > > -             /*
> > > -              * split_huge_page walks the anon_vma chain without the page
> > > -              * lock. Serialize against it with the anon_vma lock, the page
> > > -              * lock is not enough.
> > > -              */
> > > -             src_anon_vma = folio_get_anon_vma(src_folio);
> > > -             if (!src_anon_vma) {
> > > -                     err = -EAGAIN;
> > > -                     goto unlock_folio;
> > > -             }
> > > -             anon_vma_lock_write(src_anon_vma);
> > > -     } else
> > > -             src_anon_vma = NULL;
> > > -
> >
> > Hmm this seems an odd thing to include in the uffd change. Why not just include
> > it in the last commit or as a separate commit?

You're changing move_pages_huge_pmd() here in a change that's about the uffd
change, seems unrelated no?

>
> I'm not sure I follow. What am I including here?
>
> BTW, IMHO, the comment is wrong here. folio split code already
> acquires folio lock. The anon_vma lock is required here for the same
> reason as non-large page case - to avoid concurrent rmap walks.

This is called via split_huge_page() used by KMS and memory failure, not the
usual folio split logic afaict.

But those callers all take the folio look afaict :)

So yeah the comment is wrong it seems!