Re: [RESEND][PATCH v21 1/6] locking: Add task::blocked_lock to serialize blocked_on state

[John Stultz]

On Thu, Sep 11, 2025 at 10:50 PM 'K Prateek Nayak' via kernel-team
<kernel-team@xxxxxxxxxxx> wrote:
> On 9/4/2025 5:51 AM, John Stultz wrote:
> > @@ -693,25 +697,30 @@ __mutex_lock_common(struct mutex *lock, unsigned int state, unsigned int subclas
> >                       break;
> >
> >               if (first) {
> > -                     trace_contention_begin(lock, LCB_F_MUTEX | LCB_F_SPIN);
> > +                     bool opt_acquired;
> > +
> >                       /*
> >                        * mutex_optimistic_spin() can call schedule(), so
> > -                      * clear blocked on so we don't become unselectable
> > +                      * we need to release these locks before calling it,
> > +                      * and clear blocked on so we don't become unselectable
> >                        * to run.
> >                        */
> > -                     clear_task_blocked_on(current, lock);
> > -                     if (mutex_optimistic_spin(lock, ww_ctx, &waiter))
> > +                     __clear_task_blocked_on(current, lock);
> > +                     raw_spin_unlock(&current->blocked_lock);
> > +                     raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
> > +                     trace_contention_begin(lock, LCB_F_MUTEX | LCB_F_SPIN);
> > +                     opt_acquired = mutex_optimistic_spin(lock, ww_ctx, &waiter);
> > +                     raw_spin_lock_irqsave(&lock->wait_lock, flags);
> > +                     raw_spin_lock(&current->blocked_lock);
> > +                     __set_task_blocked_on(current, lock);
> > +                     if (opt_acquired)
> >                               break;
>
> nit. Is there any reason for setting the blocked state before the break
> other than for symmetry? The blocked_lock is held and we'll soon clear
> it after the break anyways so does it have any other subtle purpose?

So, basically yes, it's for symmetry, and trying to avoid subtlety
where I've found it's easy to confuse myself.

At this point in the patch series, as we don't prevent double-clears,
I guess it could be moved below the break, without much impact.
However, after the "Add blocked_on_state" patch, we do enforce that we
we don't clear cleared values, and we manage the blocked_on_state
separately here, so at that point we'd have to put it back or loosen
the constraint checking. So I think leaving it here seems the most
sensible.

> Also super silly but that above block is too dense. Can we add some
> spaces in between, groping the relevant ops, to make it easier on the
> eyes :)

Sure! Done!

> Apart from those nit picks, I don't see anything out of the place.
> Feel free to include:
>
> Reviewed-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>
>

Thanks so much again for your review and feedback!
-john