On 2025-07-08 18:53:00 [-0700], Alexei Starovoitov wrote:
> From: Alexei Starovoitov <ast@xxxxxxxxxx>
>
> 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
