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

[Vlastimil Babka <vbabka@suse.cz>]

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?

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

> +
> +	/*
> +	 * 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