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

[Suren Baghdasaryan <surenb@google.com>]

On Fri, Jan 23, 2026 at 12:04 PM Lorenzo Stoakes
<lorenzo.stoakes@oracle.com> wrote:
>
> 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.

Sorry, I didn't realize I was causing that much trouble and I
understand your frustration.
From your reply, it sounds like you made enough changes to the patch
that my concern might already be obsolete. I'll review the new
submission on Sunday and will provide my feedback.
Thanks,
Suren.

>
> Thanks, Lorenzo