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

[Vlastimil Babka <vbabka@suse.cz>]

On 1/17/25 9:33 PM, Sebastian Andrzej Siewior wrote:
> On 2025-01-14 18:17:42 [-0800], Alexei Starovoitov wrote:
>> diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
>> index 8dd71fbbb6d2..93672127c73d 100644
>> --- a/include/linux/local_lock_internal.h
>> +++ b/include/linux/local_lock_internal.h
>> @@ -75,37 +85,73 @@ do {								\
>>  
>>  #define __local_lock(lock)					\
>>  	do {							\
>> +		local_lock_t *l;				\
>>  		preempt_disable();				\
>> -		local_lock_acquire(this_cpu_ptr(lock));		\
>> +		l = this_cpu_ptr(lock);				\
>> +		lockdep_assert(l->active == 0);			\
>> +		WRITE_ONCE(l->active, 1);			\
>> +		local_lock_acquire(l);				\
>>  	} while (0)
> 
> …
> 
>> +#define __local_trylock_irqsave(lock, flags)			\
>> +	({							\
>> +		local_lock_t *l;				\
>> +		local_irq_save(flags);				\
>> +		l = this_cpu_ptr(lock);				\
>> +		if (READ_ONCE(l->active) == 1) {		\
>> +			local_irq_restore(flags);		\
>> +			l = NULL;				\
>> +		} else {					\
>> +			WRITE_ONCE(l->active, 1);		\
>> +			local_trylock_acquire(l);		\
>> +		}						\
>> +		!!l;						\
>> +	})
>> +
> 
> Part of the selling for local_lock_t was that it does not affect
> !PREEMPT_RT builds. By adding `active' you extend every data structure
> and you have an extra write on every local_lock(). It was meant to
> replace preempt_disable()/ local_irq_save() based locking with something
> that actually does locking on PREEMPT_RT without risking my life once
> people with pitchforks come talk about the new locking :)
> 
> I admire that you try to make PREEMPT_RT and !PREEMPT_RT similar in a
> way that both detect recursive locking which not meant to be supported. 
> 
> Realistically speaking as of today we don't have any recursive lock
> detection other than lockdep. So it should be enough given that the bots
> use it often and hopefully local testing.
> Your assert in local_lock() does not work without lockdep. It will only
> make local_trylock_irqsave() detect recursion and lead to two splats
> with lockdep enabled in local_lock() (one from the assert and the second
> from lockdep).
> 
> I would say you could get rid of the `active' field and solely rely on
> lockdep and the owner field. So __local_trylock_irqsave() could maybe
> use local_trylock_acquire() to conditionally acquire the lock if `owner'
> is NULL.
> 
> This makes it possible to have recursive code without lockdep, but with
> lockdep enabled the testcase should fail if it relies on recursion.
> Other than that, I don't see any advantage. Would that work?

I don't think it would work, or am I missing something? The goal is to
allow the operation (alloc, free) to opportunistically succeed in e.g.
nmi context, but only if we didn't interrupt anything that holds the
lock. Otherwise we must allow for failure - hence trylock.
(a possible extension that I mentioned is to also stop doing irqsave to
avoid its overhead and thus also operations from an irq context would be
oportunistic)
But if we detect the "trylock must fail" cases only using lockdep, we'll
deadlock without lockdep. So e.g. the "active" flag has to be there?

So yes this goes beyond the original purpose of local_lock. Do you think
it should be a different lock type then, which would mean the other
users of current local_lock that don't want the opportunistic nesting
via trylock, would not inflict the "active" flag overhead?

AFAICS the RT implementation of local_lock could then be shared for both
of these types, but I might be missing some nuance there.

> Sebastian