Re: [RESEND][PATCH v9 1/7] locking/mutex: Remove wakeups from under mutex::wait_lock

[Valentin Schneider]

On 01/04/24 16:44, John Stultz wrote:
> From: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>
> In preparation to nest mutex::wait_lock under rq::lock we need to remove
> wakeups from under it.
>
> Cc: Joel Fernandes <joelaf@xxxxxxxxxx>
> Cc: Qais Yousef <qyousef@xxxxxxxxxx>
> Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> Cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> Cc: Juri Lelli <juri.lelli@xxxxxxxxxx>
> Cc: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
> Cc: Dietmar Eggemann <dietmar.eggemann@xxxxxxx>
> Cc: Valentin Schneider <vschneid@xxxxxxxxxx>
> Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> Cc: Ben Segall <bsegall@xxxxxxxxxx>
> Cc: Zimuzo Ezeozue <zezeozue@xxxxxxxxxx>
> Cc: Youssef Esmat <youssefesmat@xxxxxxxxxx>
> Cc: Mel Gorman <mgorman@xxxxxxx>
> Cc: Daniel Bristot de Oliveira <bristot@xxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Waiman Long <longman@xxxxxxxxxx>
> Cc: Boqun Feng <boqun.feng@xxxxxxxxx>
> Cc: "Paul E. McKenney" <paulmck@xxxxxxxxxx>
> Cc: Metin Kaya <Metin.Kaya@xxxxxxx>
> Cc: Xuewen Yan <xuewen.yan94@xxxxxxxxx>
> Cc: K Prateek Nayak <kprateek.nayak@xxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: kernel-team@xxxxxxxxxxx
> Tested-by: K Prateek Nayak <kprateek.nayak@xxxxxxx>
> Acked-by: Davidlohr Bueso <dave@xxxxxxxxxxxx>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> [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@xxxxxxxxxx>
> [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@xxxxxxxxxx>

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