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

Lorenzo Stoakes
2026-01-23T18:19:02Z

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