Re: [PATCH v4 12/17] mm/huge_memory: move anon_vma handling into the anon split helper

[Kairui Song]

On Tue, Sep 8, 2026 at 4:40 AM Zi Yan <ziy@xxxxxxxxxx> wrote:
>
> On Mon Sep 7, 2026 at 2:12 PM EDT, Kairui Song via B4 Relay wrote:
> > From: Kairui Song <kasong@xxxxxxxxxxx>
> >
> > Only anon split needs the anon_vma, and it only needs it to unmap and
> > remap.  Move the folio_get_anon_vma()/anon_vma_lock_write() pair out of
> > __folio_split() into the anon helper next to the folio_mapped()
> > check that already gates unmap_folio().
> >
> > This makes the anon_vma conditional on folio_mapped(), which is a
> > behaviour change but should be fine.  folio_get_anon_vma() returns
> > NULL whenever !folio_mapped(), so an anon folio with
> > folio_mapcount() == 0 used to get -EBUSY from split_huge_page() and is
> > now split instead.  The realistic case is a THP that has been fully
> > swapped out and is still in the swap cache: swap PTEs do not contribute
> > mapcount, so it is !folio_mapped() but still alive.
> >
> > That should be safe and right to have because:
> >
> > - folio_ref_freeze() below still rejects a folio that picked up any
> >   reference, a mapping or a GUP pin, in the meantime.
> >
> > - A parallel split is excluded by the folio lock.  The anon_vma
> >   write lock was added to serialize split in commit 062f1af2170a
> >   ("mm: thp: acquire the anon_vma rwsem for write during split"), when
> >   split_huge_page() did not hold the folio lock throughout. commit
> >   e9b61f19858a ("thp: reintroduce split_huge_page()") later made the
> >   folio lock a caller requirement and added the folio_ref_freeze()
> >   scheme, so that has been covered ever since.
> >
> > - Unmapped path is already exercised by folio_split_unmapped(),
> >   and the swap cache split already runs well for a partially
> >   swapped-out mapped THP.
> >
> > - folio_get_anon_vma() and folio_lock_anon_vma_read() both bail out
>
> You mean folio_lock_anon_vma_read() called by others? Since there is no
> folio_lock_anon_vma_read() in folio split functions.

Oh, the original comment mentioned it, so I also looked into that.

>
> >   on !folio_mapped() before taking the anon_vma lock, so there is
> >   nothing to lock against.
> >
> > For mapped folios the anon_vma write lock is now released before
> > __folio_split() unlocks the after-split sub-folios, where previously it
> > was held across that loop, that window is harmless as the sub-folios
> > stay folio-locked and ref pinned.
> >
> > Signed-off-by: Kairui Song <kasong@xxxxxxxxxxx>
> > ---
> >  mm/huge_memory.c | 51 ++++++++++++++++++++++++---------------------------
> >  1 file changed, 24 insertions(+), 27 deletions(-)
> >
> > diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> > index c4910c0b6018..53614b875794 100644
> > --- a/mm/huge_memory.c
> > +++ b/mm/huge_memory.c
> > @@ -4005,16 +4005,31 @@ static int __folio_freeze_split_anon(struct folio *folio,
> >       struct swap_cluster_info *ci = NULL;
> >       struct folio *new_folio, *next;
> >       int old_order = folio_order(folio);
> > +     struct anon_vma *anon_vma = NULL;
> >       enum ttu_flags ttu_flags = 0;
> >       struct lruvec *lruvec;
> > -     bool need_remap = false;
> >       int ret = 0;
> >
> > +     /*
> > +      * Unmap/remap needs the anon_vma. The caller does not necessarily
> > +      * hold an mmap_lock that would prevent the anon_vma from
> > +      * disappearing, so we first take a reference and lock it.
> > +      *
> > +      * An unmapped folio needs none of this: folio_get_anon_vma() and
>
> I guess you mean an unmapped folio does not need a ref or a lock on
> anon_vma. It is better to be explicit about them.
>
> > +      * folio_lock_anon_vma_read() both bail out on !folio_mapped()
>
> Mentioning folio_lock_anon_vma_read() is confusing since folio split
> does not call it.

Right, I actually just copied the original comment, and I'm a little
bit confused about it too, I can try to improve it while I'm at it.

> > +      * before taking the lock, and folio_ref_freeze() below still
> > +      * rejects a folio that picked up a reference meanwhile. Note
> > +      * a swapped-out THP counts as unmapped here as swap PTEs do
> > +      * not contribute mapcount, and they are splittable.
> > +      */

So how about rewrite this comment chunk as follows:

      /*
       * Unmap/remap needs the anon_vma. The caller does not necessarily
       * hold an mmap_lock that would prevent the anon_vma from
       * disappearing, so we first take a reference on it and then lock
       * it for write, letting unmap_folio() walk the rmap with
       * TTU_RMAP_LOCKED.
       *
       * An unmapped folio needs neither the reference nor the lock:
       * no unmap walk happens for a folio without mappings, so there
       * is no anon_vma to pin and nobody to lock against, and the
       * folio_ref_freeze() below still rejects any reference picked
       * up meanwhile. Note that a fully swapped-out THP still in
       * the swap cache counts as unmapped here, as swap PTEs do not
       * contribute to the mapcount, it is splittable just fine.
       */

> >       if (folio_mapped(folio)) {
> > -             need_remap = true;
> > +             anon_vma = folio_get_anon_vma(folio);
> > +             if (!anon_vma)
> > +                     return -EBUSY;
> > +             anon_vma_lock_write(anon_vma);
> >               ret = unmap_folio(folio);
> >               if (ret)
> > -                     return ret;
> > +                     goto out_unlock;
> >       }
> >
> >       local_irq_disable();
>
> Otherwise, LGTM. With the comit message and comments addressed, feel
> free to add
>
> Reviewed-by: Zi Yan <ziy@xxxxxxxxxx>

Thanks for the review!