On 2024-11-23 12:38, Linus Torvalds wrote:
On Sat, 23 Nov 2024 at 07:31, Mathieu Desnoyers
<mathieu.desnoyers@xxxxxxxxxxxx> 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
