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

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

On Fri, Jan 23, 2026 at 11:02:04AM +0100, Vlastimil Babka wrote:
> On 1/22/26 14:01, Lorenzo Stoakes wrote:
> > It is confusing to have __vma_enter_exclusive_locked() 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 lock 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_enter_exclusive_locked() 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.
> >
> > No functional change intended.
> >
> > Signed-off-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> > ---
> >  mm/mmap_lock.c | 144 +++++++++++++++++++++++++++++++------------------
> >  1 file changed, 91 insertions(+), 53 deletions(-)
> >
> > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > index f73221174a8b..75166a43ffa4 100644
> > --- a/mm/mmap_lock.c
> > +++ b/mm/mmap_lock.c
> > @@ -46,20 +46,40 @@ EXPORT_SYMBOL(__mmap_lock_do_trace_released);
> >  #ifdef CONFIG_MMU
> >  #ifdef CONFIG_PER_VMA_LOCK
> >
> > +/* State shared across __vma_[enter, exit]_exclusive_locked(). */
> > +struct vma_exclude_readers_state {
> > +	/* Input parameters. */
> > +	struct vm_area_struct *vma;
> > +	int state; /* TASK_KILLABLE or TASK_UNINTERRUPTIBLE. */
> > +	bool detaching;
> > +
> > +	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_exit_exclusive_locked(struct vm_area_struct *vma)
> > +static void __vma_exit_exclusive_locked(struct vma_exclude_readers_state *ves)
> >  {
> > -	bool detached;
> > +	struct vm_area_struct *vma = ves->vma;
> > +
> > +	VM_WARN_ON_ONCE(ves->detached);
> > +	VM_WARN_ON_ONCE(!ves->exclusive);
>
> I think this will triger when called on wait failure from
> __vma_enter_exclusive_locked(). Given the other things Suren raised about
> the field, I wonder if it's worth keeping it?

He was suggesting I use ves->exclusive over ves->detached? I've now actioned
those changes so... yeh not dropping that.

But you're right that assert is wrong, removed.

>
> > -	detached = refcount_sub_and_test(VM_REFCNT_EXCLUDE_READERS_FLAG,
> > -					 &vma->vm_refcnt);
> > +	ves->detached = refcount_sub_and_test(VM_REFCNT_EXCLUDE_READERS_FLAG,
> > +					      &vma->vm_refcnt);
> >  	__vma_lockdep_release_exclusive(vma);
> > -	return detached;
> > +}
> > +
>
> > @@ -151,7 +176,12 @@ EXPORT_SYMBOL_GPL(__vma_start_write);
> >
> >  void vma_mark_detached(struct vm_area_struct *vma)
> >  {
> > -	bool detached;
> > +	struct vma_exclude_readers_state ves = {
> > +		.vma = vma,
> > +		.state = TASK_UNINTERRUPTIBLE,
> > +		.detaching = true,
> > +	};
> > +	int err;
> >
> >  	vma_assert_write_locked(vma);
> >  	vma_assert_attached(vma);
> > @@ -160,18 +190,26 @@ void vma_mark_detached(struct vm_area_struct *vma)
> >  	 * See the comment describing the vm_area_struct->vm_refcnt field for
> >  	 * details of possible refcnt values.
> >  	 */
> > -	detached = __vma_refcount_put(vma, NULL);
> > -	if (unlikely(!detached)) {
> > -		/* Wait until vma is detached with no readers. */
> > -		if (__vma_enter_exclusive_locked(vma, true, TASK_UNINTERRUPTIBLE)) {
> > -			/*
> > -			 * Once this is complete, no readers can increment the
> > -			 * reference count, and the VMA is marked detached.
> > -			 */
> > -			detached = __vma_exit_exclusive_locked(vma);
> > -			WARN_ON_ONCE(!detached);
> > -		}
> > +	if (likely(__vma_refcount_put(vma, NULL)))
> > +		return;
>
> Seems to me it would be worthwhile splitting this function to an
> static-inline-in-header vma_mark_detached() that does only the asserts and
> __vma_refcount_put(), and keeping the function here as __vma_mark_detached()
> (or maybe differently named since the detaching kinda already happened with
> the refcount put... __vma_mark_detached_finish()?) handling the rare case
> __vma_refcount_put() returns false.

Yeah good idea, that saves us always having the ves state etc. too and separates
it out nicely.

Have called it __vma_exclude_readers_for_detach(), and made the change.

>
> > +
> > +	/*
> > +	 * Wait until the VMA is detached with no readers. Since we hold the VMA
> > +	 * write lock, the only read locks that might be present are those from
> > +	 * threads trying to acquire the read lock and incrementing the
> > +	 * reference count before realising the write lock is held and
> > +	 * decrementing it.
> > +	 */
> > +	err = __vma_enter_exclusive_locked(&ves);
> > +	if (!err && !ves.detached) {
> > +		/*
> > +		 * Once this is complete, no readers can increment the
> > +		 * reference count, and the VMA is marked detached.
> > +		 */
> > +		__vma_exit_exclusive_locked(&ves);
> >  	}
> > +	/* If an error arose but we were detached anyway, we don't care. */
> > +	WARN_ON_ONCE(!ves.detached);
> >  }
> >
> >  /*
> > --
> > 2.52.0
>

Cheers, Lorenzo