Re: [PATCH bpf-next v2 3/6] locking/local_lock: Introduce local_trylock_irqsave()

[Vlastimil Babka <vbabka@suse.cz>]

On 12/11/24 11:53, Vlastimil Babka wrote:
> On 12/10/24 03:39, Alexei Starovoitov wrote:
>> From: Alexei Starovoitov <ast@kernel.org>
>> 
>> Similar to local_lock_irqsave() introduce local_trylock_irqsave().
>> It uses spin_trylock in PREEMPT_RT and always succeeds when !RT.
> 
> Hmm but is that correct to always succeed? If we're in an nmi, we might be
> preempting an existing local_(try)lock_irqsave() critical section because
> disabling irqs doesn't stop NMI's, right?

So unless I'm missing something, it would need to be a new kind of local
lock to support this trylock operation on !RT? Perhaps based on the same
principle of a simple active/locked flag that I tried in my sheaves RFC? [1]
There could be also the advantage that if all (potentially) irq contexts
(not just nmi) used trylock, it would be sufficient to disable preeemption
and not interrupts, which is cheaper.
The RT variant could work as you proposed here, that was wrong in my RFC as
you already pointed out when we discussed v1 of this series.

[1]
https://lore.kernel.org/all/20241112-slub-percpu-caches-v1-5-ddc0bdc27e05@suse.cz/

>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>> ---
>>  include/linux/local_lock.h          |  9 +++++++++
>>  include/linux/local_lock_internal.h | 23 +++++++++++++++++++++++
>>  2 files changed, 32 insertions(+)
>> 
>> diff --git a/include/linux/local_lock.h b/include/linux/local_lock.h
>> index 091dc0b6bdfb..6880c29b8b98 100644
>> --- a/include/linux/local_lock.h
>> +++ b/include/linux/local_lock.h
>> @@ -30,6 +30,15 @@
>>  #define local_lock_irqsave(lock, flags)				\
>>  	__local_lock_irqsave(lock, flags)
>>  
>> +/**
>> + * local_trylock_irqsave - Try to acquire a per CPU local lock, save and disable
>> + *			   interrupts. Always succeeds in !PREEMPT_RT.
>> + * @lock:	The lock variable
>> + * @flags:	Storage for interrupt flags
>> + */
>> +#define local_trylock_irqsave(lock, flags)			\
>> +	__local_trylock_irqsave(lock, flags)
>> +
>>  /**
>>   * local_unlock - Release a per CPU local lock
>>   * @lock:	The lock variable
>> diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
>> index 8dd71fbbb6d2..2c0f8a49c2d0 100644
>> --- a/include/linux/local_lock_internal.h
>> +++ b/include/linux/local_lock_internal.h
>> @@ -31,6 +31,13 @@ static inline void local_lock_acquire(local_lock_t *l)
>>  	l->owner = current;
>>  }
>>  
>> +static inline void local_trylock_acquire(local_lock_t *l)
>> +{
>> +	lock_map_acquire_try(&l->dep_map);
>> +	DEBUG_LOCKS_WARN_ON(l->owner);
>> +	l->owner = current;
>> +}
>> +
>>  static inline void local_lock_release(local_lock_t *l)
>>  {
>>  	DEBUG_LOCKS_WARN_ON(l->owner != current);
>> @@ -45,6 +52,7 @@ static inline void local_lock_debug_init(local_lock_t *l)
>>  #else /* CONFIG_DEBUG_LOCK_ALLOC */
>>  # define LOCAL_LOCK_DEBUG_INIT(lockname)
>>  static inline void local_lock_acquire(local_lock_t *l) { }
>> +static inline void local_trylock_acquire(local_lock_t *l) { }
>>  static inline void local_lock_release(local_lock_t *l) { }
>>  static inline void local_lock_debug_init(local_lock_t *l) { }
>>  #endif /* !CONFIG_DEBUG_LOCK_ALLOC */
>> @@ -91,6 +99,13 @@ do {								\
>>  		local_lock_acquire(this_cpu_ptr(lock));		\
>>  	} while (0)
>>  
>> +#define __local_trylock_irqsave(lock, flags)			\
>> +	({							\
>> +		local_irq_save(flags);				\
>> +		local_trylock_acquire(this_cpu_ptr(lock));	\
>> +		1;						\
>> +	})
>> +
>>  #define __local_unlock(lock)					\
>>  	do {							\
>>  		local_lock_release(this_cpu_ptr(lock));		\
>> @@ -148,6 +163,14 @@ typedef spinlock_t local_lock_t;
>>  		__local_lock(lock);				\
>>  	} while (0)
>>  
>> +#define __local_trylock_irqsave(lock, flags)			\
>> +	({							\
>> +		typecheck(unsigned long, flags);		\
>> +		flags = 0;					\
>> +		migrate_disable();				\
>> +		spin_trylock(this_cpu_ptr((__lock)));		\
>> +	})
>> +
>>  #define __local_unlock(__lock)					\
>>  	do {							\
>>  		spin_unlock(this_cpu_ptr((__lock)));		\
>