Re: [PATCH v2 3/6] locking/local_lock: Introduce local_lock_lockdep_start/end()

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

On 2025-07-08 18:53:00 [-0700], Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@kernel.org>
> 
> Introduce local_lock_lockdep_start/end() pair to teach lockdep
> about a region of execution where per-cpu local_lock is not taken
> and lockdep should consider such local_lock() as "trylock" to
> avoid multiple false-positives:
> - lockdep doesn't like when the same lock is taken in normal and
>   in NMI context
> - lockdep cannot recognize that local_locks that protect kmalloc
>   buckets are different local_locks and not taken together
> 
> This pair of lockdep aid is used by slab in the following way:
> 
> if (local_lock_is_locked(&s->cpu_slab->lock))
> 	goto out;
> local_lock_lockdep_start(&s->cpu_slab->lock);
> p = ___slab_alloc(s, gfpflags, node, addr, c, orig_size);
> local_lock_lockdep_end(&s->cpu_slab->lock);
> 
> Where ___slab_alloc() is calling
> local_lock_irqsave(&s->cpu_slab->lock, ...) many times,
> and all of them will not deadlock since this lock is not taken.

So you prefer this instead of using a trylock variant in ___slab_alloc()
which would simply return in case the trylock fails?
Having the local_lock_is_locked() is still good to avoid the lock
failure if it can be detected early. I am just not sure if the extra
lockdep override is really needed.

…
> --- a/include/linux/local_lock.h
> +++ b/include/linux/local_lock.h
> @@ -81,6 +81,21 @@
>  #define local_trylock_irqsave(lock, flags)			\
>  	__local_trylock_irqsave(lock, flags)
>  
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +#define local_lock_lockdep_start(lock)					\
> +	do {								\
> +		lockdep_assert(!__local_lock_is_locked(lock));		\
> +		this_cpu_ptr(lock)->dep_map.flags = LOCAL_LOCK_UNLOCKED;\
> +	} while (0)
> +
> +#define local_lock_lockdep_end(lock)					\
> +	do { this_cpu_ptr(lock)->dep_map.flags = 0; } while (0)
> +
> +#else
> +#define local_lock_lockdep_start(lock) /**/
> +#define local_lock_lockdep_end(lock) /**/

Why the /**/?

…
> index 9f361d3ab9d9..6c580081ace3 100644
> --- a/include/linux/lockdep_types.h
> +++ b/include/linux/lockdep_types.h
> @@ -190,13 +190,15 @@ struct lockdep_map {
>  	u8				wait_type_outer; /* can be taken in this context */
>  	u8				wait_type_inner; /* presents this context */
>  	u8				lock_type;
> -	/* u8				hole; */
> +	u8				flags;
>  #ifdef CONFIG_LOCK_STAT
>  	int				cpu;
>  	unsigned long			ip;
>  #endif
>  };
>  
> +#define LOCAL_LOCK_UNLOCKED		1

Maybe DEPMAP_FLAG_LL_UNLOCKED so it is kind of obvious where it belongs
to. Maybe use "u8 local_lock_unlocked:1;" instead the flags + define. It
is even used for held_lock below so it is not a new concept with
lockdep. It would narrow down the usage.

>  struct pin_cookie { unsigned int val; };
>  

Sebastian