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

[Vlastimil Babka <vbabka@suse.cz>]

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@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?

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