Re: [PATCH v4 07/10] mm/vma: introduce helper struct + thread through exclusive lock fns

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

Andrew - could we change the commit message to:

-->

It is confusing to have __vma_start_exclude_readers() return 0, 1 or an
error (but only when waiting for readers in TASK_KILLABLE state), and
having the return value be stored in a stack variable called 'locked' is
further confusion.

More generally, we are doing a lot of rather finnicky things during the
acquisition of a state in which readers are excluded and moving out of this
state, including tracking whether we are detached or not or whether an
error occurred.

We are implementing logic in __vma_start_exclude_readers() that effectively
acts as if 'if one caller calls us do X, if another then do Y', which is
very confusing from a control flow perspective.

Introducing the shared helper object state helps us avoid this, as we can
now handle the 'an error arose but we're detached' condition correctly in
both callers - a warning if not detaching, and treating the situation as if
no error arose in the case of a VMA detaching.

This also acts to help document what's going on and allows us to add some
more logical debug asserts.

Also update vma_mark_detached() to add a guard clause for the likely
'already detached' state (given we hold the mmap write lock), and add a
comment about ephemeral VMA read lock reference count increments to clarify
why we are entering/exiting an exclusive locked state here.

Finally, separate vma_mark_detached() into its fast-path component and make
it inline, then place the slow path for excluding readers in mmap_lock.c.

No functional change intended.

<--

Please as per Vlasta's comments below? Thanks!

Also could you sed the patch with:

s/__vma_exit_exclusive_locked/__vma_end_exclude_readers/
s/__vma_[enter, exit]_exclusive_locked/__vma_[start, end]_exclude_readers/

As per Vlasta's comments below?

As I have clearly forgotten to do this bit myself... doh!

Also at the bottom there is one small correction to a comment there too.

If it's too much of a pain I can sort out a fix-patch.

Thanks!

On Mon, Jan 26, 2026 at 12:16:07PM +0100, Vlastimil Babka wrote:
> On 1/23/26 21:12, Lorenzo Stoakes wrote:
> > It is confusing to have __vma_enter_exclusive_locked() return 0, 1 or an
>
> It's now __vma_start_exclude_readers()
>
> > error (but only when waiting for readers in TASK_KILLABLE state), and
> > having the return value be stored in a stack variable called 'locked' is
> > further confusion.
> >
> > More generally, we are doing a lock of rather finnicky things during the
>
> 				 ^ lot?
>
> > acquisition of a state in which readers are excluded and moving out of this
> > state, including tracking whether we are detached or not or whether an
> > error occurred.
> >
> > We are implementing logic in __vma_enter_exclusive_locked() that
>
> again __vma_start_exclude_readers()
>
> > effectively acts as if 'if one caller calls us do X, if another then do Y',
> > which is very confusing from a control flow perspective.
> >
> > Introducing the shared helper object state helps us avoid this, as we can
> > now handle the 'an error arose but we're detached' condition correctly in
> > both callers - a warning if not detaching, and treating the situation as if
> > no error arose in the case of a VMA detaching.
> >
> > This also acts to help document what's going on and allows us to add some
> > more logical debug asserts.
> >
> > Also update vma_mark_detached() to add a guard clause for the likely
> > 'already detached' state (given we hold the mmap write lock), and add a
> > comment about ephemeral VMA read lock reference count increments to clarify
> > why we are entering/exiting an exclusive locked state here.
> >
> > Finally, separate vma_mark_detached() into its fast-path component and make
> > it inline, then place the slow path for excluding readers in mmap_lock.c.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
>
> Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
>
> Great improvement, thanks.

Thanks! I will refrain from saying thanks on all of your tags without nits
btw to save the noise ;)

Addressed nits above/below with kind plea to Andrew to fix up my typos :)

Cheers, Lorenzo

