linux-kernel - Re: [RESEND][PATCH v21 2/6] sched/locking: Add blocked_on_state to provide necessary tri-state for proxy return-migration

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: <337322ea-6efe-4814-a813-e55d4c80fda7@amd.com>
Date: Mon, 15 Sep 2025 13:35:57 +0530
From: K Prateek Nayak <kprateek.nayak@....com>
To: John Stultz <jstultz@...gle.com>, LKML <linux-kernel@...r.kernel.org>
CC: 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

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

>  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 :)

[..snip..]

> @@ -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?

> +deactivate_donor:
> +	return proxy_deactivate(rq, donor);
>  }
>  #else /* SCHED_PROXY_EXEC */
>  static struct task_struct *
-- 
Thanks and Regards,
Prateek