linux-kernel - Re: [RESEND][PATCH v9 1/7] locking/mutex: Remove wakeups from under mutex::wait

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: <xhsmhbk6ia52c.mognet@vschneid-thinkpadt14sgen2i.remote.csb>
Date: Tue, 09 Apr 2024 18:12:11 +0200
From: Valentin Schneider <vschneid@...hat.com>
To: John Stultz <jstultz@...gle.com>, LKML <linux-kernel@...r.kernel.org>
Cc: Peter Zijlstra <peterz@...radead.org>, Joel Fernandes
 <joelaf@...gle.com>, Qais Yousef <qyousef@...gle.com>, Ingo Molnar
 <mingo@...hat.com>, Juri Lelli <juri.lelli@...hat.com>, Vincent Guittot
 <vincent.guittot@...aro.org>, Dietmar Eggemann <dietmar.eggemann@....com>,
 Steven Rostedt <rostedt@...dmis.org>, Ben Segall <bsegall@...gle.com>,
 Zimuzo Ezeozue <zezeozue@...gle.com>, Youssef Esmat
 <youssefesmat@...gle.com>, Mel Gorman <mgorman@...e.de>, Daniel Bristot de
 Oliveira <bristot@...hat.com>, 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>, K Prateek Nayak <kprateek.nayak@....com>,
 Thomas Gleixner <tglx@...utronix.de>, kernel-team@...roid.com, Davidlohr
 Bueso <dave@...olabs.net>, John Stultz <jstultz@...gle.com>
Subject: Re: [RESEND][PATCH v9 1/7] locking/mutex: Remove wakeups from under
 mutex::wait_lock

On 01/04/24 16:44, John Stultz wrote:
> From: Peter Zijlstra <peterz@...radead.org>
>
> In preparation to nest mutex::wait_lock under rq::lock we need to remove
> wakeups from under it.
>
> Cc: Joel Fernandes <joelaf@...gle.com>
> Cc: Qais Yousef <qyousef@...gle.com>
> Cc: Ingo Molnar <mingo@...hat.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Juri Lelli <juri.lelli@...hat.com>
> Cc: Vincent Guittot <vincent.guittot@...aro.org>
> Cc: Dietmar Eggemann <dietmar.eggemann@....com>
> Cc: Valentin Schneider <vschneid@...hat.com>
> Cc: Steven Rostedt <rostedt@...dmis.org>
> Cc: Ben Segall <bsegall@...gle.com>
> Cc: Zimuzo Ezeozue <zezeozue@...gle.com>
> Cc: Youssef Esmat <youssefesmat@...gle.com>
> Cc: Mel Gorman <mgorman@...e.de>
> Cc: Daniel Bristot de Oliveira <bristot@...hat.com>
> Cc: Will Deacon <will@...nel.org>
> Cc: Waiman Long <longman@...hat.com>
> Cc: Boqun Feng <boqun.feng@...il.com>
> Cc: "Paul E. McKenney" <paulmck@...nel.org>
> Cc: Metin Kaya <Metin.Kaya@....com>
> Cc: Xuewen Yan <xuewen.yan94@...il.com>
> Cc: K Prateek Nayak <kprateek.nayak@....com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: kernel-team@...roid.com
> Tested-by: K Prateek Nayak <kprateek.nayak@....com>
> Acked-by: Davidlohr Bueso <dave@...olabs.net>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
> [Heavily changed after 55f036ca7e74 ("locking: WW mutex cleanup") and
> 08295b3b5bee ("locking: Implement an algorithm choice for Wound-Wait
> mutexes")]
> Signed-off-by: Juri Lelli <juri.lelli@...hat.com>
> [jstultz: rebased to mainline, added extra wake_up_q & init
>  to avoid hangs, similar to Connor's rework of this patch]
> Signed-off-by: John Stultz <jstultz@...gle.com>

This looks mostly good to me, some preemption questions below.

> @@ -934,6 +942,7 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
>  		}
>  	}
>  
> +	preempt_disable();
>  	raw_spin_lock(&lock->wait_lock);
>  	debug_mutex_unlock(lock);
>  	if (!list_empty(&lock->wait_list)) {
> @@ -952,8 +961,8 @@ static noinline void __sched __mutex_unlock_slowpath(struct mutex *lock, unsigne
>  		__mutex_handoff(lock, next);
>

(minor nit) Could the preempt_disable() be moved here instead? IMO if it's
closer to the unlock it makes it clearer why it is there
(e.g. sched/core.c::affine_move_task(), rt_mutex_setprio(), __sched_setscheduler().

>  	raw_spin_unlock(&lock->wait_lock);
> -
>  	wake_up_q(&wake_q);
> +	preempt_enable();
>  }
>  

> @@ -1775,8 +1782,9 @@ static int __sched rt_mutex_slowlock(struct rt_mutex_base *lock,
>  	 * irqsave/restore variants.
>  	 */
>  	raw_spin_lock_irqsave(&lock->wait_lock, flags);
> -	ret = __rt_mutex_slowlock_locked(lock, ww_ctx, state);
> +	ret = __rt_mutex_slowlock_locked(lock, ww_ctx, state, &wake_q);
>  	raw_spin_unlock_irqrestore(&lock->wait_lock, flags);
> +	wake_up_q(&wake_q);

Shouldn't this also be wrapped in a preempt-disabled region?

>  	rt_mutex_post_schedule();
>  
>  	return ret;

> @@ -122,6 +123,7 @@ static int __sched __rwbase_read_lock(struct rwbase_rt *rwb,
>  	if (!ret)
>  		atomic_inc(&rwb->readers);
>  	raw_spin_unlock_irq(&rtm->wait_lock);
> +	wake_up_q(&wake_q);

Same question wrt preemption.

>  	if (!ret)
>  		rwbase_rtmutex_unlock(rtm);
>