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