Skip to contentSkip to conversation

tracepoint: add lockdep rcu_is_watching() check to trace_call__##name()

PATCH Superseded David Carlier 2 participants 5 messages Apr 30 – May 20 Tracing

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 tracepoint: add lockdep rcu_is_watching() check to trace_call__##name()

trace_call__##name() was added by commit 677a3d82b640 ("tracepoint: Add trace_call__##name() API") to let callers that already gate on trace_##name##_enabled() skip the redundant static_branch_unlikely() re-evaluation. The changelog states that the new helper retains the LOCKDEP RCU-watching assertion carried by trace_##name(), but the WARN_ONCE() was omitted from both definitions.

Without it, sites converted from trace_foo(args) to trace_call__foo(args) lose the WARN_ONCE(!rcu_is_watching()) coverage under CONFIG_LOCKDEP when the tracepoint is enabled - the case that commit c2679254b9c9 ("tracing:
Make tracepoint lockdep check actually test something") added the warning for.

Mirror the same block in both trace_call__##name() bodies, gated by (cond) in __DECLARE_TRACE to match its trace_##name() and ungated in __DECLARE_TRACE_SYSCALL.

Fixes 677a3d82b640 ("tracepoint: Add trace_call__##name() API")
Cc Vineeth Pillai (Google) <[email protected]>
Cc Mathieu Desnoyers <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 include/linux/tracepoint.h | 8 ++++++++
 1 file changed, 8 insertions(+)
include/linux/tracepoint.h +8 -0
index 578e520b6ee6..048e0035d4fa 100644
@@ -318,6 +318,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	static inline void trace_call__##name(proto)			\
 	{								\
 		__do_trace_##name(args);				\
+		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
+			WARN_ONCE(!rcu_is_watching(),			\
+				  "RCU not watching for tracepoint");	\
+		}							\
 	}
 
 #define __DECLARE_TRACE_SYSCALL(name, proto, args, data_proto)		\
@@ -342,6 +346,10 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	{								\
 		might_fault();						\
 		__do_trace_##name(args);				\
+		if (IS_ENABLED(CONFIG_LOCKDEP)) {			\
+			WARN_ONCE(!rcu_is_watching(),			\
+				  "RCU not watching for tracepoint");	\
+		}							\
 	}
 
 /*
Signature · 1 line
2.53.0
maintainer
#1 lore raw
Quoted 9 lines · On Thu, 30 Apr 2026 06:35:10 +0100 David Carlier <[email protected]> wrote:

Without it, sites converted from trace_foo(args) to trace_call__foo(args) lose the WARN_ONCE(!rcu_is_watching()) coverage under CONFIG_LOCKDEP when the tracepoint is enabled - the case that commit c2679254b9c9 ("tracing:
Make tracepoint lockdep check actually test something") added the warning for.

Mirror the same block in both trace_call__##name() bodies, gated by (cond) in __DECLARE_TRACE to match its trace_##name() and ungated in __DECLARE_TRACE_SYSCALL.

If it gets called without rcu watching, the rcu dereference in __DO_TRACE_CALL() will trigger.

No need to add the warning here. The reason the trace_foo() had it, was to trigger when the tracepoint WASN'T enabled.

Now, perhaps where the warning should go, is in the trace_foo_enabled() code.

-- Steve

#2 lore raw

PATCH v2 1/1 tracepoint: add lockdep rcu_is_watching() check to trace_##name##_enabled()

In reply to David Carlier

trace_call__##name() was added by commit 677a3d82b640 ("tracepoint: Add trace_call__##name() API") for callers that already gate on trace_##name##_enabled(), letting them skip the redundant static_branch_unlikely() re-evaluation. The LOCKDEP rcu_is_watching() WARN_ONCE() that trace_##name() carries on the disabled path was not mirrored, so the cold-path coverage added by commit c2679254b9c9 ("tracing: Make tracepoint lockdep check actually test something") is lost for trace_call__##name() callers when the tracepoint is disabled.

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. The natural place is the gate that trace_call__##name() callers consult: trace_##name##_enabled().

Fixes 677a3d82b640 ("tracepoint: Add trace_call__##name() API")
Cc Vineeth Pillai (Google) <[email protected]>
Cc Mathieu Desnoyers <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 include/linux/tracepoint.h | 4 ++++
 1 file changed, 4 insertions(+)
include/linux/tracepoint.h +4 -0
index 578e520b6ee6..3b3ab19c2627 100644
@@ -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);\
 	}
 
Signature · 1 line
2.53.0
maintainer
#3 lore raw

Re: [PATCH v2] tracepoint: add lockdep rcu_is_watching() check to trace_##name##_enabled()

Quoted 9 lines · On Thu, 30 Apr 2026 15:41:59 +0100 David Carlier <[email protected]> wrote:

trace_call__##name() was added by commit 677a3d82b640 ("tracepoint: Add trace_call__##name() API") for callers that already gate on trace_##name##_enabled(), letting them skip the redundant static_branch_unlikely() re-evaluation. The LOCKDEP rcu_is_watching() WARN_ONCE() that trace_##name() carries on the disabled path was not mirrored, so the cold-path coverage added by commit c2679254b9c9 ("tracing: Make tracepoint lockdep check actually test something") is lost for trace_call__##name() callers when the tracepoint is disabled.

Quoted 6 lines

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. The natural place is the gate that trace_call__##name() callers consult: trace_##name##_enabled().

Fixes 677a3d82b640 ("tracepoint: Add trace_call__##name() API")

Thanks, but I'm 86'ing the Fixes tag. The above commit has absolutely nothing to do with this patch. This issue was there from the beginning of having the trace_*_enabled() macro. This isn't a fix.

I'll modify the change log to be more accurate as well.

-- Steve

Quoted 24 lines
Cc Vineeth Pillai (Google) <[email protected]>
Cc Mathieu Desnoyers <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Peter Zijlstra <[email protected]>
---
 include/linux/tracepoint.h | 4 ++++
 1 file changed, 4 insertions(+)
include/linux/tracepoint.h +4 -0
index 578e520b6ee6..3b3ab19c2627 100644
@@ -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);\
 	}
 
maintainer
#4 lore raw

Re: [PATCH v2] tracepoint: add lockdep rcu_is_watching() check to trace_##name##_enabled()

In reply to David Carlier

[ Replying to Sashiko ]

Quoted 27 lines

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.

Quoted 3 lines

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.

include/linux/tracepoint.h static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p) · line 301

Quoted 13 lines
include/linux/tracepoint.h +4 -0
@@ -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);\
 	}
Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 3 lines

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