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

On Tue, Aug 4, 2026 at 1:47 AM Lorenzo Stoakes (ARM) <[email protected]> wrote:
>
> 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) <[email protected]> wrote:
> > > >
> > >
> > > Would be nice to have a:
> > >
> > > Suggested-by: Lorenzo Stoakes (ARM) <[email protected]>
> >
> > 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?

Will do. Thanks!

>
> > > > 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