[RFC PATCH 0/5] tracing: Remove conditional locking from tracepoints

[RFC PATCH 0/5] tracing: Remove conditional locking from tracepoints

[Mathieu Desnoyers]

Linus pointed out that he was not pleased by the implementation of
faultable syscall tracepoints because it contains conditional locking.
This patch series addresses those concerns.

This is based on commit 06afb0f36106 ("Merge tag 'trace-v6.13' of
git://git.kernel.org/pub/scm/linux/kernel/git/trace/linux-trace")

Many thanks to Linus for the analysis and feedback.

Link: https://lore.kernel.org/lkml/CAHk-=witPrLcu22dZ93VCyRQonS7+-dFYhQbna=KBa-TAhayMw@mail.gmail.com/
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michael Jeanson <mjeanson@efficios.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Jordan Rife <jrife@google.com>
Cc: linux-trace-kernel@vger.kernel.org

Mathieu Desnoyers (5):
  tracing: Move it_func[0] comment to the relevant context
  tracing: Remove __idx variable from __DO_TRACE
  rcupdate_trace: Define rcu_tasks_trace lock guard
  tracing: Remove conditional locking from __DO_TRACE()
  tracing: Remove cond argument from __DECLARE_TRACE_SYSCALL

 include/linux/rcupdate_trace.h |  5 +++
 include/linux/tracepoint.h     | 70 ++++++++++++----------------------
 2 files changed, 29 insertions(+), 46 deletions(-)

-- 
2.39.5

[RFC PATCH 1/5] tracing: Move it_func[0] comment to the relevant context

[Mathieu Desnoyers]

When introducing __DO_TRACE_CALL(), the iteration over it_func moved
from __DO_TRACE() to __tracepoint_iter_##_name(), but the comment
relevant for this iterator was left in its original location.

Move the comment to the relevant context.

Fixes: d25e37d89dd2 ("tracepoint: Optimize using static_call()")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michael Jeanson <mjeanson@efficios.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Jordan Rife <jrife@google.com>
Cc: linux-trace-kernel@vger.kernel.org
---
 include/linux/tracepoint.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 425123e921ac..d390e8cabf02 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -210,9 +210,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 #endif /* CONFIG_HAVE_STATIC_CALL */
 
 /*
- * it_func[0] is never NULL because there is at least one element in the array
- * when the array itself is non NULL.
- *
  * With @syscall=0, the tracepoint callback array dereference is
  * protected by disabling preemption.
  * With @syscall=1, the tracepoint callback array dereference is
@@ -316,6 +313,9 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
  * We have no guarantee that gcc and the linker won't up-align the tracepoint
  * structures, so we create an array of pointers that will be used for iteration
  * on the tracepoints.
+ *
+ * it_func[0] is never NULL because there is at least one element in the array
+ * when the array itself is non NULL.
  */
 #define __DEFINE_TRACE_EXT(_name, _ext, proto, args)			\
 	static const char __tpstrtab_##_name[]				\
-- 
2.39.5

[RFC PATCH 2/5] tracing: Remove __idx variable from __DO_TRACE

[Mathieu Desnoyers]

Since the removal of SRCU-protected tracepoints, the __idx variable in
__DO_TRACE is unused. Remove this variable.

Fixes: 48bcda684823 ("tracing: Remove definition of trace_*_rcuidle()")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michael Jeanson <mjeanson@efficios.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Jordan Rife <jrife@google.com>
Cc: linux-trace-kernel@vger.kernel.org
---
 include/linux/tracepoint.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index d390e8cabf02..867f3c1ac7dc 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -218,8 +218,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
  */
 #define __DO_TRACE(name, args, cond, syscall)				\
 	do {								\
-		int __maybe_unused __idx = 0;				\
-									\
 		if (!(cond))						\
 			return;						\
 									\
-- 
2.39.5

[RFC PATCH 3/5] rcupdate_trace: Define rcu_tasks_trace lock guard

[Mathieu Desnoyers]

Define a rcu_tasks_trace lock guard for use by the syscall enter/exit
tracepoints.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michael Jeanson <mjeanson@efficios.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Jordan Rife <jrife@google.com>
Cc: linux-trace-kernel@vger.kernel.org
---
 include/linux/rcupdate_trace.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/rcupdate_trace.h b/include/linux/rcupdate_trace.h
index eda493200663..e6c44eb428ab 100644
--- a/include/linux/rcupdate_trace.h
+++ b/include/linux/rcupdate_trace.h
@@ -10,6 +10,7 @@
 
 #include <linux/sched.h>
 #include <linux/rcupdate.h>
+#include <linux/cleanup.h>
 
 extern struct lockdep_map rcu_trace_lock_map;
 
@@ -98,4 +99,8 @@ static inline void rcu_read_lock_trace(void) { BUG(); }
 static inline void rcu_read_unlock_trace(void) { BUG(); }
 #endif /* #ifdef CONFIG_TASKS_TRACE_RCU */
 
+DEFINE_LOCK_GUARD_0(rcu_tasks_trace,
+	rcu_read_lock_trace(),
+	rcu_read_unlock_trace())
+
 #endif /* __LINUX_RCUPDATE_TRACE_H */
-- 
2.39.5

[RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Mathieu Desnoyers]

Remove conditional locking by moving the __DO_TRACE() code into
trace_##name().

When the faultable syscall tracepoints were implemented, __DO_TRACE()
had a rcuidle argument which selected between SRCU and preempt disable.
Therefore, the RCU tasks trace protection for faultable syscall
tracepoints was introduced using the same pattern.

At that point, it did not appear obvious that this feedback from Linus [1]
applied here as well, because the __DO_TRACE() modification was
extending a pre-existing pattern.

Shortly before pulling the faultable syscall tracepoints modifications,
Steven removed the rcuidle argument and SRCU protection scheme entirely
from tracepoint.h:

commit 48bcda684823 ("tracing: Remove definition of trace_*_rcuidle()")

This required a rebase of the faultable syscall tracepoints series,
which missed a perfect opportunity to integrate the prior recommendation
from Linus.

In response to the pull request, Linus pointed out [2] that he was not
pleased by the implementation, expecting this to be fixed in a follow up
patch series.

Move __DO_TRACE() code into trace_##name() within each of
__DECLARE_TRACE() and __DECLARE_TRACE_SYSCALL(). Use a scoped guard
to guard the preempt disable notrace and RCU tasks trace critical
sections.

Link: https://lore.kernel.org/all/CAHk-=wggDLDeTKbhb5hh--x=-DQd69v41137M72m6NOTmbD-cw@mail.gmail.com/ [1]
Link: https://lore.kernel.org/lkml/CAHk-=witPrLcu22dZ93VCyRQonS7+-dFYhQbna=KBa-TAhayMw@mail.gmail.com/ [2]
Fixes: a363d27cdbc2 ("tracing: Allow system call tracepoints to handle page faults")
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michael Jeanson <mjeanson@efficios.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Jordan Rife <jrife@google.com>
Cc: linux-trace-kernel@vger.kernel.org
---
 include/linux/tracepoint.h | 45 ++++++++++----------------------------
 1 file changed, 12 insertions(+), 33 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 867f3c1ac7dc..832f49b56b1f 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -209,31 +209,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 #define __DO_TRACE_CALL(name, args)	__traceiter_##name(NULL, args)
 #endif /* CONFIG_HAVE_STATIC_CALL */
 
-/*
- * With @syscall=0, the tracepoint callback array dereference is
- * protected by disabling preemption.
- * With @syscall=1, the tracepoint callback array dereference is
- * protected by Tasks Trace RCU, which allows probes to handle page
- * faults.
- */
-#define __DO_TRACE(name, args, cond, syscall)				\
-	do {								\
-		if (!(cond))						\
-			return;						\
-									\
-		if (syscall)						\
-			rcu_read_lock_trace();				\
-		else							\
-			preempt_disable_notrace();			\
-									\
-		__DO_TRACE_CALL(name, TP_ARGS(args));			\
-									\
-		if (syscall)						\
-			rcu_read_unlock_trace();			\
-		else							\
-			preempt_enable_notrace();			\
-	} while (0)
-
 /*
  * Make sure the alignment of the structure in the __tracepoints section will
  * not add unwanted padding between the beginning of the section and the
@@ -282,10 +257,12 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), cond, PARAMS(data_proto)) \
 	static inline void trace_##name(proto)				\
 	{								\
-		if (static_branch_unlikely(&__tracepoint_##name.key))	\
-			__DO_TRACE(name,				\
-				TP_ARGS(args),				\
-				TP_CONDITION(cond), 0);			\
+		if (static_branch_unlikely(&__tracepoint_##name.key)) { \
+			if (cond) {					\
+				scoped_guard(preempt_notrace)		\
+					__DO_TRACE_CALL(name, TP_ARGS(args)); \
+			}						\
+		}							\
 		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
 			WARN_ONCE(!rcu_is_watching(),			\
 				  "RCU not watching for tracepoint");	\
@@ -297,10 +274,12 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	static inline void trace_##name(proto)				\
 	{								\
 		might_fault();						\
-		if (static_branch_unlikely(&__tracepoint_##name.key))	\
-			__DO_TRACE(name,				\
-				TP_ARGS(args),				\
-				TP_CONDITION(cond), 1);			\
+		if (static_branch_unlikely(&__tracepoint_##name.key)) {	\
+			if (cond) {					\
+				scoped_guard(rcu_tasks_trace)		\
+					__DO_TRACE_CALL(name, TP_ARGS(args)); \
+			}						\
+		}							\
 		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
 			WARN_ONCE(!rcu_is_watching(),			\
 				  "RCU not watching for tracepoint");	\
-- 
2.39.5

[RFC PATCH 5/5] tracing: Remove cond argument from __DECLARE_TRACE_SYSCALL

[Mathieu Desnoyers]

Syscall tracepoints do not require a "cond" argument, because they are
meant to be used only for sys_enter and sys_exit instrumentation, which
don't require condition evaluation.

Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michael Jeanson <mjeanson@efficios.com>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Alexei Starovoitov <ast@kernel.org>
Cc: Yonghong Song <yhs@fb.com>
Cc: Paul E. McKenney <paulmck@kernel.org>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: bpf@vger.kernel.org
Cc: Joel Fernandes <joel@joelfernandes.org>
Cc: Jordan Rife <jrife@google.com>
Cc: linux-trace-kernel@vger.kernel.org
---
 include/linux/tracepoint.h | 25 +++++++++++++------------
 1 file changed, 13 insertions(+), 12 deletions(-)

diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 832f49b56b1f..b2633a72e871 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -220,7 +220,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
  * site if it is not watching, as it will need to be active when the
  * tracepoint is enabled.
  */
-#define __DECLARE_TRACE_COMMON(name, proto, args, cond, data_proto)	\
+#define __DECLARE_TRACE_COMMON(name, proto, args, data_proto)		\
 	extern int __traceiter_##name(data_proto);			\
 	DECLARE_STATIC_CALL(tp_func_##name, __traceiter_##name);	\
 	extern struct tracepoint __tracepoint_##name;			\
@@ -254,7 +254,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 	}
 
 #define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
-	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), cond, PARAMS(data_proto)) \
+	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto)) \
 	static inline void trace_##name(proto)				\
 	{								\
 		if (static_branch_unlikely(&__tracepoint_##name.key)) { \
@@ -269,18 +269,16 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 		}							\
 	}
 
-#define __DECLARE_TRACE_SYSCALL(name, proto, args, cond, data_proto)	\
-	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), cond, PARAMS(data_proto)) \
+#define __DECLARE_TRACE_SYSCALL(name, proto, args, data_proto)		\
+	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto)) \
 	static inline void trace_##name(proto)				\
 	{								\
 		might_fault();						\
 		if (static_branch_unlikely(&__tracepoint_##name.key)) {	\
-			if (cond) {					\
-				scoped_guard(rcu_tasks_trace)		\
-					__DO_TRACE_CALL(name, TP_ARGS(args)); \
-			}						\
+			scoped_guard(rcu_tasks_trace)			\
+				__DO_TRACE_CALL(name, TP_ARGS(args));	\
 		}							\
-		if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {		\
+		if (IS_ENABLED(CONFIG_LOCKDEP)) {			\
 			WARN_ONCE(!rcu_is_watching(),			\
 				  "RCU not watching for tracepoint");	\
 		}							\
@@ -363,7 +361,7 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 
 
 #else /* !TRACEPOINTS_ENABLED */
-#define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
+#define __DECLARE_TRACE_COMMON(name, proto, args, data_proto)		\
 	static inline void trace_##name(proto)				\
 	{ }								\
 	static inline int						\
@@ -387,7 +385,11 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 		return false;						\
 	}
 
-#define __DECLARE_TRACE_SYSCALL	__DECLARE_TRACE
+#define __DECLARE_TRACE(name, proto, args, cond, data_proto)		\
+	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto))
+
+#define __DECLARE_TRACE_SYSCALL(name, proto, args, data_proto)		\
+	__DECLARE_TRACE_COMMON(name, PARAMS(proto), PARAMS(args), PARAMS(data_proto))
 
 #define DEFINE_TRACE_FN(name, reg, unreg, proto, args)
 #define DEFINE_TRACE_SYSCALL(name, reg, unreg, proto, args)
@@ -453,7 +455,6 @@ static inline struct tracepoint *tracepoint_ptr_deref(tracepoint_ptr_t *p)
 
 #define DECLARE_TRACE_SYSCALL(name, proto, args)			\
 	__DECLARE_TRACE_SYSCALL(name, PARAMS(proto), PARAMS(args),	\
-				cpu_online(raw_smp_processor_id()),	\
 				PARAMS(void *__data, proto))
 
 #define TRACE_EVENT_FLAGS(event, flag)
-- 
2.39.5

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Linus Torvalds]

On Sat, 23 Nov 2024 at 07:31, Mathieu Desnoyers
<mathieu.desnoyers@efficios.com> wrote:
>
>  include/linux/tracepoint.h | 45 ++++++++++----------------------------
>  1 file changed, 12 insertions(+), 33 deletions(-)

Thanks. This looks much more straightforward, and obviously is smaller too.

Side note: I realize I was the one suggesting "scoped_guard()", but
looking at the patch I do think that just unnecessarily added another
level of indentation. Since you already wrote the

    if (cond) {
        ..
    }

part as a block statement, there's no upside to the guard having its
own scoped block, so instead of

    if (cond) { \
        scoped_guard(preempt_notrace)           \
            __DO_TRACE_CALL(name, TP_ARGS(args)); \
    }

this might be simpler as just a plain "guard()" and one less indentation:

    if (cond) { \
        guard(preempt_notrace);           \
        __DO_TRACE_CALL(name, TP_ARGS(args)); \
    }

but by now this is just an unimportant detail.

I think I suggested scoped_guard() mainly because that would then just
make the "{ }" in the if-statement superfluous, but that's such a
random reason that it *really* doesn't matter.

             Linus

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Mathieu Desnoyers]

On 2024-11-23 12:38, Linus Torvalds wrote:
> On Sat, 23 Nov 2024 at 07:31, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>>   include/linux/tracepoint.h | 45 ++++++++++----------------------------
>>   1 file changed, 12 insertions(+), 33 deletions(-)
> 
> Thanks. This looks much more straightforward, and obviously is smaller too.
> 
> Side note: I realize I was the one suggesting "scoped_guard()", but
> looking at the patch I do think that just unnecessarily added another
> level of indentation. Since you already wrote the
> 
>      if (cond) {
>          ..
>      }
> 
> part as a block statement, there's no upside to the guard having its
> own scoped block, so instead of
> 
>      if (cond) { \
>          scoped_guard(preempt_notrace)           \
>              __DO_TRACE_CALL(name, TP_ARGS(args)); \
>      }
> 
> this might be simpler as just a plain "guard()" and one less indentation:
> 
>      if (cond) { \
>          guard(preempt_notrace);           \
>          __DO_TRACE_CALL(name, TP_ARGS(args)); \
>      }
> 
> but by now this is just an unimportant detail.
> 
> I think I suggested scoped_guard() mainly because that would then just
> make the "{ }" in the if-statement superfluous, but that's such a
> random reason that it *really* doesn't matter.

Thanks for the follow up. I agree that guard() would remove one level
of nesting and would be an improvement.

Steven, do you want me to update the series with this change or
should I leave the scoped_guard() as is considering the ongoing
testing in linux-next ? We can always keep this minor change
(scoped_guard -> guard) for a follow up patch.

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Steven Rostedt]

On Sun, 24 Nov 2024 20:50:12 -0500
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> Steven, do you want me to update the series with this change or
> should I leave the scoped_guard() as is considering the ongoing
> testing in linux-next ? We can always keep this minor change
> (scoped_guard -> guard) for a follow up patch.

Yeah, just send a patch on top. I haven't pushed to linux-next yet
though, because I found that it conflicts with my rust pull request
and I'm testing the merge of the two at the moment.

-- Steve

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Mathieu Desnoyers]

On 2024-11-23 12:38, Linus Torvalds wrote:
> On Sat, 23 Nov 2024 at 07:31, Mathieu Desnoyers
> <mathieu.desnoyers@efficios.com> wrote:
>>
>>   include/linux/tracepoint.h | 45 ++++++++++----------------------------
>>   1 file changed, 12 insertions(+), 33 deletions(-)
> 
> Thanks. This looks much more straightforward, and obviously is smaller too.
> 
> Side note: I realize I was the one suggesting "scoped_guard()", but
> looking at the patch I do think that just unnecessarily added another
> level of indentation. Since you already wrote the
> 
>      if (cond) {
>          ..
>      }
> 
> part as a block statement, there's no upside to the guard having its
> own scoped block, so instead of
> 
>      if (cond) { \
>          scoped_guard(preempt_notrace)           \
>              __DO_TRACE_CALL(name, TP_ARGS(args)); \
>      }
> 
> this might be simpler as just a plain "guard()" and one less indentation:
> 
>      if (cond) { \
>          guard(preempt_notrace);           \
>          __DO_TRACE_CALL(name, TP_ARGS(args)); \
>      }
> 
> but by now this is just an unimportant detail.
> 
> I think I suggested scoped_guard() mainly because that would then just
> make the "{ }" in the if-statement superfluous, but that's such a
> random reason that it *really* doesn't matter.

I tried the following alteration to the code, which triggers an
unexpected compiler warning on master, but not on v6.12. I suspect
this is something worth discussing:

         static inline void trace_##name(proto)                          \
         {                                                               \
                 if (static_branch_unlikely(&__tracepoint_##name.key)) { \
                         if (cond)                                       \
                                 scoped_guard(preempt_notrace)           \
                                         __DO_TRACE_CALL(name, TP_ARGS(args)); \
                 }                                                       \
                 if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {             \
                         WARN_ONCE(!rcu_is_watching(),                   \
                                   "RCU not watching for tracepoint");   \
                 }                                                       \
         }

It triggers this warning with gcc version 12.2.0 (Debian 12.2.0-14):

In file included from ./include/trace/syscall.h:5,
                  from ./include/linux/syscalls.h:94,
                  from init/main.c:21:
./include/trace/events/tlb.h: In function ‘trace_tlb_flush’:
./include/linux/tracepoint.h:261:28: warning: suggest explicit braces to avoid ambiguous ‘else’ [-Wdangling-else]
   261 |                         if (cond)                                       \
       |                            ^
./include/linux/tracepoint.h:446:9: note: in expansion of macro ‘__DECLARE_TRACE’
   446 |         __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args),              \
       |         ^~~~~~~~~~~~~~~
./include/linux/tracepoint.h:584:9: note: in expansion of macro ‘DECLARE_TRACE’
   584 |         DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))
       |         ^~~~~~~~~~~~~
./include/trace/events/tlb.h:38:1: note: in expansion of macro ‘TRACE_EVENT’
    38 | TRACE_EVENT(tlb_flush,
       | ^~~~~~~~~~~

I suspect this is caused by the "else" at the end of the __scoped_guard() macro:

#define __scoped_guard(_name, _label, args...)                          \
         for (CLASS(_name, scope)(args);                                 \
              __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);       \
              ({ goto _label; }))                                        \
                 if (0) {                                                \
_label:                                                                 \
                         break;                                          \
                 } else

#define scoped_guard(_name, args...)    \
         __scoped_guard(_name, __UNIQUE_ID(label), args)

AFAIU this is a new warning introduced by

commit fcc22ac5baf ("cleanup: Adjust scoped_guard() macros to avoid potential warning")

Thanks,

Mathieu


-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Peter Zijlstra]

On Mon, Nov 25, 2024 at 09:18:18AM -0500, Mathieu Desnoyers wrote:
> On 2024-11-23 12:38, Linus Torvalds wrote:

> I tried the following alteration to the code, which triggers an
> unexpected compiler warning on master, but not on v6.12. I suspect
> this is something worth discussing:
> 
>         static inline void trace_##name(proto)                          \
>         {                                                               \
>                 if (static_branch_unlikely(&__tracepoint_##name.key)) { \
>                         if (cond)                                       \
>                                 scoped_guard(preempt_notrace)           \
>                                         __DO_TRACE_CALL(name, TP_ARGS(args)); \

So coding style would like braces here for it being multi-line. As
opposed to C that only mandates it for multi-statement. And then the
problem doesn't occur.

>                 }                                                       \
>                 if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {             \
>                         WARN_ONCE(!rcu_is_watching(),                   \
>                                   "RCU not watching for tracepoint");   \
>                 }                                                       \
>         }
> 

> I suspect this is caused by the "else" at the end of the __scoped_guard() macro:
> 
> #define __scoped_guard(_name, _label, args...)                          \
>         for (CLASS(_name, scope)(args);                                 \
>              __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);       \
>              ({ goto _label; }))                                        \
>                 if (0) {                                                \
> _label:                                                                 \
>                         break;                                          \
>                 } else
> 
> #define scoped_guard(_name, args...)    \
>         __scoped_guard(_name, __UNIQUE_ID(label), args)
> 
> AFAIU this is a new warning introduced by
> 
> commit fcc22ac5baf ("cleanup: Adjust scoped_guard() macros to avoid potential warning")

Yeah,.. So strictly speaking the code is fine, but the various compilers
don't like it when that else dangles :/

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Przemek Kitszel]

On 11/25/24 15:26, Peter Zijlstra wrote:
> On Mon, Nov 25, 2024 at 09:18:18AM -0500, Mathieu Desnoyers wrote:
>> On 2024-11-23 12:38, Linus Torvalds wrote:
> 
>> I tried the following alteration to the code, which triggers an
>> unexpected compiler warning on master, but not on v6.12. I suspect
>> this is something worth discussing:
>>
>>          static inline void trace_##name(proto)                          \
>>          {                                                               \
>>                  if (static_branch_unlikely(&__tracepoint_##name.key)) { \
>>                          if (cond)                                       \
>>                                  scoped_guard(preempt_notrace)           \
>>                                          __DO_TRACE_CALL(name, TP_ARGS(args)); \
> 
> So coding style would like braces here for it being multi-line. As
> opposed to C that only mandates it for multi-statement. And then the
> problem doesn't occur.
> 
>>                  }                                                       \
>>                  if (IS_ENABLED(CONFIG_LOCKDEP) && (cond)) {             \
>>                          WARN_ONCE(!rcu_is_watching(),                   \
>>                                    "RCU not watching for tracepoint");   \
>>                  }                                                       \
>>          }
>>
> 
>> I suspect this is caused by the "else" at the end of the __scoped_guard() macro:
>>
>> #define __scoped_guard(_name, _label, args...)                          \
>>          for (CLASS(_name, scope)(args);                                 \
>>               __guard_ptr(_name)(&scope) || !__is_cond_ptr(_name);       \
>>               ({ goto _label; }))                                        \
>>                  if (0) {                                                \
>> _label:                                                                 \
>>                          break;                                          \
>>                  } else
>>
>> #define scoped_guard(_name, args...)    \
>>          __scoped_guard(_name, __UNIQUE_ID(label), args)
>>
>> AFAIU this is a new warning introduced by
>>
>> commit fcc22ac5baf ("cleanup: Adjust scoped_guard() macros to avoid potential warning")
> 
> Yeah,.. So strictly speaking the code is fine, but the various compilers
> don't like it when that else dangles :/

At one point I had a version that did:
	if (0)
label: ;
	else
		for (....)

but it is goto-jumping back in the code
https://lore.kernel.org/netdev/20241001145718.8962-1-przemyslaw.kitszel@intel.com/#t

I could switch to it again to reduce noise like this problem, but such
change would be to essentially allow bad formatting

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Linus Torvalds]

On Mon, 25 Nov 2024 at 07:35, Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> At one point I had a version that did:
>         if (0)
> label: ;
>         else
>                 for (....)

Well, that is impressively ugly.

> but it is goto-jumping back in the code

I'm not sure why you think *that* is a problem. It does look like it
avoids the dangling else issue, which seems to be the more immediate
problem.

(Of course, "immediate" is all very relative - the use-case that
triggered this is going away anyway and being replaced by a regular
'guard()').

That said, I have a "lovely" suggestion. Instead of the "if(0)+goto"
games, I think you can just do this:

  #define scoped_guard(_name, args...)                                   \
         for (CLASS(_name, scope)(args), *_once = (void *)1; _once &&    \
              (__guard_ptr(_name)(&scope) || !__is_cond_ptr(_name));     \
              _once = NULL)

which avoids the whole UNIQUE_NAME on the label too.

Yeah, yeah, if somebody has nested uses of scoped_guard(), they will
have shadowing of the "_once" variable and extrawarn enables -Wshadow.

But dammit, that isn't actually an error, and I think -Wshadow is bad
for these situations. Nested temporary variables in macros shouldn't
warn. Oh well.

Is there a way to shut up -Wshadow on a per-variable basis? My
google-fu is too weak.

Did I test the above macro? Don't be silly. All my code works on first
try. Except when it doesn't.

          Linus

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Peter Zijlstra]

On Mon, Nov 25, 2024 at 09:51:43AM -0800, Linus Torvalds wrote:

> That said, I have a "lovely" suggestion. Instead of the "if(0)+goto"
> games, I think you can just do this:
> 
>   #define scoped_guard(_name, args...)                                   \
>          for (CLASS(_name, scope)(args), *_once = (void *)1; _once &&    \
>               (__guard_ptr(_name)(&scope) || !__is_cond_ptr(_name));     \
>               _once = NULL)
> 

Right, so that's more or less what we used to have:

#define scoped_guard(_name, args...)                                    \
        for (CLASS(_name, scope)(args), *done = NULL;			\
	     __guard_ptr(_name)(&scope) && !done; done = (void *)1)

But it turns out the compilers have a hard time dealing with this. From
commit fcc22ac5baf0 ("cleanup: Adjust scoped_guard() macros to avoid
potential warning"):	

    int foo(struct my_drv *adapter)
    {
            scoped_guard(spinlock, &adapter->some_spinlock)
                    return adapter->spinlock_protected_var;
    }

Using that (old) form results in:

    error: control reaches end of non-void function [-Werror=return-type]


Now obviously the above can also be written like:

    int foo(struct my_drv *adapter)
    {
            guard(spinlock)(&adapter->some_spinlock);
	    return adapter->spinlock_protected_var;
    }

But the point is to show the compilers get confused. Additionally Dan
Carpenter noted that smatch has a much easier time dealing with the new
form.

And the commit notes the generated code is actually slighly smaller with
e new form too (probably because the compiler is less confused about
control flow).

Except of course, now we get that dangling-else warning, there is no
winning this :-/

So I merged that patch because of the compilers getting less confused
and better code-gen, but if you prefer the old one we can definitely go
back. In which case we should go talk to compiler folks to figure out
how to make it not so confused.

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Linus Torvalds]

On Tue, 26 Nov 2024 at 00:46, Peter Zijlstra <peterz@infradead.org> wrote:
>
> Using that (old) form results in:
>
>     error: control reaches end of non-void function [-Werror=return-type]

Ahh. Annoying, but yeah.

> Except of course, now we get that dangling-else warning, there is no
> winning this :-/

Well, was there any actual problem with the "jump backwards" version?
Przemek implied some problem, but ..

> So I merged that patch because of the compilers getting less confused
> and better code-gen, but if you prefer the old one we can definitely go
> back.

Oh, I'm not in any way trying to force that "_once" variable kind of
pattern. I didn't realize it had had that other compiler confusion
issue.

         Linus

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Dmitry Torokhov]

On Tue, Nov 26, 2024 at 10:13:43AM -0800, Linus Torvalds wrote:
> On Tue, 26 Nov 2024 at 00:46, Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > Using that (old) form results in:
> >
> >     error: control reaches end of non-void function [-Werror=return-type]
> 
> Ahh. Annoying, but yeah.
> 
> > Except of course, now we get that dangling-else warning, there is no
> > winning this :-/
> 
> Well, was there any actual problem with the "jump backwards" version?
> Przemek implied some problem, but ..

No, it was based on my feedback with "jump backwards" looking confusing
to me.  But if it gets rid of a warning then we should use it instead.

Thanks.

-- 
Dmitry

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Przemek Kitszel]

On 11/26/24 21:47, Dmitry Torokhov wrote:
> On Tue, Nov 26, 2024 at 10:13:43AM -0800, Linus Torvalds wrote:
>> On Tue, 26 Nov 2024 at 00:46, Peter Zijlstra <peterz@infradead.org> wrote:
>>>
>>> Using that (old) form results in:
>>>
>>>      error: control reaches end of non-void function [-Werror=return-type]
>>
>> Ahh. Annoying, but yeah.
>>
>>> Except of course, now we get that dangling-else warning, there is no
>>> winning this :-/
>>
>> Well, was there any actual problem with the "jump backwards" version?
>> Przemek implied some problem, but ..
> 
> No, it was based on my feedback with "jump backwards" looking confusing
> to me.  But if it gets rid of a warning then we should use it instead.
> 
> Thanks.
> 

yeah, no problem per-se, just "a better" choice given properly formatted
code :|, will turn it the other way, to have less surprises for those
not stressing about such aesthetic all the time ;)

--
BTW shadowing of the goto-label is not a -Wshadow but a -Wfckoff level
of complain; I have spend a few days to think of something better,
including abusing of switch and more ugly things, -ENOIDEA

Re: [RFC PATCH 4/5] tracing: Remove conditional locking from __DO_TRACE()

[Linus Torvalds]

On Tue, 26 Nov 2024 at 14:32, Przemek Kitszel
<przemyslaw.kitszel@intel.com> wrote:
>
> BTW shadowing of the goto-label is not a -Wshadow but a -Wfckoff level
> of complain; I have spend a few days to think of something better,
> including abusing of switch and more ugly things, -ENOIDEA

Yeah, I think you need to do that __UNIQUE_ID() thing for labels,
because while you can introduce local labels (see "__label__" use in
the wait macros and a few other places, where we do it) you need to
have a block scope to do that and the goto needs to be inside that
scope, of course.

Which I don't see how to do in this situation.

        Linus