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:34:25AM -0800, Suren Baghdasaryan wrote:
> > > >  int __vma_start_write(struct vm_area_struct *vma, unsigned int mm_lock_seq,
> > > >                 int state)
> > > >  {
> > > > -       int locked;
> > > > +       int err;
> > > > +       struct vma_exclude_readers_state ves = {
> > > > +               .vma = vma,
> > > > +               .state = state,
> > > > +       };
> > > >
> > > > -       locked = __vma_enter_exclusive_locked(vma, false, state);
> > > > -       if (locked < 0)
> > > > -               return locked;
> > > > +       err = __vma_enter_exclusive_locked(&ves);
> > > > +       if (err) {
> > > > +               WARN_ON_ONCE(ves.detached);
> > >
> > > I believe the above WARN_ON_ONCE() should stay inside of
> > > __vma_enter_exclusive_locked(). Its correctness depends on the
> > > implementation details of __vma_enter_exclusive_locked(). More
> >
> > Well this was kind of horrible in the original implementation, as you are
> > literally telling the function whether you are detaching or not, and only doing
> > this assert if you were not.
> >
> > That kind of 'if the caller is X do A, if the caller is Y do B' is really a code
> > smell, you should have X do the thing.
> >
> > > specifically, it is only correct because
> > > __vma_enter_exclusive_locked() returns 0 if the VMA is detached, even
> > > if there was a pending SIGKILL.
> >
> > Well it's a documented aspect of the function that we return 0 immediately on
> > detached state so I'm not sure that is an implementation detail?
> >
> > I significantly prefer having that here vs. 'if not detaching then assert if
> > detached' for people to scratch their heads over in the function.
> >
> > I think this detail is incorrect anyway, because:
> >
> >         if (err) {
> >                 if (__vma_exit_exclusive_locked(vma)) {
> >                         /*
> >                          * The wait failed, but the last reader went away
> >                          * as well. Tell the caller the VMA is detached.
> >                          */
> >                          WARN_ON_ONCE(!detaching);
> >                          err = 0;
> >                 }
> >                 ...
> >         }
> >
> > Implies - hey we're fine with err not being zero AND detaching right? In which
> > case reset the error?
> >
> > Except when detaching we set TASK_UNINTERRUPTIBLE? Which surely means we never
> > seen an error?
> >
> > Or do we?
> >
> > Either way it's something we handle differently based on _caller_. So it doesn't
> > belong in the function at all.
> >
> > It's certainly logic that's highly confusing and needs to be handled
> > differently.
>
> Just to be clear, I'm not defending the way it is done before your
> change, however the old check for "if not detaching then assert if

I mean you basically are since here I am trying to change it and you're
telling me not to, so you are definitely defending this.

> detached" makes more sense to me than "if
> __vma_enter_exclusive_locked() failed assert that we VMA is still
> attached". The latter one does not make logical sense to me. It's only

I don't understand what you're quoting here?

> correct because of the implementation detail of
> __vma_enter_exclusive_locked().

Except that implementation detail no longer exists?


Before:

         if (err) {
                 if (__vma_exit_exclusive_locked(vma)) {
                         /*
                          * The wait failed, but the last reader went away
                          * as well. Tell the caller the VMA is detached.
                          */
                          WARN_ON_ONCE(!detaching);
                          err = 0;
                 }
                 ...
         }

After:

	if (err) {
		__vma_end_exclude_readers(ves);
		return err;
	}

So now each caller receives an error _and decides what to do with it_.

In __vma_exclude_readers_for_detach():


	err = __vma_start_exclude_readers(&ves);
	if (!err && ves.exclusive) {
		...
	}
	/* If an error arose but we were detached anyway, we don't care. */
	WARN_ON_ONCE(!ves.detached);

Right that's pretty clear? We expect to be detached no matter what, and the
comment points out that, yeah, err could result in detachment.

In the __vma_start_write() path:

	err = __vma_start_exclude_readers(&ves);
	if (err) {
		WARN_ON_ONCE(ves.detached);
		return err;
	}

I mean, yes we don't expect to be detached when we're acquiring a write.

Honestly I've spent the past 6 hours responding to review for a series I
really didn't want to write in the first place, updating and testing
etc. as I go, and I've essentially accepted every single point of feedback.

So I'm a little frustrated at getting stuck on this issue.

So I'm afraid I'm going to send the v4 out as-is and we can have a v5 (or
ideally, a fix-patch) if we have to, but you definitely need to be more
convincing about this.

I might just be wrong and missing the point out of tiredness but, at this
stage, I'm not going to hold up the respin over this.

Thanks, Lorenzo