Message-ID: <CANDhNCpmQLF_03t3PMEtjBU_tpL10FJ_iL=x3zMG+Bs0PEFESw@mail.gmail.com>
Date: Thu, 18 Sep 2025 15:57:10 -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 2/6] sched/locking: Add blocked_on_state to
 provide necessary tri-state for proxy return-migration

On Mon, Sep 15, 2025 at 1:06 AM K Prateek Nayak <kprateek.nayak@....com> wrote:
>
> Hello John,
>
> On 9/4/2025 5:51 AM, John Stultz wrote:
> >  static inline void __clear_task_blocked_on(struct task_struct *p, struct mutex *m)
> >  {
> > +     /* The task should only be clearing itself */
> > +     WARN_ON_ONCE(p != current);
> >       /* Currently we serialize blocked_on under the task::blocked_lock */
> >       lockdep_assert_held_once(&p->blocked_lock);
> > -     /*
> > -      * There may be cases where we re-clear already cleared
> > -      * blocked_on relationships, but make sure we are not
> > -      * clearing the relationship with a different lock.
> > -      */
> > -     WARN_ON_ONCE(m && p->blocked_on && p->blocked_on != m);
> > +     /* Make sure we are clearing the relationship with the right lock */
> > +     WARN_ON_ONCE(m && p->blocked_on != m);
> >       p->blocked_on = NULL;
> > +     p->blocked_on_state = BO_RUNNABLE;
> >  }
> >
>
> Maybe it is just me, but I got confused a couple of time only to
> realize __clear_task_blocked_on() clears the "blocked_on" and sets
> "blocked_on_state" to BO_RUNNABLE.
>
> Can we decouple the two and only set "p->blocked_on" in
> *_task_blocked_on_* and "p->blocked_on_state" in
> *{set,clear,force}_blocked_on* functions so it becomes easier to
> follow in the mutex path as:
>
>     __set_task_blocked_on(p, mutex); // blocked on mutex
>     __force_blocked_on_blocked(p); // blocked from running on CPU
>
>    ...
>
>     __clear_task_blocked_on(p, mutex); // p is no longer blocked on a mutex
>     __set_blocked_on_runnable(p); // p is now runnable

Hrm. So I guess I was thinking of
set_task_blocked_on()/clear_task_blocked_on() as the main enter/exit
states, with set_blocked_on_waking(), set_blocked_on_runnable() as
transition states within.

But, the various force_blocked_on_<state>() cases do add more
complexity, so maybe handling it completely separately would be more
intuitive? I dunno.


> >  static inline void clear_task_blocked_on(struct task_struct *p, struct mutex *m)
>
> Of the three {set,clear}_task_blcoked_on() usage:
>
>     $ grep -r "\(set\|clear\)_task_blocked_on" include/
>     kernel/locking/mutex.c: __set_task_blocked_on(current, lock);
>     kernel/locking/mutex.c: __clear_task_blocked_on(current, lock);
>     kernel/locking/mutex.c: clear_task_blocked_on(current, lock);
>
> two can be converted directly and perhaps clear_task_blocked_on() can be
> renamed as clear_task_blocked_on_set_runnable()?
>
> This is just me rambling on so feel free to ignore. I probably have to
> train my mind enough to see __clear_task_blocked_on() not only clears
> "blocked_on" but also sets task to runnable :)

Yeah, the case where we don't already hold the lock and want to do
both gets more complex in that case.

Hrm. Maybe just the way the functions are organized in the header make
it seem like we're managing two separate bits of state, where really
they are ordered.
I'll try to re-arrange that introducing the
set_task_blocked_on/clear_task_blocked_on first, then the transition
set_blocked_on_<state>() helpers after?
Maybe that and  some comments will make that clearer?

> > @@ -6749,6 +6776,15 @@ find_proxy_task(struct rq *rq, struct task_struct *donor, struct rq_flags *rf)
> >
> >       WARN_ON_ONCE(owner && !owner->on_rq);
> >       return owner;
> > +
> > +     /*
> > +      * NOTE: This logic is down here, because we need to call
> > +      * the functions with the mutex wait_lock and task
> > +      * blocked_lock released, so we have to get out of the
> > +      * guard() scope.
> > +      */
>
> I didn't know that was possible! Neat. Since cleanup.h has a note
> reading:
>
>     ... the expectation is that usage of "goto" and cleanup helpers is
>     never mixed in the same function.
>
> are there any concerns w.r.t. compiler versions etc. or am I just being
> paranoid?

Hrrrrmmmm.  I hadn't seen that detail. :/   I guess I was just lucky
it worked with my toolchain.

Oof. That may require reworking all this logic back to explicit
lock/unlock calls, away from the guard() usage.

Let me think on if there's a better way.

Thanks so much again for pointing this out!
-john

Powered by blists - more mailing lists