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

["Lorenzo Stoakes (ARM)" <ljs@kernel.org>]

On Mon, Aug 03, 2026 at 12:01:21PM -0700, Suren Baghdasaryan wrote:
> On Mon, Aug 3, 2026 at 4:28 AM Lorenzo Stoakes (ARM) <ljs@kernel.org> wrote:
> > >
> >
> > Would be nice to have a:
> >
> > Suggested-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
>
> Ack.

Thanks!

>
> >
> > Here given https://lore.kernel.org/linux-mm/af4Zx0gJIWbdDeY2@lucifer/ :)
> >

> > > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > > index e20d01e8d38f..6ff05e68e61b 100644
> > > --- a/mm/mmap_lock.c
> > > +++ b/mm/mmap_lock.c
> > > @@ -338,6 +338,35 @@ struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> > >       return NULL;
> > >  }
> > >
> > > +/*
> >
> > Why not a kdoc comment?
>
> Indeed. Will change.

Thanks!

>
> >
> > > + * 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'.
> > > + *
> > > + * Use only in code paths where no mmap_lock and no VMA lock is held.
> >
> > Well, a VMA read lock can be held which would make this a noop essentially.
> >
> > If a VMA write lock is held you're also ok as the mmap read lock will preclude
> > an mmap write lock, meaning vma_end_write_all() will have been called and the
> > write lock released.
> >
> > So I think you can just drop this line?
>
> Maybe instead of this I should say: Use when mmap_lock is not held,
> otherwise use vma_start_read_locked() ? I think that was the original
> reason for this comment.

Yeah that works thanks!

>
> >
> > > + *
> > > + * The fast path does not take mmap_lock.
> > > + */
> > > +struct vm_area_struct *vma_start_read_unlocked(struct mm_struct *mm,
> > > +                                            unsigned long address)
> > > +{
> > > +     struct vm_area_struct *vma;
> > > +
> > > +     /* Fast path: return stable VMA covering 'address': */
> > > +     vma = lock_vma_under_rcu(mm, address);
> > > +     if (vma)
> > > +             return vma;
> > > +
> > > +     /* Slow path: preclude VMA writers by temporarily getting mmap read lock. */
> > > +     mmap_read_lock(mm);
> > > +     vma = vma_lookup(mm, address);
> > > +     if (vma && !vma_start_read_locked(vma))
> >
> > This maybe warrants an unlikely() given it can only happen if refcount
> > overflows? Also worth having a comment to that effect here?
>
> vma_start_read_locked()
>   vma_start_read_locked()
>     vma_start_read_locked_nested()
>       unlikely(!__refcount_inc_not_zero_limited_acquire())
>
> already contains "unlikely" clause. Do we need to add it in all its users?
> I can modify the comment for vma_start_read_locked() stating that
> refcount overflow is very unlikely. Would that work or do you want
> callers to have that too?

Ah ok not an issue then. But good to add a comment at least?

> > > diff --git a/mm/userfaultfd.c b/mm/userfaultfd.c
> > > index edd90892f8cc..c3a0c38a3dc3 100644
> > > --- a/mm/userfaultfd.c
> > > +++ b/mm/userfaultfd.c
> > > @@ -129,8 +129,10 @@ struct vm_area_struct *find_vma_and_prepare_anon(struct mm_struct *mm,
> > >   *
> > >   * Should be called without holding mmap_lock.
> > >   *
> > > - * Return: A locked vma containing @address, -ENOENT if no vma is found, or
> > > - * -ENOMEM if anon_vma couldn't be allocated.
> > > + * Return: A locked vma containing @address, -ENOENT if no vma is found,
> > > + * -ENOMEM if anon_vma couldn't be allocated, or -EAGAIN if vma refcount
> > > + * overflow happened due to high number of readers and the caller should
> > > + * retry later.
> >
> > I'm guessing you're fixing this up as part of the patch? But it feels a bit
> > random, I mean fine but you should mention this change in the commit message +
> > explain why.
>
> I'm fixing that because Vlastimil asked about these inconsistencies in
> his previous review :)
> I can package these fixes as a separate patch or add a comment in the
> changelog, smth like "While at it, fix the comments for related
> functions". Would that work?

Comment in change log is fine thanks!

--
Cheers, Lorenzo