>
> Just some more nits wrt naming.
>
> > ---
> >  include/linux/mm_types.h  |  14 ++--
> >  include/linux/mmap_lock.h |  23 +++++-
> >  mm/mmap_lock.c            | 152 +++++++++++++++++++++-----------------
> >  3 files changed, 112 insertions(+), 77 deletions(-)
> >
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 12281a1128c9..ca47a5d3d71e 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -1011,15 +1011,15 @@ struct vm_area_struct {
> >  	 * decrementing it again.
> >  	 *
> >  	 * VM_REFCNT_EXCLUDE_READERS_FLAG - Detached, pending
> > -	 * __vma_exit_locked() completion which will decrement the reference
> > -	 * count to zero. IMPORTANT - at this stage no further readers can
> > -	 * increment the reference count. It can only be reduced.
> > +	 * __vma_exit_exclusive_locked() completion which will decrement the
>
> __vma_end_exclude_readers()
>
> > +	 * reference count to zero. IMPORTANT - at this stage no further readers
> > +	 * can increment the reference count. It can only be reduced.
> >  	 *
> >  	 * VM_REFCNT_EXCLUDE_READERS_FLAG + 1 - A thread is either write-locking
> > -	 * an attached VMA and has yet to invoke __vma_exit_locked(), OR a
> > -	 * thread is detaching a VMA and is waiting on a single spurious reader
> > -	 * in order to decrement the reference count. IMPORTANT - as above, no
> > -	 * further readers can increment the reference count.
> > +	 * an attached VMA and has yet to invoke __vma_exit_exclusive_locked(),
>
> __vma_end_exclude_readers()
>
> (also strictly speaking, these would belong to the previous patch, but not
> worth the trouble moving)
>
> > +	 * OR a thread is detaching a VMA and is waiting on a single spurious
> > +	 * reader in order to decrement the reference count. IMPORTANT - as
> > +	 * above, no further readers can increment the reference count.
> >  	 *
> >  	 * > VM_REFCNT_EXCLUDE_READERS_FLAG + 1 - A thread is either
> >  	 * write-locking or detaching a VMA is waiting on readers to
> > diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> > index d6df6aad3e24..678f90080fa6 100644
> > --- a/include/linux/mmap_lock.h
> > +++ b/include/linux/mmap_lock.h
> > @@ -358,7 +358,28 @@ static inline void vma_mark_attached(struct vm_area_struct *vma)
> >  	refcount_set_release(&vma->vm_refcnt, 1);
> >  }
> >
> > -void vma_mark_detached(struct vm_area_struct *vma);
> > +void __vma_exclude_readers_for_detach(struct vm_area_struct *vma);
> > +
> > +static inline void vma_mark_detached(struct vm_area_struct *vma)
> > +{
> > +	vma_assert_write_locked(vma);
> > +	vma_assert_attached(vma);
> > +
> > +	/*
> > +	 * The VMA still being attached (refcnt > 0) - is unlikely, because the
> > +	 * vma has been already write-locked and readers can increment vm_refcnt
> > +	 * only temporarily before they check vm_lock_seq, realize the vma is
> > +	 * locked and drop back the vm_refcnt. That is a narrow window for
> > +	 * observing a raised vm_refcnt.
> > +	 *
> > +	 * See the comment describing the vm_area_struct->vm_refcnt field for
> > +	 * details of possible refcnt values.
> > +	 */
> > +	if (likely(!__vma_refcount_put_return(vma)))
> > +		return;
> > +
> > +	__vma_exclude_readers_for_detach(vma);
> > +}
> >
> >  struct vm_area_struct *lock_vma_under_rcu(struct mm_struct *mm,
> >  					  unsigned long address);
> > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > index 72f15f606093..b523a3fe110c 100644
> > --- a/mm/mmap_lock.c
> > +++ b/mm/mmap_lock.c
> > @@ -46,20 +46,38 @@ EXPORT_SYMBOL(__mmap_lock_do_trace_released);
> >  #ifdef CONFIG_MMU
> >  #ifdef CONFIG_PER_VMA_LOCK
> >
> > +/* State shared across __vma_[enter, exit]_exclusive_locked(). */
>
> __vma_[start,end]_exclude_readers
>
> > +struct vma_exclude_readers_state {
> > +	/* Input parameters. */
> > +	struct vm_area_struct *vma;
> > +	int state; /* TASK_KILLABLE or TASK_UNINTERRUPTIBLE. */
> > +	bool detaching;
> > +
> > +	/* Output parameters. */
> > +	bool detached;
> > +	bool exclusive; /* Are we exclusively locked? */
> > +};
> > +
> >  /*
> >   * Now that all readers have been evicted, mark the VMA as being out of the
> >   * 'exclude readers' state.
> > - *
> > - * Returns true if the VMA is now detached, otherwise false.
> >   */
> > -static bool __must_check __vma_end_exclude_readers(struct vm_area_struct *vma)
> > +static void __vma_end_exclude_readers(struct vma_exclude_readers_state *ves)
> >  {
> > -	bool detached;
> > +	struct vm_area_struct *vma = ves->vma;
> >
> > -	detached = refcount_sub_and_test(VM_REFCNT_EXCLUDE_READERS_FLAG,
> > -					 &vma->vm_refcnt);
> > +	VM_WARN_ON_ONCE(ves->detached);
> > +
> > +	ves->detached = refcount_sub_and_test(VM_REFCNT_EXCLUDE_READERS_FLAG,
> > +					      &vma->vm_refcnt);
> >  	__vma_lockdep_release_exclusive(vma);
> > -	return detached;
> > +}
> > +
> > +static unsigned int get_target_refcnt(struct vma_exclude_readers_state *ves)
> > +{
> > +	const unsigned int tgt = ves->detaching ? 0 : 1;
> > +
> > +	return tgt | VM_REFCNT_EXCLUDE_READERS_FLAG;
> >  }
> >
> >  /*
> > @@ -69,32 +87,29 @@ static bool __must_check __vma_end_exclude_readers(struct vm_area_struct *vma)
> >   * Note that this function pairs with vma_refcount_put() which will wake up this
> >   * thread when it detects that the last reader has released its lock.
> >   *
> > - * The state parameter ought to be set to TASK_UNINTERRUPTIBLE in cases where we
> > - * wish the thread to sleep uninterruptibly or TASK_KILLABLE if a fatal signal
> > - * is permitted to kill it.
> > + * The ves->state parameter ought to be set to TASK_UNINTERRUPTIBLE in cases
> > + * where we wish the thread to sleep uninterruptibly or TASK_KILLABLE if a fatal
> > + * signal is permitted to kill it.
> >   *
> > - * The function will return 0 immediately if the VMA is detached, or wait for
> > - * readers and return 1 once they have all exited, leaving the VMA exclusively
> > - * locked.
> > + * The function sets the ves->exclusive parameter to true if readers were
> > + * excluded, or false if the VMA was detached or an error arose on wait.
> >   *
> > - * If the function returns 1, the caller is required to invoke
> > - * __vma_end_exclude_readers() once the exclusive state is no longer required.
> > + * If the function indicates an exclusive lock was acquired via ves->exclusive
> > + * the caller is required to invoke __vma_end_exclude_readers() once the
> > + * exclusive state is no longer required.
> >   *
> > - * If state is set to something other than TASK_UNINTERRUPTIBLE, the function
> > - * may also return -EINTR to indicate a fatal signal was received while waiting.
> > + * If ves->state is set to something other than TASK_UNINTERRUPTIBLE, the
> > + * function may also return -EINTR to indicate a fatal signal was received while
> > + * waiting.
>
> It says "may also return..." but now doesn't say anywhere that otherwise
> it's always 0.

Ack. Andrew could you append ' Otherwise, the function returns 0' to the
final paragraph above? Thanks!

>
> >   */
> > -static int __vma_start_exclude_readers(struct vm_area_struct *vma,
> > -		bool detaching, int state)
> > +static int __vma_start_exclude_readers(struct vma_exclude_readers_state *ves)