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

Lorenzo Stoakes
2026-01-23T20:06:10Z

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