Message-ID: <108e3f4e-cd4f-452e-9bd2-1a5b892f6d60@lucifer.local>
Date: Fri, 23 Jan 2026 14:41:42 +0000
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: Vlastimil Babka <vbabka@...e.cz>,
        Andrew Morton <akpm@...ux-foundation.org>,
        David Hildenbrand <david@...nel.org>,
        "Liam R . Howlett" <Liam.Howlett@...cle.com>,
        Mike Rapoport <rppt@...nel.org>, Michal Hocko <mhocko@...e.com>,
        Shakeel Butt <shakeel.butt@...ux.dev>, Jann Horn <jannh@...gle.com>,
        linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        linux-rt-devel@...ts.linux.dev, Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>, Will Deacon <will@...nel.org>,
        Boqun Feng <boqun.feng@...il.com>, Waiman Long <longman@...hat.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Clark Williams <clrkwllms@...nel.org>,
        Steven Rostedt <rostedt@...dmis.org>
Subject: Re: [PATCH RESEND v3 03/10] mm/vma: rename is_vma_write_only(),
 separate out shared refcount put

On Thu, Jan 22, 2026 at 11:31:05AM -0800, Suren Baghdasaryan wrote:
> On Thu, Jan 22, 2026 at 9:36 AM Vlastimil Babka <vbabka@...e.cz> 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).
>
> Sorry, by mistake I replied to an earlier version here:
> https://lore.kernel.org/all/CAJuCfpF-tVr==bCf-PXJFKPn99yRjfONeDnDtPvTkGUfyuvtcw@mail.gmail.com/
> Copying my comments here.
>
> IIUC, __refcount_dec_and_test() can decrement the refcount by only 1
> and the old value returned (oldcnt) will be the exact value that it
> was before this decrement. Therefore oldcnt - 1 must reflect the

Yes.

> refcount value after the decrement. It's possible the refcount gets

Well no...

> manipulated after this operation but that does not make this operation
> wrong. I don't quite understand why you think that's racy or finnicky.

...because of this.

I only mentioned in passing that you might get raced, which you agree with
so I think that's fine.

_I_ feel this function is _very_ finnicky given the various caveats
required to understand exactly what's happening here. But clearly that's
distracting, so I'll rephrase it.

Have updated commit msg to say:

"
Relatedly, we use a __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.

This reduces duplication in the two invocations of this function.
"

(Adding the point about duplication since it wasn't clear before).

> > > +{
> > > +     int oldcnt;
> > > +     bool detached;
> > > +
> > > +     detached = __refcount_dec_and_test(&vma->vm_refcnt, &oldcnt);
> > > +     if (refcnt)
> > > +             *refcnt = oldcnt - 1;
> > > +     return detached;
>
> IIUC there is always a connection between detached and *refcnt
> resulting value. If detached==true then the resulting *refcnt has to
> be 0. If so, __vma_refcount_put() can simply return (oldcnt - 1) as
> new count:
>
> static inline int __vma_refcount_put(struct vm_area_struct *vma)
> {
>        int oldcnt;
>
>        __refcount_dec_and_test(&vma->vm_refcnt, &oldcnt);

You can't do this as it's __must_check... :)

So have to replace with __refcount_dec(), which is a void function.

>        return oldcnt - 1;
> }
>
> And later:
>
> newcnt = __vma_refcount_put(&vma->vm_refcnt);
> detached = newcnt == 0;

This is kind of horrible though.

Maybe better to just do:

	newcnt = __vma_refcount_put(vma);
	...
	if (newcnt && __vma_are_readers_excluded(newcnt))
		...

And:

	if (unlikely(__vma_refcount_put(vma))) {
		...
	}

And now we have vma->vm_refcnt documented clearly we can safely assume
developers understand what this means.

>
> > > +}
> > > +
> > > +/**
> > > + * vma_refcount_put() - Drop reference count in VMA vm_refcnt field due to a
> > > + * read-lock being dropped.
> > > + * @vma: The VMA whose reference count we wish to decrement.
> > > + *
> > > + * If we were the last reader, wake up threads waiting to obtain an exclusive
> > > + * lock.
> > > + */
> > >  static inline void vma_refcount_put(struct vm_area_struct *vma)
> > >  {
> > > -     /* Use a copy of vm_mm in case vma is freed after we drop vm_refcnt */
> > > +     /* Use a copy of vm_mm in case vma is freed after we drop vm_refcnt. */
> > >       struct mm_struct *mm = vma->vm_mm;
> > > -     int oldcnt;
> > > +     int refcnt;
> > > +     bool detached;
> > >
> > >       rwsem_release(&vma->vmlock_dep_map, _RET_IP_);
> > > -     if (!__refcount_dec_and_test(&vma->vm_refcnt, &oldcnt)) {
> > >
> > > -             if (is_vma_writer_only(oldcnt - 1))
> > > -                     rcuwait_wake_up(&mm->vma_writer_wait);
> > > -     }
> > > +     detached = __vma_refcount_put(vma, &refcnt);
> > > +     /*
> > > +      * __vma_enter_locked() may be sleeping waiting for readers to drop
> > > +      * their reference count, so wake it up if we were the last reader
> > > +      * blocking it from being acquired.
> > > +      */
> > > +     if (!detached && are_readers_excluded(refcnt))
> > > +             rcuwait_wake_up(&mm->vma_writer_wait);
> > >  }
> > >
> > >  /*
> > > diff --git a/mm/mmap_lock.c b/mm/mmap_lock.c
> > > index 75dc098aea14..ebacb57e5f16 100644
> > > --- a/mm/mmap_lock.c
> > > +++ b/mm/mmap_lock.c
> > > @@ -130,25 +130,23 @@ EXPORT_SYMBOL_GPL(__vma_start_write);
> > >
> > >  void vma_mark_detached(struct vm_area_struct *vma)
> > >  {
> > > +     bool detached;
> > > +
> > >       vma_assert_write_locked(vma);
> > >       vma_assert_attached(vma);
> > >
> > >       /*
> > > -      * We are the only writer, so no need to use vma_refcount_put().
> > > -      * The condition below is unlikely because the vma has been already
> > > -      * write-locked and readers can increment vm_refcnt only temporarily
>
> I think the above part of the comment is still important and should be
> kept intact.

I think it's confusing, because 'we are the only writer' is not clear as to
why it obviates the need to call vma_refcount_put().

In fact vma_refcount_put() explicitly invokes a lockdep read lock drop
which would be incorrect to invoke here. Also I guess the other point here
is we don't do wake ups because nobody else will be waiting.

I think updating this to actually explain the context would be hugely
distracting and not all that useful, but leaving it as-is is confusing.

So I think the best thing to do is to add back the thing about the
condition, which I've done but but placed it above unlikely(!detached) as:

	/*
	 * This condition - that the VMA is still 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.
	 */

>
> > > -      * 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 (unlikely(!refcount_dec_and_test(&vma->vm_refcnt))) {
> > > +     detached = __vma_refcount_put(vma, NULL);
> > > +     if (unlikely(!detached)) {
> > >               /* Wait until vma is detached with no readers. */
> > >               if (__vma_enter_locked(vma, true, TASK_UNINTERRUPTIBLE)) {
> > > -                     bool detached;
> > > -
> > > +                     /*
> > > +                      * Once this is complete, no readers can increment the
> > > +                      * reference count, and the VMA is marked detached.
> > > +                      */
> > >                       __vma_exit_locked(vma, &detached);
> > >                       WARN_ON_ONCE(!detached);
> > >               }
> > > --
> > > 2.52.0
> >

Powered by blists - more mailing lists