[ Replying to Sashiko ]
> commit 7596bdcd9abf9a26b3964f10b5d5de3249e731e9 > Author: David Carlier <[email protected]> > tracepoint: add lockdep rcu_is_watching() check to trace_##name##_enabled() > This commit attempts to restore RCU lockdep coverage for trace_call__##name() > callers by adding a warning directly to the trace_##name##_enabled() static > branch check. > > When the tracepoint is enabled, the rcu_dereference inside > > __DO_TRACE_CALL() already trips under PROVE_RCU, so the warning is > > only needed on the !enabled path. > Is this description accurate regarding __DO_TRACE_CALL()? > Looking at the implementation, it uses rcu_dereference_raw() which > intentionally bypasses lockdep checking. This means trace_call__##name() > lacks lockdep coverage on both the enabled and disabled paths. > > diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h > > --- a/include/linux/tracepoint.h > > +++ b/include/linux/tracepoint.h > > @@ -293,6 +293,10 @@ static inline struct tracepoint > > *tracepoint_ptr_deref(tracepoint_ptr_t *p) > > static inline bool \ > > trace_##name##_enabled(void) \ > > { \ > > + if (IS_ENABLED(CONFIG_LOCKDEP)) { \ > > + WARN_ONCE(!rcu_is_watching(), \ > > + "RCU not watching for tracepoint"); \ > > + } \ > > return static_branch_unlikely(&__tracepoint_##name.key);\ > > } > Does this unconditional check cause spurious lockdep splats? > Unlike the original trace_##name() macro, trace_##name##_enabled() takes no > arguments and cannot evaluate the tracepoint's condition, such as > cpu_online(raw_smp_processor_id()). If a subsystem queries > trace_##name##_enabled() on an offline CPU where RCU is not watching, won't > this trigger a false positive warning, even if the tracepoint itself would > have been safely skipped by its condition? If the CPU is offline, you shouldn't be using trace_##name##_enabled() to begin with. > Furthermore, is it already safe to query a static branch outside of an RCU > watch window? Subsystems sometimes use this boolean query in RCU-idle > contexts simply to skip data gathering without ever intending to execute > the tracepoint. The entire point of this patch is to make sure lockdep *always* checks that "RCU is watching" at every trace_##name##_enabled() location regardless if the tracepoint is active or not. It's not about the static branch, but the tracepoint that is hidden behind it. > Would it be more reliable to add the WARN_ONCE check, guarded by its cond > parameter, directly to trace_call__##name() to appropriately mirror how > trace_##name() behaves? Hmm, if this does become an issue, that may be worth doing. But currently, I do not believe the enabled() code is used by anything with conditional tracepoints. -- Steve
