Re: [PATCH RESEND v3 03/10] mm/vma: rename is_vma_write_only(), separate out shared refcount put

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

On Thu, Jan 22, 2026 at 06:36:03PM +0100, Vlastimil Babka wrote:
> On 1/22/26 14:01, Lorenzo Stoakes wrote:
> > The is_vma_writer_only() function is misnamed - this isn't determining if
> > there is only a write lock, as it checks for the presence of the
> > VM_REFCNT_EXCLUDE_READERS_FLAG.
> >
> > Really, it is checking to see whether readers are excluded, with a
> > possibility of a false positive in the case of a detachment (there we
> > expect the vma->vm_refcnt to eventually be set to
> > VM_REFCNT_EXCLUDE_READERS_FLAG, whereas for an attached VMA we expect it to
> > eventually be set to VM_REFCNT_EXCLUDE_READERS_FLAG + 1).
> >
> > Rename the function accordingly.
> >
> > Relatedly, we use a finnicky __refcount_dec_and_test() primitive directly
> > in vma_refcount_put(), using the old value to determine what the reference
> > count ought to be after the operation is complete (ignoring racing
> > reference count adjustments).
> >
> > Wrap this into a __vma_refcount_put() function, which we can then utilise
> > in vma_mark_detached() and thus keep the refcount primitive usage
> > abstracted.
> >
> > Also adjust comments, removing duplicative comments covered elsewhere and
> > adding more to aid understanding.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Again very useful, thanks!

Thanks.

>
> > ---
> >  include/linux/mmap_lock.h | 62 +++++++++++++++++++++++++++++++--------
> >  mm/mmap_lock.c            | 18 +++++-------
> >  2 files changed, 57 insertions(+), 23 deletions(-)
> >
> > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> > index a764439d0276..0b3614aadbb4 100644
> > --- a/include/linux/mmap_lock.h
> > +++ b/include/linux/mmap_lock.h
> > @@ -122,15 +122,27 @@ static inline void vma_lock_init(struct vm_area_struct *vma, bool reset_refcnt)
> >  	vma->vm_lock_seq = UINT_MAX;
> >  }
> >
> > -static inline bool is_vma_writer_only(int refcnt)
> > +/**
> > + * are_readers_excluded() - Determine whether @refcnt describes a VMA which has
> > + * excluded all VMA read locks.
> > + * @refcnt: The VMA reference count obtained from vm_area_struct->vm_refcnt.
> > + *
> > + * We may be raced by other readers temporarily incrementing the reference
> > + * count, though the race window is very small, this might cause spurious
> > + * wakeups.
>
> I think this part about spurious wakeups belongs more to the usage of the
> function in vma_refcount_put()? Because there are no wakeups done here. So
> it should be enough to explain how it can be false positive like in the
> paragraph below.

OK moved the paragraph to vma_refcount_put().

>
> > + *
> > + * In the case of a detached VMA, we may incorrectly indicate that readers are
> > + * excluded when one remains, because in that scenario we target a refcount of
> > + * VM_REFCNT_EXCLUDE_READERS_FLAG, rather than the attached target of
> > + * VM_REFCNT_EXCLUDE_READERS_FLAG + 1.
> > + *
> > + * However, the race window for that is very small so it is unlikely.
> > + *
> > + * Returns: true if readers are excluded, false otherwise.
> > + */
> > +static inline bool are_readers_excluded(int refcnt)
>
> I wonder if a include/linux/ header should have such a generically named
> function (I understand it's necessary for it to be here). Maybe prefix the
> name and make the comment not a kerneldoc because it's going to be only the
> vma locking implementation using it and not the vma locking end-users? (i.e.
> it's "intermediate").

OK, renamed to __vma_are_readers_excluded() and dropped the kdoc.

>
> >  {
> >  	/*
> > -	 * With a writer and no readers, refcnt is VM_REFCNT_EXCLUDE_READERS_FLAG
> > -	 * if the vma is detached and (VM_REFCNT_EXCLUDE_READERS_FLAG + 1) if it is
> > -	 * attached. Waiting on a detached vma happens only in
> > -	 * vma_mark_detached() and is a rare case, therefore most of the time
> > -	 * there will be no unnecessary wakeup.
> > -	 *
> >  	 * See the comment describing the vm_area_struct->vm_refcnt field for
> >  	 * details of possible refcnt values.
> >  	 */
> > @@ -138,18 +150,42 @@ static inline bool is_vma_writer_only(int refcnt)
> >  		refcnt <= VM_REFCNT_EXCLUDE_READERS_FLAG + 1;
> >  }
> >
> > +static inline bool __vma_refcount_put(struct vm_area_struct *vma, int *refcnt)
>
> Basically change are_readers_excluded() like this, with __vma prefix?

Yup did that already.

>
> But this one could IMHO use use some comment (also not kerneldoc) saying
> what the return value and *refcnt indicate?

I felt doing that would be overdocumenting... I've had people moan about that
before :) but sure makes sense.

Added:

/*
 * Actually decrement the VMA reference count.
 *
 * The functions sets *refcnt to the reference count immediately prior to the
 * decrement if refcnt is not NULL.
 *
 * Returns true if the decrement resulted in the VMA being detached
 * (i.e. reduced it to zero), or false otherwise.
 */

Cheers, Lorenzo