Re: [PATCH 3/6] mm: Add RCU-based VMA lookup that waits for writers

[Suren Baghdasaryan]

On Fri, May 8, 2026 at 10:26 AM Lorenzo Stoakes <ljs@xxxxxxxxxx> wrote:
>
> On Wed, Apr 29, 2026 at 11:19:59AM -0700, Dave Hansen wrote:
> >
> > From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> >
> > == Background ==
> >
> > There are basically two parallel ways to look up a VMA: the
> > traditional way, which is protected by mmap_lock, and the RCU-based
> > per-VMA lock way which is based on RCU and refcounts.
> >
> > == Problems ==
> >
> > The mmap_lock one is more straightforward to use but it has a big
> > disadvantage in that it can not be mixed with page faults since those
> > can take mmap_lock for read.

Hmm. You can take mmap_read_lock and execute VMA lookup without
blocking page faults. It's the mmap_write_lock that would interfere
with page faults but you don't need a write lock for a lookup.
The disadvantage of using mmap_lock is that you will block (or wait
for) all other writers even if they want to modify VMAs unrelated to
your VMA.

Reading this problem statement I'm still unclear what the problem is
and the fact that the helper function is not yet used is not
helping... Hopefully in the later patches where it's used I can find
the clues.

> >
> > The RCU one can be mixed with faults, but it is not available in all
> > configs, so all RCU users need to be able to fall back to the
> > traditional way.
> >
> > == Solution ==
> >
> > Add a variant of the RCU-based lookup that waits for writers. This is
> > basically the same as the existing RCU-based lookup, but it also takes
> > mmap_lock for read and waits for writers to finish before returning
> > the VMA. This has two big advantages:
> >
> >  1. Callers do not need to have a fallback path for when they
> >     collide with writers.
> >  2. It can be used in contexts where page faults can happen because
> >     it can take the mmap_lock for read but never *holds* it.
> >
> > == Discussion ==
> >
> > I am not married to the naming here at all. Naming suggestions would
> > be much appreciated.
> >
> > This basically uses mmap_lock to wait for writers, nothing else. The
> > VMA is obviously stable under mmap_read_lock() and the code _can_
> > likely take advantage of that and possibly even remove the goto. For
> > instance, it could (probably) bump the VMA refcount and exclude future
> > writers. That would eliminate the goto.
> >
> > But the approach as-is is probably the smallest line count and
> > arguably the simplest approach. It is a good place to start a
> > conversation if nothing else.

This idea is totally sane and we already have something very similar
in the form of uffd_lock_vma(). I would suggest using it as an
example.

> >
> > Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>
> > Cc: Suren Baghdasaryan <surenb@xxxxxxxxxx>
> > Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> > Cc: "Liam R. Howlett" <Liam.Howlett@xxxxxxxxxx>
> > Cc: Lorenzo Stoakes <ljs@xxxxxxxxxx>
> > Cc: Vlastimil Babka <vbabka@xxxxxxxxxx>
> > Cc: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> > Cc: linux-mm@xxxxxxxxx
> > ---
> >
> >  b/include/linux/mmap_lock.h |    2 ++
> >  b/mm/mmap_lock.c            |   43 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 45 insertions(+)
> >
> > diff -puN include/linux/mmap_lock.h~lock-vma-under-rcu-wait include/linux/mmap_lock.h
> > --- a/include/linux/mmap_lock.h~lock-vma-under-rcu-wait       2026-04-29 11:18:50.633628887 -0700
> > +++ b/include/linux/mmap_lock.h       2026-04-29 11:18:50.707631737 -0700
> > @@ -470,6 +470,8 @@ static inline void vma_mark_detached(str
> >
> >  struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >                                         unsigned long address);
> > +struct vm_area_struct *lock_vma_under_rcu_wait(struct mm_struct *mm,
> > +                                       unsigned long address);
> >
> >  /*
> >   * Locks next vma pointed by the iterator. Confirms the locked vma has not
> > diff -puN mm/mmap_lock.c~lock-vma-under-rcu-wait mm/mmap_lock.c
> > --- a/mm/mmap_lock.c~lock-vma-under-rcu-wait  2026-04-29 11:18:50.704631622 -0700
> > +++ b/mm/mmap_lock.c  2026-04-29 11:18:50.707631737 -0700
> > @@ -340,6 +340,49 @@ inval:
> >       return NULL;
> >  }
> >
> > +/*
> > + * Find the VMA covering 'address' and lock it for reading. Waits for writers to
> > + * finish if the VMA is being modified. Returns NULL if there is no VMA covering
> > + * 'address'.
> > + *
> > + * The fast path does not take mmap lock.
> > + */
> > +struct vm_area_struct *lock_vma_under_rcu_wait(struct mm_struct *mm,
> > +                                            unsigned long address)
> > +{
> > +     struct vm_area_struct *vma;
> > +
> > +retry:
> > +     vma = lock_vma_under_rcu(mm, address);
> > +     /* Fast path: return stable VMA covering 'address': */
> > +     if (vma)
> > +             return vma;
> > +
> > +     /*
> > +      * Slow path: the VMA covering 'address' is being modified.
> > +      * or there is no VMA covering 'address'. Rule out the
> > +      * possibility that the VMA is being modified:
> > +      */
> > +     mmap_read_lock(mm);
> > +     vma = vma_lookup(mm, address);
> > +     mmap_read_unlock(mm);
> > +
> > +     /* There was for sure no VMA covering 'address': */
> > +     if (!vma)
> > +             return NULL;
> > +
> > +     /*
> > +      * VMA was likely being modified during RCU lookup. Try again.
> > +      * mmap_read_lock() waited for the writer to complete and the
> > +      * writer is now done.
> > +      *
> > +      * There is no guarantee that any single retry will succeed,
> > +      * and it is possible but highly unlikely this will loop
> > +      * forever.
> > +      */
> > +     goto retry;
> > +}
>
> Hmm yeah this is not ideal :)
>
> You don't have to do any of this we already have logic to help out here -
> vma_start_read_locked().

+1

>
> That uses the fact the mmap read lock is held to pin the VMA lock, because VMA
> write locks require an mmap write lock, and the mmap read lock prevents them.
>
> That way you can eliminate the retry.
>
> So instead:
>
> /*
>  * Tries to lock under RCU, failing that it acquires the VMA lock with the mmap
>  * read lock held.
>  */
> struct vm_area_struct *vma_start_read_unlocked(struct mm_struct *mm,
>                                                unsigned long address)
> {
>         struct vm_area_struct *vma;
>
>         might_sleep();
>
>         vma = lock_vma_under_rcu(mm, address);
>         if (vma)
>                 return vma;
>
>         /* Slow path: preclude VMA writers by getting mmap read lock. */
>         guard(rwsem_read)(&mm->mmap_lock);
>         vma = vma_lookup(mm, address);
>         /* VMA isn't there. */
>         if (!vma)
>                 return NULL;
>
>         return vma_start_read_locked(vma);
> }
>
> (Untested, not even build tested code)

uffd_lock_vma() has beed merged for a while, so that one is tested and
can be used as a model. Unfortunately it has some additional logic, so
we can't simply replace it with a new generic version, I think.

>
> Not sure if we can use the linux/cleanup.h guard here, because mmap_read_lock()
> also does some trace stuff, but the guard makes it WAY nicer so (when it goes
> out of scope the mmap read lock is dropped).
>
> Maybe we could add a custom mmap lock guard to cover that too?
>
> Suren - I actually wonder if vma_start_read_locked() actually needs to return a
> boolean? The failure cases for __refcount_inc_not_zero_limited_acquire() are -
> detached or excluding readers on write/detach.
>
> But in both of those cases, vma_lookup() would surely not find the VMA, and
> since we're precluding writers (so no write lock, no detach possible) that means
> we should never hit it?
>
> Wonder if we should make it a void and add a VM_WARN_ON_ONCE()?

vma_start_read_locked_nested() uses
__refcount_inc_not_zero_limited_acquire(..., VM_REFCNT_LIMIT) to
protect the refcount from overflowing into
VM_REFCNT_EXCLUDE_READERS_FLAG bit. So, even though this is a very
unlikely case, we still need to consider it and return failure even
when there is no contention.

>
>
> > +
> >  static struct vm_area_struct *lock_next_vma_under_mmap_lock(struct mm_struct *mm,
> >                                                           struct vma_iterator *vmi,
> >                                                           unsigned long from_addr)
> > _
>
> Cheers, Lorenzo