| Message ID | 20241123153031.2884933-5-mathieu.desnoyers@efficios.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | tracing: Remove conditional locking from tracepoints | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
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
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
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 :/
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
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
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.
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
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.
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
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
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(-)