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

[Sebastian Andrzej Siewior <bigeasy@linutronix.de>]

On 2025-01-21 16:59:40 [+0100], Vlastimil Babka wrote:
> 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?

You are right. I noticed that myself but didn't get to reply…

> 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.

I was thinking about this over the weekend and this implementation
extends the data structure by 4 bytes and has this mandatory read/ write
on every lock/ unlock operation. This is what makes it a bit different
than the original.

If the local_lock_t is replaced with spinlock_t then the data structure
is still extended by four bytes (assuming no lockdep) and we have a
mandatory read/ write operation. The whole thing still does not work on
PREEMPT_RT but it isn't much different from what we have now. This is
kind of my favorite. This could be further optimized to avoid the atomic
operation given it is always local per-CPU memory. Maybe a
local_spinlock_t :)

Sebastian