Re: [PATCH] locking/semaphore: Use raw_spin_trylock_irqsave() in down_trylock()

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Jan 20, 2025 at 02:36:08PM -0500, Waiman Long wrote:
> A circular lock dependency splat has been seen with down_trylock().
> 
> [ 4011.795602] ======================================================
> [ 4011.795603] WARNING: possible circular locking dependency detected
> [ 4011.795607] 6.12.0-41.el10.s390x+debug
> [ 4011.795612] ------------------------------------------------------
> [ 4011.795613] dd/32479 is trying to acquire lock:
> [ 4011.795617] 0015a20accd0d4f8 ((console_sem).lock){-.-.}-{2:2}, at: down_trylock+0x26/0x90
> [ 4011.795636]
> [ 4011.795636] but task is already holding lock:
> [ 4011.795637] 000000017e461698 (&zone->lock){-.-.}-{2:2}, at: rmqueue_bulk+0xac/0x8f0
> [ 4011.795644]
> [ 4011.795644] which lock already depends on the new lock.
>   :
> [ 4011.796025]   (console_sem).lock --> hrtimer_bases.lock --> &zone->lock
> [ 4011.796025]
> [ 4011.796029]  Possible unsafe locking scenario:
> [ 4011.796029]
> [ 4011.796030]        CPU0
> [ 4011.796031]        ----
> [ 4011.796032]   lock(&zone->lock);
> [ 4011.796034]                                lock(hrtimer_bases.lock);
> [ 4011.796036]                                lock(&zone->lock);
> [ 4011.796038]   lock((console_sem).lock);
> [ 4011.796039]
> [ 4011.796039]  *** DEADLOCK ***

Urgh, I hate this ^ bit of the lockdep output, it pretends to be
something useful, while it is the least useful part. Doubly so for
anything with more than 2 locks involved.

> The calling sequence was
>   rmqueue_pcplist()
>   => __rmqueue_pcplist()
>     => rmqueue_bulk()
>       => expand()
> 	=> __add_to_free_list()
> 	  => __warn_printk()
> 	    => ...
> 	      => console_trylock()
> 		=> __down_trylock_console_sem()
> 		  => down_trylock()
> 		    => _raw_spin_lock_irqsave()
> 
> Normally, a trylock call should avoid this kind of circular lock
> dependency splat, but down_trylock() is an exception. Fix this problem
> by using raw_spin_trylock_irqsave() in down_trylock() to make it behave
> like the other trylock calls.
> 
> Signed-off-by: Waiman Long <longman@redhat.com>
> ---
>  kernel/locking/semaphore.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/locking/semaphore.c b/kernel/locking/semaphore.c
> index 34bfae72f295..cb27cbf5162f 100644
> --- a/kernel/locking/semaphore.c
> +++ b/kernel/locking/semaphore.c
> @@ -127,6 +127,7 @@ EXPORT_SYMBOL(down_killable);
>   *
>   * NOTE: This return value is inverted from both spin_trylock and
>   * mutex_trylock!  Be careful about this when converting code.
> + * I.e. 0 on success, 1 on failure.
>   *
>   * Unlike mutex_trylock, this function can be used from interrupt context,
>   * and the semaphore can be released by any task or interrupt.
> @@ -136,7 +137,8 @@ int __sched down_trylock(struct semaphore *sem)
>  	unsigned long flags;
>  	int count;
>  
> -	raw_spin_lock_irqsave(&sem->lock, flags);
> +	if (!raw_spin_trylock_irqsave(&sem->lock, flags))
> +		return 1;
>  	count = sem->count - 1;
>  	if (likely(count >= 0))
>  		sem->count = count;

Urgh, this is terrible *again*. Didn't you try and do something
similarly daft with the rt_mutex_trylock ? And you didn't learn from
that?

So please, start by actually explaining things.