Re: [PATCH 4/6] locking/local_lock: Introduce local_lock_irqsave_check()

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-04-30 20:27:16 [-0700], Alexei Starovoitov wrote:
> --- a/include/linux/local_lock_internal.h
> +++ b/include/linux/local_lock_internal.h
> @@ -168,6 +168,15 @@ do {								\
>  /* preemption or migration must be disabled before calling __local_lock_is_locked */
>  #define __local_lock_is_locked(lock) READ_ONCE(this_cpu_ptr(lock)->acquired)
>  
> +#define __local_lock_irqsave_check(lock, flags)					\
> +	do {									\
> +		if (IS_ENABLED(CONFIG_DEBUG_LOCK_ALLOC) &&			\
> +		    (!__local_lock_is_locked(lock) || in_nmi()))		\
> +			WARN_ON_ONCE(!__local_trylock_irqsave(lock, flags));	\
> +		else								\
> +			__local_lock_irqsave(lock, flags);			\
> +	} while (0)
> +

Hmm. If I see this right in SLUB then this is called from preemptible
context. Therefore the this_cpu_ptr() from __local_lock_is_locked()
should trigger a warning here.

This check variant provides only additional debugging and otherwise
behaves as local_lock_irqsave(). Therefore the in_nmi() should return
immediately with a WARN_ON() regardless if the lock is available or not
because the non-try variant should never be invoked from an NMI. 

This looks like additional debug infrastructure that should be part of
local_lock_irqsave() itself, maybe hidden behind a debug switch which is
forced now from one of the current user. I don't think we need an extra
interface for this. It might be restricted to the trylock_test variant
if it makes sense but I wouldn't introduce an extra function for that.

>  #define __local_lock_release(lock)					\
>  	do {								\
>  		local_trylock_t *tl;					\

Sebastian