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: <a6c0b576-393d-4283-86ab-5c88c22ebdc3@amd.com>
Date: Fri, 12 Sep 2025 11:20:22 +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 1/6] locking: Add task::blocked_lock to
 serialize blocked_on state

Hello John,

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?

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

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>

I'm still lagging behind on testing (Sorry about that!) but I should
have the results for this by Monday.

-- 
Thanks and Regards,
Prateek

> -			set_task_blocked_on(current, lock);
>  			trace_contention_begin(lock, LCB_F_MUTEX);
>  		}
> -
> -		raw_spin_lock_irqsave(&lock->wait_lock, flags);
>  	}
> -	raw_spin_lock_irqsave(&lock->wait_lock, flags);
> -acquired:
>  	__clear_task_blocked_on(current, lock);
>  	__set_current_state(TASK_RUNNING);
> +	raw_spin_unlock(&current->blocked_lock);
>  
>  	if (ww_ctx) {
>  		/*