On 7/11/25 09:50, Sebastian Andrzej Siewior wrote:
> 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?
The code isn't always in a position to "simply return". On !RT I think we
can at least assume that if we succeeded once, it means we're not a irq/nmi
interrupting a locked context so we'll succeed the following attempts too.
On RT IIUC the lock might be taken by someone else, so a trylock might fail
(even if it should also mean we're in a context that can do a non-try lock).
> 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
