linux-kernel - Re: [RESEND][PATCH v21 1/6] locking: Add task::blocked_lock to serialize blocked

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANDhNCpJaE4Y=SvvMFFM90uLkabBygGnfqAfL1YH=JDXTSEVmg@mail.gmail.com>
Date: Thu, 18 Sep 2025 12:58:59 -0700
From: John Stultz <jstultz@...gle.com>
To: K Prateek Nayak <kprateek.nayak@....com>
Cc: LKML <linux-kernel@...r.kernel.org>, Joel Fernandes <joelagnelf@...dia.com>, 
	Qais Yousef <qyousef@...alina.io>, Ingo Molnar <mingo@...hat.com>, 
	Peter Zijlstra <peterz@...radead.org>, Juri Lelli <juri.lelli@...hat.com>, 
	Vincent Guittot <vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>, 
	Valentin Schneider <vschneid@...hat.com>, Steven Rostedt <rostedt@...dmis.org>, 
	Ben Segall <bsegall@...gle.com>, Zimuzo Ezeozue <zezeozue@...gle.com>, Mel Gorman <mgorman@...e.de>, 
	Will Deacon <will@...nel.org>, Waiman Long <longman@...hat.com>, Boqun Feng <boqun.feng@...il.com>, 
	"Paul E. McKenney" <paulmck@...nel.org>, Metin Kaya <Metin.Kaya@....com>, 
	Xuewen Yan <xuewen.yan94@...il.com>, Thomas Gleixner <tglx@...utronix.de>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Suleiman Souhlal <suleiman@...gle.com>, 
	kuyo chang <kuyo.chang@...iatek.com>, hupu <hupu.gm@...il.com>, kernel-team@...roid.com
Subject: Re: [RESEND][PATCH v21 1/6] locking: Add task::blocked_lock to
 serialize blocked_on state

On Thu, Sep 11, 2025 at 10:50 PM 'K Prateek Nayak' via kernel-team
<kernel-team@...roid.com> 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@....com>
>

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