On Wed, 05 Aug 2026, quanyeyang wrote: > On Wed, Aug 5, 2026 at 6:49 AM NeilBrown <[email protected]> wrote: > > I don't think that detecting recursive acquisition is a scalable > > solution. You'll keep finding new locks that you need to enhance. > > Yes, it makes sense. > > > > > If I step back a bit, it looks like TP-BPF (BPF attached to tracepoints) > > introduces a new execution context, similar to SOFTIRQ and HARDIRQ. > > i.e. it is something that can start running at almost any point and > > blocks the currently running code until it completes. > > I suspect it would be good to handle it in a similar way. > > Thanks for the detailed explanation. > > > > > So I suggest adding a new > > LOCKDEP_STATE(TP_BPF) > > to kernel/locking/lockdep_states.h, and teaching lockdep to understand > > it. Then it could help avoid all these problems. > > > > An important part of this would be the ability to temporarily disable > > TP_BPF much as we can disable interrupts. What happens at present if > > there is tracepoint that happens in BPF code, and a BPF handler is attached > > to that. Does it get called recursively? > > Would there be a problem with disabling new TP_BPF handlers while TP_BPF > > code is running? Maybe this is already done? > > It's already partly done, but it differs by attach type. > > The perf-event tracepoint path -- trace_call_bpf(), and the perf-overflow > (PMI) path bpf_overflow_handler() -- share a global per-CPU counter, > bpf_prog_active: while a BPF program is running on a CPU, any further > BPF on that CPU is skipped (the prog_array is re-entered only to bump > the miss counter). So a tracepoint firing inside BPF code does not > recurse there; the handler is dropped. That is already the "disable > TP_BPF while TP_BPF runs" behaviour, analogous to disabling interrupts. > > The raw_tracepoint path is different: __bpf_trace_run() uses > bpf_prog_get_recursion_context(), which is per-program -- only the *same* > program is prevented from recursing; *different* programs can nest. So a > raw_tp handler can run nested inside another raw_tp handler on the same > CPU. > > So the recursion/disabling mechanism exists, but it isn't uniform: the > perf path is already "disable-like", raw_tp is not. That asymmetry is > itself part of what makes the lockdep picture messy. > > > > > Circling back the original patch to rhashtables that was proposed - had > > you said that you needed to use rhashtables in a different context > > similar to HARDIRQ or SOFTIRQ, then I think I would have had a different > > response. That is a well-specified problem with well-understood > > solutions. > > > > I would likely be OK with making is possible for lockdep to see locks > > from different tables as different locks, because they need to be taken > > in different lockdep contexts. > > > > I would then see if the new lock_class_key could be declared in the > > rhashtable_params struct. This is already passed around everywhere, so > > this would be much less intrusive. We would have to use some cast to > > get rid of the 'const' attribute when passing the lock_class_key to > > lockdep, but I think that is justifiable. > > On the per-table lock_class_key in rhashtable_params: that sounds right > to me. I'll rework the rhashtable patch so each table can declare its own > key in params -- framed as "different tables live in different lockdep > contexts" (not per-init-site, as in my first attempt) -- and resend. > > Two small notes: > - if the field is a `struct lock_class_key *` pointer, the params struct > can stay const: lockdep writes to the pointed-to key, not the struct, > so no cast-away-const is needed;
I hoped that the keys would be embedded in the params. That would work nicely if all users simply used a global params structure. But a few copy the structures around (I cannot imaging why, but maybe there is a good reason). Copying lockdep keys doesn't work. (And some use extern references.... I wonder home much performance they lose because the hash/cmp functions cannot be inlined....) One thing I didn't like about the original patch is that it seemed to add a lot of noise to the code, passing keys around in multiple places. Maybe the cleanest approach would be to embed the keys in the "struct rhashtable" so each table gets its own key. Then use lockdep_register_key() in rhashtable_init and use it as needed. You could even have just one key and use the different subclasses for the different locks. 0 for rhashtable.mutex 1 for rhashtable.lock 2 for the bit locks but maybe that is needless complexity. All of the rest of your email is interesting and useful - thanks. NeilBrown > - callers that don't set it would fall back to the shared class (current > behaviour), so it's opt-in and doesn't churn existing users. > > On LOCKDEP_STATE(TP_BPF): it reads as the more general fix, but it's a > larger change in the locking core and would need the recursion gate made > uniform across attach types first (raw_tp currently runs despite the > global gate). I'd lean toward starting with the per-table-key change for > the reported false positive and treating the TP_BPF-context modelling as > the longer-term direction -- happy to take guidance from you and the > locking folks on scope. > > > > > NeilBrown > Note: Sending from an alternate address because Gmail is > temporarily rejecting my mailing-list replies. > Thanks, > quanyeyang >
