Skip to contentSkip to conversation

tracing: Add a no-rcu-check version of trace_##event##_enabled()

PATCH In review Steven Rostedt 2 participants 2 messages Jul 1 – Jul 2 Tracing

lore
Share & tools
lore

Steven Rostedt

author maintainer
#0 lore raw

PATCH 1/1 tracing: Add a no-rcu-check version of trace_##event##_enabled()

From: Steven Rostedt <[email protected]>

Tracepoints require that RCU is watching. To prevent them from being used in places that RCU is not watching, the trace_##event() macro always calls rcu_is_watching() even when the event is not enabled and warns if RCU is not watching. This is to make sure a warning is triggered even if the tracepoint is never enabled (as it is only a bug when it is).

It was noticed that tracepoints could be hidden within trace_#event#_enabled() calls, which are used to do extra work for the tracepoint only if the tracepoint is enabled. But this also can hide the fact that a tracepoint is placed in a location that can be called when RCU is not watching.

Commit 9764e731ef6ab ("tracepoint: Add lockdep rcu_is_watching() check to trace_##name##_enabled()") added a check to the trace_##event##_enabled() macro to make sure RCU is watching when it is called to make sure not to hide the bug of a tracepoint being called when RCU is not watching.

There is one case in the irq_disable tracepoint where it is within a trace_irq_disable_enabled() block, but it checks if RCU is watching, and if it isn't, it makes a call to ct_irq_enter() that makes RCU watch again. But because trace_irq_disable_enabled() now checks if RCU is watching and will trigger if it isn't. This is a false warning as the code within the block handles this case.

Add a new internal macro __trace_##event##_enabled() that doesn't check if RCU is watching, and convert the irq_enable/disable tracepoints over to it.

Fixes 9764e731ef6ab ("tracepoint: Add lockdep rcu_is_watching() check to trace_##name##_enabled()")
---
 include/linux/tracepoint.h      | 12 +++++++++++-
 kernel/trace/trace_preemptirq.c |  2 +-
 2 files changed, 12 insertions(+), 2 deletions(-)
include/linux/tracepoint.h +11 -1
index 4a0c36f40fe2..e0d838c9ce93 100644
@@ -292,13 +292,18 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	{								\
 	}								\
 	static inline bool						\
+	__trace_##name##_enabled(void)					\
+	{								\
+		return static_branch_unlikely(&__tracepoint_##name.key);\
+	}								\
+	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);\
+		return __trace_##name##_enabled();			\
 	}
 
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto)			\
@@ -457,6 +462,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	{								\
 	}								\
 	static inline bool						\
+	__trace_##name##_enabled(void)					\
+	{								\
+		return false;						\
+	}								\
+	static inline bool						\
 	trace_##name##_enabled(void)					\
 	{								\
 		return false;						\
kernel/trace/trace_preemptirq.c +1 -1
index 0c42b15c3800..b63e3558948f 100644
@@ -30,7 +30,7 @@
 #else
 #define trace(point, args)					\
 	do {							\
-		if (trace_##point##_enabled()) {		\
+		if (__trace_##point##_enabled()) {		\
 			bool exit_rcu = false;			\
 			if (in_nmi())				\
 				break;				\
Signature · 1 line
2.53.0
Tested-by on v1 1/1
#1 lore raw
Quoted 26 lines · Steven Rostedt

From: Steven Rostedt <[email protected]>

Tracepoints require that RCU is watching. To prevent them from being used in places that RCU is not watching, the trace_##event() macro always calls rcu_is_watching() even when the event is not enabled and warns if RCU is not watching. This is to make sure a warning is triggered even if the tracepoint is never enabled (as it is only a bug when it is).

It was noticed that tracepoints could be hidden within trace_#event#_enabled() calls, which are used to do extra work for the tracepoint only if the tracepoint is enabled. But this also can hide the fact that a tracepoint is placed in a location that can be called when RCU is not watching.

Commit 9764e731ef6ab ("tracepoint: Add lockdep rcu_is_watching() check to trace_##name##_enabled()") added a check to the trace_##event##_enabled() macro to make sure RCU is watching when it is called to make sure not to hide the bug of a tracepoint being called when RCU is not watching.

There is one case in the irq_disable tracepoint where it is within a trace_irq_disable_enabled() block, but it checks if RCU is watching, and if it isn't, it makes a call to ct_irq_enter() that makes RCU watch again. But because trace_irq_disable_enabled() now checks if RCU is watching and will trigger if it isn't. This is a false warning as the code within the block handles this case.

Quoted 8 lines

Add a new internal macro __trace_##event##_enabled() that doesn't check if RCU is watching, and convert the irq_enable/disable tracepoints over to it.

Fixes 9764e731ef6ab ("tracepoint: Add lockdep rcu_is_watching() check to trace_##name##_enabled()")

Thanks, warning gone on all affected platforms.

Gr{oetje,eeting}s,

Geert

Signature · 5 lines
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds