| Message ID | 20241123153031.2884933-5-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | Queued |
| Headers | show |
| Series | tracing: Remove conditional locking from tracepoints | expand |
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
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
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
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"); \
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(-)