tracing/mm: Add tracepoint_enabled() helper function for headers
Subsystems (4)
Steven Rostedt
maintainercover tracing/mm: Add tracepoint_enabled() helper function for headers
Tracepoints are not safe to be called directly from header files as they may be included by C code that has CREATE_TRACE_POINTS defined, and this would cause side effects and possibly break the build in hard to debug ways.
Instead, it is recommended to call a tracepoint helper function that is defined in a C file that calls the tracepoint. But we would only want this function to be called if the tracepoint is enabled, as function calls add overhead.
The trace_<tracepoint>_enabled() function is also no safe to be called in a header file as it is created by the tracepoint header, which suffers the same fate if CREATE_TRACE_POINTS is defined. Instead, the tracepoint needs to be declared as an extern, and the helper function can test the static key to call the helper function that calls the tracepoint.
This has been done by open coding the tracepoint extern and calling the static key directly:
commit 95813b8faa0cd ("mm/page_ref: add tracepoint to track down page
reference manipulation")
does this (back in 2016). Now we have another use case, so a helper function should be created to keep the internals of the tracepoints from being spread out in other subsystems.
This adds tracepoint_enabled() helper macro and DECLARE_TRACEPOINT() macro to allow this to be done without exposing the internals of the tracepoints.
The first patch adds the infrastructure, the second converts page_ref over to it. I also noticed that the msr tracepoint needs to be converted as well.
Steven Rostedt (VMware) (2):
tracepoints: Add helper to test if tracepoint is enabled in a header mm/page_ref: Convert the open coded tracepoint enabled to the new helper
----
Documentation/trace/tracepoints.rst | 25 ++++++++++++++++++++++ include/linux/page_ref.h | 42 ++++++++++++++++++------------------- include/linux/tracepoint-defs.h | 33 +++++++++++++++++++++++++++++ 3 files changed, 79 insertions(+), 21 deletions(-)
Steven Rostedt
maintainerPATCH 1/2 tracepoints: Add helper to test if tracepoint is enabled in a header
From: "Steven Rostedt (VMware)" <[email protected]>
As tracepoints are discouraged from being added in a header because it can cause side effects if other tracepoints are in headers, the common workaround is to add a function call that calls a wrapper function in a C file that then calls the tracepoint. But as function calls add overhead, this function should only be called when the tracepoint in question is enabled. To get around the overhead, a static_branch can be used that only gets set when the tracepoint is enabled, and then inside the block of the static branch can contain the call to the tracepoint wrapper.
Add a tracepoint_enabled(tp) macro that gets passed the name of the tracepoint, and this becomes a static_branch that is enabled when the tracepoint is enabled and is a nop when the tracepoint is disabled.
--- Documentation/trace/tracepoints.rst | 25 ++++++++++++++++++++++ include/linux/tracepoint-defs.h | 33 +++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+)
Documentation/trace/tracepoints.rst +25 -0
@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled() +function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled(). + +In a C file:: + + void do_trace_foo_bar_wrapper(args) + { + trace_foo_bar(args); + } + +In the header file:: + + DECLEARE_TRACEPOINT(foo_bar); + + static inline void some_inline_function() + { + [..] + if (trace_enabled(foo_bar)) + do_trace_foo_bar_wrapper(args); + [..] + }
include/linux/tracepoint-defs.h +33 -0
@@ -48,4 +48,37 @@ struct bpf_raw_event_map { u32 writable_size; }__aligned(32) ; +/* +* If a tracepoint needs to be called from a header file, it is not +* recommended to call it directly, as tracepoints in header files +* may cause side-effects. Instead, use trace_enabled() to test +* if the tracepoint is enabled, then if it is, call a wrapper +* function defined in a C file that will then call the tracepoint. +* +* For "trace_foo()", you would need to create a wrapper function +* in a C file to call trace_foo(): +* void trace_bar(args) { trace_foo(args); } +* Then in the header file, declare the tracepoint: +* DECLARE_TRACEPOINT(foo); +* And call your wrapper: +* static inline void some_inlined_function() { +* [..] +* if (tracepoint_enabled(foo)) +* trace_bar(args); +* [..] +* } +* +* Note: tracepoint_enabled(foo) is equivalent to trace_foo_enabled() +* but is safe to have in headers, where trace_foo_enabled() is not. +*/ +#define DECLARE_TRACEPOINT (tp) \ + extern struct tracepoint __tracepoint_##tp + +#ifdef CONFIG_TRACEPOINTS +# define tracepoint_enabled (tp) \ + static_key_false(&(__tracepoint_##tp).key) +#else +# define tracepoint_enabled (tracepoint) false +#endif +#endif
Signature · 1 line
2.28.0
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 27 lines · ----- On Sep 24, 2020, at 1:09 PM, rostedt [email protected] wrote:
From: "Steven Rostedt (VMware)" <[email protected]>
As tracepoints are discouraged from being added in a header because it can cause side effects if other tracepoints are in headers, the common workaround is to add a function call that calls a wrapper function in a C file that then calls the tracepoint. But as function calls add overhead, this function should only be called when the tracepoint in question is enabled. To get around the overhead, a
static_branchcan be used that only gets set when the tracepoint is enabled, and then inside the block of the static branch can contain the call to the tracepoint wrapper.Add a
tracepoint_enabled(tp)macro that gets passed the name of the tracepoint, and this becomes astatic_branchthat is enabled when the tracepoint is enabled and is a nop when the tracepoint is disabled.Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +0
-0
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 9 lines
Documentation/trace/tracepoints.rst +6
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled() +function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().
Tracepoints per-se have no issues being used from header files. The TRACE_EVENT infrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.
I am not against the idea of a tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.
Quoted 6 lines
Documentation/trace/tracepoints.rst +6
-0+ +In a C file:: + + void do_trace_foo_bar_wrapper(args) + { + trace_foo_bar(args);
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 170
Quoted 10 lines
Documentation/trace/tracepoints.rst +10
-0+ } + +In the header file:: + + DECLEARE_TRACEPOINT(foo_bar); + + static inline void some_inline_function() + { + [..] + if (trace_enabled(foo_bar))
Is it trace_enabled() or tracepoint_enabled() ? There is a mismatch between the commit message/code and the documentation.
Thanks,
Mathieu
Quoted 47 lines
Documentation/trace/tracepoints.rst +3
-0+ do_trace_foo_bar_wrapper(args); + [..] + }include/linux/tracepoint-defs.h +33
-1@@ -48,4 +48,37 @@ struct bpf_raw_event_map { u32 writable_size; }__aligned(32) ; +/* +* If a tracepoint needs to be called from a header file, it is not +* recommended to call it directly, as tracepoints in header files +* may cause side-effects. Instead, use trace_enabled() to test +* if the tracepoint is enabled, then if it is, call a wrapper +* function defined in a C file that will then call the tracepoint. +* +* For "trace_foo()", you would need to create a wrapper function +* in a C file to call trace_foo(): +* void trace_bar(args) { trace_foo(args); } +* Then in the header file, declare the tracepoint: +* DECLARE_TRACEPOINT(foo); +* And call your wrapper: +* static inline void some_inlined_function() { +* [..] +* if (tracepoint_enabled(foo)) +* trace_bar(args); +* [..] +* } +* +* Note: tracepoint_enabled(foo) is equivalent to trace_foo_enabled() +* but is safe to have in headers, where trace_foo_enabled() is not. +*/ +#define DECLARE_TRACEPOINT (tp) \ + extern struct tracepoint __tracepoint_##tp + +#ifdef CONFIG_TRACEPOINTS +# define tracepoint_enabled (tp) \ + static_key_false(&(__tracepoint_##tp).key) +#else +# define tracepoint_enabled (tracepoint) false +#endif +#endif --2.28.0
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 45 lines · Mathieu Desnoyers
Quoted 27 lines · ----- On Sep 24, 2020, at 1:09 PM, rostedt [email protected] wrote:
From: "Steven Rostedt (VMware)" <[email protected]>
As tracepoints are discouraged from being added in a header because it can cause side effects if other tracepoints are in headers, the common workaround is to add a function call that calls a wrapper function in a C file that then calls the tracepoint. But as function calls add overhead, this function should only be called when the tracepoint in question is enabled. To get around the overhead, a
static_branchcan be used that only gets set when the tracepoint is enabled, and then inside the block of the static branch can contain the call to the tracepoint wrapper.Add a
tracepoint_enabled(tp)macro that gets passed the name of the tracepoint, and this becomes astatic_branchthat is enabled when the tracepoint is enabled and is a nop when the tracepoint is disabled.Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +0
-0Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 9 lines
Documentation/trace/tracepoints.rst +6
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled() +function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().Tracepoints per-se have no issues being used from header files. The
TRACE_EVENTinfrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.
Quoted 2 lines
I am not against the idea of a
tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.
Perhaps anecdotally, I've found that the situation Steven described occurs not just because of the TRACE_EVENT infrastructure. We also run into this problem when adding tracepoints under any "very core" APIs, i.e. anything that is transiently included from linux/tracepoint.h. For example, I ran into this issue while adding tracepoints under the linux/mmap_lock.h API, because that header is somehow transiently included by linux/tracepoint.h (sorry, I don't have the exact transient include path on hand; I can dig it up if it would be useful).
Quoted 77 lines
Quoted 7 lines
Documentation/trace/tracepoints.rst +6
-0+ +In a C file:: + + void do_trace_foo_bar_wrapper(args) + { + trace_foo_bar(args);Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 170
Quoted 10 lines
Documentation/trace/tracepoints.rst +10
-0+ } + +In the header file:: + + DECLEARE_TRACEPOINT(foo_bar); + + static inline void some_inline_function() + { + [..] + if (trace_enabled(foo_bar))Is it
trace_enabled()ortracepoint_enabled()? There is a mismatch between the commit message/code and the documentation.Thanks,
Mathieu
Quoted 46 lines
Documentation/trace/tracepoints.rst +3
-0+ do_trace_foo_bar_wrapper(args); + [..] + }include/linux/tracepoint-defs.h +33
-1@@ -48,4 +48,37 @@ struct bpf_raw_event_map { u32 writable_size; }__aligned(32) ; +/* +* If a tracepoint needs to be called from a header file, it is not +* recommended to call it directly, as tracepoints in header files +* may cause side-effects. Instead, use trace_enabled() to test +* if the tracepoint is enabled, then if it is, call a wrapper +* function defined in a C file that will then call the tracepoint. +* +* For "trace_foo()", you would need to create a wrapper function +* in a C file to call trace_foo(): +* void trace_bar(args) { trace_foo(args); } +* Then in the header file, declare the tracepoint: +* DECLARE_TRACEPOINT(foo); +* And call your wrapper: +* static inline void some_inlined_function() { +* [..] +* if (tracepoint_enabled(foo)) +* trace_bar(args); +* [..] +* } +* +* Note: tracepoint_enabled(foo) is equivalent to trace_foo_enabled() +* but is safe to have in headers, where trace_foo_enabled() is not. +*/ +#define DECLARE_TRACEPOINT (tp) \ + extern struct tracepoint __tracepoint_##tp + +#ifdef CONFIG_TRACEPOINTS +# define tracepoint_enabled (tp) \ + static_key_false(&(__tracepoint_##tp).key) +#else +# define tracepoint_enabled (tracepoint) false +#endif +#endif --Quoted 1 line
2.28.0
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 51 lines · ----- On Sep 24, 2020, at 2:19 PM, Axel Rasmussen [email protected]…
Quoted 45 lines · Mathieu Desnoyers
Quoted 27 lines · ----- On Sep 24, 2020, at 1:09 PM, rostedt [email protected] wrote:
From: "Steven Rostedt (VMware)" <[email protected]>
As tracepoints are discouraged from being added in a header because it can cause side effects if other tracepoints are in headers, the common workaround is to add a function call that calls a wrapper function in a C file that then calls the tracepoint. But as function calls add overhead, this function should only be called when the tracepoint in question is enabled. To get around the overhead, a
static_branchcan be used that only gets set when the tracepoint is enabled, and then inside the block of the static branch can contain the call to the tracepoint wrapper.Add a
tracepoint_enabled(tp)macro that gets passed the name of the tracepoint, and this becomes astatic_branchthat is enabled when the tracepoint is enabled and is a nop when the tracepoint is disabled.Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +0
-0Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 9 lines
Documentation/trace/tracepoints.rst +6
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled() +function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().Tracepoints per-se have no issues being used from header files. The
TRACE_EVENTinfrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.Quoted 3 lines
I am not against the idea of a
tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.
Quoted 9 lines
Perhaps anecdotally, I've found that the situation Steven described occurs not just because of the
TRACE_EVENTinfrastructure. We also run into this problem when adding tracepoints under any "very core" APIs, i.e. anything that is transiently included fromlinux/tracepoint.h. For example, I ran into this issue while adding tracepoints under thelinux/mmap_lock.hAPI, because that header is somehow transiently included bylinux/tracepoint.h(sorry, I don't have the exact transient include path on hand; I can dig it up if it would be useful).
If it's not too much trouble, it would be useful to know what headers included from tracepoint.h are problematic. When I originally wrote tracepoint.h, I made sure to include the minimum set needed, but I suspect some feature-creep may have ended up including additional headers which are not strictly needed for the core instrumentation.
Once we identify those, we can then make sure we split tracepoint.h into a "core instrumentation" header (tracepoint.h) and non-core stuff (e.g. tracepoint-utils.h or better name).
Another possible approach is to make sure that whatever is included by the tracepoint.h "core" is split into *.h and *-{defs,types}.h, for cases where all we need from tracepoint.h is type declarations.
Thanks,
Mathieu
Quoted 80 lines
Quoted 80 lines
Quoted 10 lines
Documentation/trace/tracepoints.rst +6
-0+ +In a C file:: + + void do_trace_foo_bar_wrapper(args) + { + trace_foo_bar(args);Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 170
Quoted 10 lines
Documentation/trace/tracepoints.rst +10
-0+ } + +In the header file:: + + DECLEARE_TRACEPOINT(foo_bar); + + static inline void some_inline_function() + { + [..] + if (trace_enabled(foo_bar))Is it
trace_enabled()ortracepoint_enabled()? There is a mismatch between the commit message/code and the documentation.Thanks,
Mathieu
Quoted 46 lines
Documentation/trace/tracepoints.rst +3
-0+ do_trace_foo_bar_wrapper(args); + [..] + }include/linux/tracepoint-defs.h +33
-1@@ -48,4 +48,37 @@ struct bpf_raw_event_map { u32 writable_size; }__aligned(32) ; +/* +* If a tracepoint needs to be called from a header file, it is not +* recommended to call it directly, as tracepoints in header files +* may cause side-effects. Instead, use trace_enabled() to test +* if the tracepoint is enabled, then if it is, call a wrapper +* function defined in a C file that will then call the tracepoint. +* +* For "trace_foo()", you would need to create a wrapper function +* in a C file to call trace_foo(): +* void trace_bar(args) { trace_foo(args); } +* Then in the header file, declare the tracepoint: +* DECLARE_TRACEPOINT(foo); +* And call your wrapper: +* static inline void some_inlined_function() { +* [..] +* if (tracepoint_enabled(foo)) +* trace_bar(args); +* [..] +* } +* +* Note: tracepoint_enabled(foo) is equivalent to trace_foo_enabled() +* but is safe to have in headers, where trace_foo_enabled() is not. +*/ +#define DECLARE_TRACEPOINT (tp) \ + extern struct tracepoint __tracepoint_##tp + +#ifdef CONFIG_TRACEPOINTS +# define tracepoint_enabled (tp) \ + static_key_false(&(__tracepoint_##tp).key) +#else +# define tracepoint_enabled (tracepoint) false +#endif +#endif --Quoted 1 line
2.28.0
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
In reply to Mathieu DesnoyersQuoted 18 lines · On Thu, 24 Sep 2020 13:42:25 -0400 (EDT) Mathieu Desnoyers…
Quoted 18 lines
Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +3
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled()
Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 3 lines
Documentation/trace/tracepoints.rst +3
-0+function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().Tracepoints per-se have no issues being used from header files. The
TRACE_EVENTinfrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.
That's like saying "let's solve include hell". Note, in the past there has also been issues with the headers included also having issues including other headers and cause a dependency loop.
But the magic of trace events (for both perf and ftrace, sorry if you refused to use it), is that people who add tracepoints do not need to know how tracepoints work. There's no adding of registering of them, or anything else. The formats and everything they record appear in the tracefs file system.
How are your trace events created? All manual, or do you have helper macros. Would these be safe if a bunch were included?
Quoted 3 lines
I am not against the idea of a
tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.
It removes the open coded code that has been in the kernel for the last 4 years.
Quoted 12 lines
Quoted 12 lines
Documentation/trace/tracepoints.rst +11
-0+ +In a C file:: + + void do_trace_foo_bar_wrapper(args) + { + trace_foo_bar(args); + } + +In the header file:: + + DECLEARE_TRACEPOINT(foo_bar);
Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 170
Quoted 5 lines
Documentation/trace/tracepoints.rst +5
-0+ + static inline void some_inline_function() + { + [..] + if (trace_enabled(foo_bar))Is it
trace_enabled()ortracepoint_enabled()? There is a mismatch between the commit message/code and the documentation.
Yes, it should be tracepoint_enabled(). Thanks for catching that.
Anyway, this shouldn't affect you in any way, as it's just adding wrappers around locations that have been doing this for years.
If you want, I can change the name to trace_event_enabled() and put the code in trace_events-defs.h instead. Which would simply include tracepoints-defs.h and have the exact same code.
-- Steve
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 30 lines · ----- On Sep 24, 2020, at 2:30 PM, rostedt [email protected] wrote:
Quoted 18 lines · On Thu, 24 Sep 2020 13:42:25 -0400 (EDT) Mathieu Desnoyers…
Quoted 18 lines
Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +3
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled()Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 3 lines
Documentation/trace/tracepoints.rst +3
-0+function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().Tracepoints per-se have no issues being used from header files. The
TRACE_EVENTinfrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.
Quoted 3 lines
That's like saying "let's solve include hell". Note, in the past there has also been issues with the headers included also having issues including other headers and cause a dependency loop.
AFAIU, there are a few scenarios we care about here:
1) Includes done by tracepoint.h (directly and indirectly). Some additional includes may have crept in and bloated tracepoint.h since its original implementation. We should identify and fix those.
2) Includes done by trace event headers when CREATE_TRACE_POINTS is defined. define_trace.h already takes care of #undef/#define CREATE_TRACE_POINTS to prevent recursion, so it should not be an issue.
3) Includes done by a compile unit after #define CREATE_TRACE_POINTS, but which are not meant to create trace point probes. For this case, requiring to #undef CREATE_TRACE_POINTS after all the relevant headers are included should solve it.
So what additional issues am I missing here ?
Quoted 3 lines
But the magic of trace events (for both perf and ftrace, sorry if you refused to use it),
I cannot not use it to this day because changes I needed to upstream in order to make it useful to me were rejected.
Quoted 4 lines
is that people who add tracepoints do not need to know
how tracepoints work. There's no adding of registering of them, or anything else. The formats and everything they record appear in the tracefs file system.
That's all fine by me.
Quoted 3 lines
How are your trace events created? All manual, or do you have helper macros.
I suspect you mean LTTng's trace events. Those are created with helper macros (LTTNG_TRACEPOINT_EVENT) which have a few improvements over TRACE_EVENT, namely:
- Ability to create arrays of events (because lots of semicolons are removed), removing the need to dynamically register each event at init time,
- Ability to do pre-filtering of events, before they hit the ring buffer, allowed by combining TP_struct and TP_fast_assign into a single structured TP_FIELDS.
LTTng also has an include pass which uses TRACE_EVENT from the kernel to perform tracepoint prototype signature validation of LTTNG_TRACEPOINT_EVENT against TRACE_EVENT prototypes.
Quoted 1 line
Would these be safe if a bunch were included?
Can you elaborate on this question ? I have a hard time figuring out what scenario(s) you are after.
Quoted 8 lines
Quoted 5 lines
I am not against the idea of a
tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.It removes the open coded code that has been in the kernel for the last 4 years.
There are indeed many cases where a tracepoint_enabled() macro makes sense. In some situations we encounter in user-space for lttng-ust, there is need to prepare data before it is passed as tracepoint arguments. Having this "enabled" macros allows to only prepare the data when needed.
Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
Documentation/trace/tracepoints.rst +11
-0+ +In a C file:: + + void do_trace_foo_bar_wrapper(args) + { + trace_foo_bar(args); + } + +In the header file:: + + DECLEARE_TRACEPOINT(foo_bar);Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 170
Quoted 5 lines
Documentation/trace/tracepoints.rst +5
-0+ + static inline void some_inline_function() + { + [..] + if (trace_enabled(foo_bar))Is it
trace_enabled()ortracepoint_enabled()? There is a mismatch between the commit message/code and the documentation.
Quoted 8 lines
Yes, it should be
tracepoint_enabled(). Thanks for catching that.Anyway, this shouldn't affect you in any way, as it's just adding wrappers around locations that have been doing this for years.
If you want, I can change the name to
trace_event_enabled()and put the code intrace_events-defs.hinstead. Which would simply includetracepoints-defs.hand have the exact same code.
I'm ok with tracepoint_enabled(). However, I would have placed it in tracepoint.h rather than tracepoint-defs.h, and we should figure out why people complain that tracepoint.h is including headers too eagerly.
Thanks,
Mathieu
Quoted 2 lines
-- Steve
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 46 lines · On Thu, 24 Sep 2020 15:08:30 -0400 (EDT) Mathieu Desnoyers…
Quoted 30 lines · ----- On Sep 24, 2020, at 2:30 PM, rostedt [email protected] wrote:
Quoted 18 lines · On Thu, 24 Sep 2020 13:42:25 -0400 (EDT) Mathieu Desnoyers…
Quoted 18 lines
Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +3
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled()Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 3 lines
Documentation/trace/tracepoints.rst +3
-0+function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().Tracepoints per-se have no issues being used from header files. The
TRACE_EVENTinfrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.Quoted 3 lines
That's like saying "let's solve include hell". Note, in the past there has also been issues with the headers included also having issues including other headers and cause a dependency loop.
AFAIU, there are a few scenarios we care about here:
1) Includes done by
tracepoint.h(directly and indirectly). Some additional includes may have crept in and bloatedtracepoint.hsince its original implementation. We should identify and fix those.2) Includes done by trace event headers when
CREATE_TRACE_POINTSis defined.define_trace.halready takes care of #undef/#defineCREATE_TRACE_POINTSto prevent recursion, so it should not be an issue.
Quoted 6 lines
3) Includes done by a compile unit after #define
CREATE_TRACE_POINTS, but which are not meant to create trace point probes. For this case, requiring to #undefCREATE_TRACE_POINTSafter all the relevant headers are included should solve it.So what additional issues am I missing here ?
How do we get to DECLARE_TRACE()?
Right now it requires a TRACE_EVENT() macro. Those are in the include/trace/events/ directory and some external. The files there that create all trace events and their tracepoints. Although you may not use TRACE_EVENT() yourself, you use what was created by them, which includes the trace_<tracepoint>() call.
As the TRACE_EVENT macro defines the parameters passed to the function, it's a requirement.
Quoted 12 lines
Quoted 3 lines
How are your trace events created? All manual, or do you have helper macros.
I suspect you mean LTTng's trace events. Those are created with helper macros (
LTTNG_TRACEPOINT_EVENT) which have a few improvements overTRACE_EVENT, namely:- Ability to create arrays of events (because lots of semicolons are removed), removing the need to dynamically register each event at init time,
- Ability to do pre-filtering of events, before they hit the ring buffer, allowed by combiningTP_structandTP_fast_assigninto a single structuredTP_FIELDS.
Quoted 8 lines
LTTng also has an include pass which uses
TRACE_EVENTfrom the kernel to perform tracepoint prototype signature validation ofLTTNG_TRACEPOINT_EVENTagainstTRACE_EVENTprototypes.Quoted 1 line
Would these be safe if a bunch were included?
Can you elaborate on this question ? I have a hard time figuring out what scenario(s) you are after.
Actually, this doesn't matter. It's the creating of the trace_*() that is important here, not the call backs.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
I am not against the idea of a
tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.
Quoted 7 lines
Quoted 2 lines
It removes the open coded code that has been in the kernel for the last 4 years.
There are indeed many cases where a
tracepoint_enabled()macro makes sense. In some situations we encounter in user-space for lttng-ust, there is need to prepare data before it is passed as tracepoint arguments. Having this "enabled" macros allows to only prepare the data when needed.
We already have it today. It's just not safe to have in header files. This extension allows it, but also requires adding the DECLARE_TRACEPOINT(). Hmm, we may be able to replace the trace_<tracepoint>_enabled() with this instead.
Quoted 9 lines
Quoted 4 lines
If you want, I can change the name to
trace_event_enabled()and put the code intrace_events-defs.hinstead. Which would simply includetracepoints-defs.hand have the exact same code.I'm ok with
tracepoint_enabled(). However, I would have placed it intracepoint.hrather thantracepoint-defs.h, and we should figure out why people complain thattracepoint.his including headers too eagerly.
I could see if it would work in tracepoints.h.
It might. I was just being extra cautious.
-- Steve
Steven Rostedt
maintainerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 8 lines · On Thu, 24 Sep 2020 15:35:17 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 4 lines
I'm ok with
tracepoint_enabled(). However, I would have placed it intracepoint.hrather thantracepoint-defs.h, and we should figure out why people complain thattracepoint.his including headers too eagerly.I could see if it would work in
tracepoints.h.It might. I was just being extra cautious.
Well that blew up quick!
Try using tracepoint.h in include/linux/page_ref.h and arch/x86/include/asm/msr.h and see what happens.
CC arch/x86/kernel/asm-offsets.s
In file included from /work/git/linux-trace.git/include/linux/srcu.h:20,
from /work/git/linux-trace.git/include/linux/tracepoint.h:16,
from /work/git/linux-trace.git/arch/x86/include/asm/msr.h:68,
from /work/git/linux-trace.git/arch/x86/include/asm/processor.h:22,
from /work/git/linux-trace.git/arch/x86/include/asm/cpufeature.h:5,
from /work/git/linux-trace.git/arch/x86/include/asm/thread_info.h:53,
from /work/git/linux-trace.git/include/linux/thread_info.h:38,
from /work/git/linux-trace.git/arch/x86/include/asm/preempt.h:7,
from /work/git/linux-trace.git/include/linux/preempt.h:78,
from /work/git/linux-trace.git/include/linux/spinlock.h:51,
from /work/git/linux-trace.git/include/linux/mmzone.h:8,
from /work/git/linux-trace.git/include/linux/gfp.h:6,
from /work/git/linux-trace.git/include/linux/slab.h:15,
from /work/git/linux-trace.git/include/linux/crypto.h:20,
from /work/git/linux-trace.git/arch/x86/kernel/asm-offsets.c:9:
/work/git/linux-trace.git/include/linux/rcupdate.h: In function ‘rcu_read_lock_sched’:
/work/git/linux-trace.git/include/linux/rcupdate.h:740:2: error: implicit declaration of function ‘preempt_disable’ [-Werror=implicit-function-declaration]
740 | preempt_disable();
| ^~~~~~~~~~~~~~~
/work/git/linux-trace.git/include/linux/rcupdate.h: In function ‘rcu_read_lock_sched_notrace’:
/work/git/linux-trace.git/include/linux/rcupdate.h:750:2: error: implicit declaration of function ‘preempt_disable_notrace’; did you mean ‘paravirt_disable_iospace’? [-Werror=implicit-function-declaration]
750 | preempt_disable_notrace();
| ^~~~~~~~~~~~~~~~~~~~~~~
| paravirt_disable_iospace
/work/git/linux-trace.git/include/linux/rcupdate.h: In function ‘rcu_read_unlock_sched’:
/work/git/linux-trace.git/include/linux/rcupdate.h:765:2: error: implicit declaration of function ‘preempt_enable’ [-Werror=implicit-function-declaration]
765 | preempt_enable();
| ^~~~~~~~~~~~~~
/work/git/linux-trace.git/include/linux/rcupdate.h: In function ‘rcu_read_unlock_sched_notrace’:
/work/git/linux-trace.git/include/linux/rcupdate.h:772:2: error: implicit declaration of function ‘preempt_enable_notrace’ [-Werror=implicit-function-declaration]
772 | preempt_enable_notrace();
| ^~~~~~~~~~~~~~~~~~~~~~-- Steve
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 8 lines · ----- On Sep 24, 2020, at 3:40 PM, rostedt [email protected] wrote:
Quoted 5 lines · On Thu, 24 Sep 2020 15:35:17 -0400 Steven Rostedt <[email protected]> wrote:
Quoted 4 lines
I'm ok with
tracepoint_enabled(). However, I would have placed it intracepoint.hrather thantracepoint-defs.h, and we should figure out why people complain thattracepoint.his including headers too eagerly.
Quoted 8 lines
Quoted 3 lines
I could see if it would work in
tracepoints.h.It might. I was just being extra cautious.
Well that blew up quick!
Try using
tracepoint.hininclude/linux/page_ref.handarch/x86/include/asm/msr.hand see what happens.
Indeed, msr.h is an issue. So having tracepoint_enabled() in include/linux/tracepoints-defs.h and going through a function would make sense for this kind of core use-case I guess.
I tried including tracepoint.h from include/linux/page_ref.h and did not notice any compile issue. Am I missing something to trigger an issue related to that scenario ?
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
In reply to Steven RostedtQuoted 56 lines · ----- On Sep 24, 2020, at 3:35 PM, rostedt [email protected] wrote:
Quoted 46 lines · On Thu, 24 Sep 2020 15:08:30 -0400 (EDT) Mathieu Desnoyers…
Quoted 30 lines · ----- On Sep 24, 2020, at 2:30 PM, rostedt [email protected] wrote:
Quoted 18 lines · On Thu, 24 Sep 2020 13:42:25 -0400 (EDT) Mathieu Desnoyers…
Quoted 18 lines
Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +3
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled()Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 3 lines
Documentation/trace/tracepoints.rst +3
-0+function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().Tracepoints per-se have no issues being used from header files. The
TRACE_EVENTinfrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.Quoted 3 lines
That's like saying "let's solve include hell". Note, in the past there has also been issues with the headers included also having issues including other headers and cause a dependency loop.
AFAIU, there are a few scenarios we care about here:
1) Includes done by
tracepoint.h(directly and indirectly). Some additional includes may have crept in and bloatedtracepoint.hsince its original implementation. We should identify and fix those.2) Includes done by trace event headers when
CREATE_TRACE_POINTSis defined.define_trace.halready takes care of #undef/#defineCREATE_TRACE_POINTSto prevent recursion, so it should not be an issue.Quoted 6 lines
3) Includes done by a compile unit after #define
CREATE_TRACE_POINTS, but which are not meant to create trace point probes. For this case, requiring to #undefCREATE_TRACE_POINTSafter all the relevant headers are included should solve it.So what additional issues am I missing here ?
Quoted 7 lines
How do we get to
DECLARE_TRACE()?Right now it requires a
TRACE_EVENT()macro. Those are in theinclude/trace/events/directory and some external. The files there that create all trace events and their tracepoints. Although you may not useTRACE_EVENT()yourself, you use what was created by them, which includes the trace_<tracepoint>() call.
Just to be clear: LTTng (the kernel tracer) uses the TP_PROTO() field of the TRACE_EVENT(). So it does need it as well for prototype validation.
Quoted 2 lines
As the
TRACE_EVENTmacro defines the parameters passed to the function, it's a requirement.
Yes.
So is the underlying issue caused by headers under include/trace/events/ including various headers which are only needed for probe creation with CREATE_TRACE_PROBES, but useless for TP_PROTO() ? If so, those includes could easily be wrapped with #ifndef CREATE_TRACE_POINTS / #include .... / #endif. module.h, sched.h and writeback.h already use that trick to only create static inline helpers when included from probe creation context.
Quoted 21 lines
Quoted 21 lines
Quoted 5 lines
How are your trace events created? All manual, or do you have helper macros.
I suspect you mean LTTng's trace events. Those are created with helper macros (
LTTNG_TRACEPOINT_EVENT) which have a few improvements overTRACE_EVENT, namely:- Ability to create arrays of events (because lots of semicolons are removed), removing
the need to dynamically register each event at init time,
- Ability to do pre-filtering of events, before they hit the ring buffer, allowed by
combiningTP_structandTP_fast_assigninto a single structuredTP_FIELDS.LTTng also has an include pass which uses
TRACE_EVENTfrom the kernel to perform tracepoint prototype signature validation ofLTTNG_TRACEPOINT_EVENTagainstTRACE_EVENT
prototypes.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Would these be safe if a bunch were included?
Can you elaborate on this question ? I have a hard time figuring out what scenario(s) you are after.
Actually, this doesn't matter. It's the creating of the trace_*() that is important here, not the call backs.
Right. But headers included as dependencies for the call backs might be the root cause of our descent into #include hell. ;-)
Quoted 17 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
I am not against the idea of a
tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.Quoted 7 lines
Quoted 2 lines
It removes the open coded code that has been in the kernel for the last 4 years.
There are indeed many cases where a
tracepoint_enabled()macro makes sense. In some situations we encounter in user-space for lttng-ust, there is need to prepare data before it is passed as tracepoint arguments. Having this "enabled" macros allows to only prepare the data when needed.
Quoted 4 lines
We already have it today. It's just not safe to have in header files. This extension allows it, but also requires adding the
DECLARE_TRACEPOINT(). Hmm, we may be able to replace the trace_<tracepoint>_enabled() with this instead.
Yes. It could probably look like the tracepoint_enabled() macro in lttng-ust [1]. It does not require any special "DECLARE_TRACEPOINT". The only difference in the Linux kernel is that it should support static branches/asm gotos.
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
If you want, I can change the name to
trace_event_enabled()and put the code intrace_events-defs.hinstead. Which would simply includetracepoints-defs.hand have the exact same code.
Quoted 8 lines
Quoted 4 lines
I'm ok with
tracepoint_enabled(). However, I would have placed it intracepoint.hrather thantracepoint-defs.h, and we should figure out why people complain thattracepoint.his including headers too eagerly.I could see if it would work in
tracepoints.h.It might. I was just being extra cautious.
If headers included by tracepoint.h are an issue, we should look into that and remove what is unneeded.
If headers which happens to be included by include/trace/events/ headers are the issue, and they happen to only be needed by CREATE_TRACE_PROBES, then we should consider wrapping those #include with #ifdef CREATE_TRACE_PROBES guards.
Quoted 2 lines
-- Steve
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 3 lines · On Thu, 24 Sep 2020 16:05:35 -0400 (EDT) Mathieu Desnoyers…
If headers which happens to be included by
include/trace/events/headers are the issue, and they happen to only be needed byCREATE_TRACE_PROBES, then we should consider wrapping those #include with #ifdefCREATE_TRACE_PROBESguards.
Well, we are at a point you can't even use preempt_disable().
Whatever wants to use this tracepoint_enabled() macro, is going to have to include something that doesn't include tracepoint.h.
-- Steve
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 11 lines · ----- On Sep 24, 2020, at 4:13 PM, rostedt [email protected] wrote:
Quoted 3 lines · On Thu, 24 Sep 2020 16:05:35 -0400 (EDT) Mathieu Desnoyers…
If headers which happens to be included by
include/trace/events/headers are the issue, and they happen to only be needed byCREATE_TRACE_PROBES, then we should consider wrapping those #include with #ifdefCREATE_TRACE_PROBESguards.Well, we are at a point you can't even use
preempt_disable().Whatever wants to use this
tracepoint_enabled()macro, is going to have to include something that doesn't includetracepoint.h.
I'd be a bit more specific: so far, the msr.h use-case requires to include directly tracepoint-defs.h and use a tracepoint_enabled() macro defined there.
Other less "core" header use-cases could still include tracepoint.h, as long as there is no circular dependency.
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 5 lines · On Thu, 24 Sep 2020 16:27:34 -0400 (EDT) Mathieu Desnoyers…
I'd be a bit more specific: so far, the
msr.huse-case requires to include directlytracepoint-defs.hand use atracepoint_enabled()macro defined there.Other less "core" header use-cases could still include
tracepoint.h, as long as there is no circular dependency.
Well, I'll keep tracepoint-defs.h for the msr.h case, and I could see if tracepoint.h is good enough for the other cases.
But does it really matter, if we only need what is in tracepoint-defs.h? Why add something that may cause issues in the future?
-- Steve
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 9 lines · ----- On Sep 24, 2020, at 4:33 PM, rostedt [email protected] wrote:
Quoted 5 lines · On Thu, 24 Sep 2020 16:27:34 -0400 (EDT) Mathieu Desnoyers…
I'd be a bit more specific: so far, the
msr.huse-case requires to include directlytracepoint-defs.hand use atracepoint_enabled()macro defined there.Other less "core" header use-cases could still include
tracepoint.h, as long as there is no circular dependency.
Quoted 6 lines
Well, I'll keep
tracepoint-defs.hfor themsr.hcase, and I could see iftracepoint.his good enough for the other cases.But does it really matter, if we only need what is in
tracepoint-defs.h? Why add something that may cause issues in the future?
The trade-off here is tracing (on) speed and code size vs header instrumentation coverage.
Adding the trampoline as is done in msr.h adds the overhead of an extra function call when tracing is active. It also slightly increases the code size. This is why we don't have that extra trampoline in the common case.
The main limitation with respect to tracepoint instrumentation coverage is header dependencies of RCU read-side synchronization. Currently, tracepoint.h uses rcu-sched and SRCU. Moving that synchronization into a trampoline is one way to work-around circular dependency issues.
Note that I have plans to make tracepoint.h use Tasks Trace RCU as well, so some probes can take pages faults (especially useful for sys enter/exit). Michael Jeanson has been working on a prototype implementing this, and he should be able to post a RFC patch publicly soon.
That being said, I suspect that Tasks Trace RCU has fewer header dependencies than rcu-sched and SRCU. Maybe one idea worth considering is replacing tracepoint's use of rcu-sched and SRCU by Tasks Trace RCU altogether, if the latter has read-side performance close to rcu-sched. This could be another way to minimize the amount of tracepoint.h header dependencies.
With the current dependencies of tracepoint.h, I would argue that we should only do the trampoline work-around for cases where there is an unavoidable circular dependency, like the case of msr.h. For other headers which don't have circular dependency issues with tracepoint.h, we should use the usual tracepoint instrumentation because not having the trampoline provides better tracing (on) speed and reduces (slightly) code size.
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 6 lines · On Fri, 25 Sep 2020 10:41:56 -0400 (EDT) Mathieu Desnoyers…
With the current dependencies of
tracepoint.h, I would argue that we should only do the trampoline work-around for cases where there is an unavoidable circular dependency, like the case ofmsr.h. For other headers which don't have circular dependency issues withtracepoint.h, we should use the usual tracepoint instrumentation because not having the trampoline provides better tracing (on) speed and reduces (slightly) code size.
Well, for now, I'm going to add the helper function and have the header use cases use that.
A while back ago I had patches that moves the DO_TRACE() work into a separate function and with that we probably could have let all tracepoints be in headers (as they would all just do a function call to the trace algorithm that does the rest of the work). But you balked at that because of the added overhead with tracing on.
Anyway, I don't see any issues with the current patch set as is (besides the documentation fix, which I already updated locally). And will add this to my queue for linux-next.
-- Steve
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 19 lines · ----- On Sep 25, 2020, at 11:14 AM, rostedt [email protected] wrote:
Quoted 6 lines · On Fri, 25 Sep 2020 10:41:56 -0400 (EDT) Mathieu Desnoyers…
With the current dependencies of
tracepoint.h, I would argue that we should only do the trampoline work-around for cases where there is an unavoidable circular dependency, like the case ofmsr.h. For other headers which don't have circular dependency issues withtracepoint.h, we should use the usual tracepoint instrumentation because not having the trampoline provides better tracing (on) speed and reduces (slightly) code size.Well, for now, I'm going to add the helper function and have the header use cases use that.
A while back ago I had patches that moves the
DO_TRACE()work into a separate function and with that we probably could have let all tracepoints be in headers (as they would all just do a function call to the trace algorithm that does the rest of the work). But you balked at that because of the added overhead with tracing on.
Quoted 3 lines
Anyway, I don't see any issues with the current patch set as is (besides the documentation fix, which I already updated locally). And will add this to my queue for linux-next.
The only thing I would change in the documentation is to word this as "here is a trampoline trick which can be used to work-around rare cases of tracepoint header circular dependency issues" rather than "always use this when instrumenting a header".
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Steven Rostedt
maintainerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 9 lines · On Fri, 25 Sep 2020 11:30:06 -0400 (EDT) Mathieu Desnoyers…
Quoted 3 lines
Anyway, I don't see any issues with the current patch set as is (besides the documentation fix, which I already updated locally). And will add this to my queue for linux-next.
The only thing I would change in the documentation is to word this as "here is a trampoline trick which can be used to work-around rare cases of tracepoint header circular dependency issues" rather than "always use this when instrumenting a header".
I rather not have tracepoints in headers. Period!
It's not just about circular dependencies, it also bloats the code.
-- Steve
Mathieu Desnoyers
reviewerRe: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
Quoted 13 lines · ----- On Sep 25, 2020, at 12:26 PM, rostedt [email protected] wrote:
Quoted 9 lines · On Fri, 25 Sep 2020 11:30:06 -0400 (EDT) Mathieu Desnoyers…
Quoted 3 lines
Anyway, I don't see any issues with the current patch set as is (besides the documentation fix, which I already updated locally). And will add this to my queue for linux-next.
The only thing I would change in the documentation is to word this as "here is a trampoline trick which can be used to work-around rare cases of tracepoint header circular dependency issues" rather than "always use this when instrumenting a header".
Quoted 3 lines
I rather not have tracepoints in headers. Period!
It's not just about circular dependencies, it also bloats the code.
Fair enough. We could indeed argue that having a tracepoint in a header's static inline function will end up replicating that tracepoint at every site where the function is used. So in terms of code size, it's better to use the trampoline approach.
Thanks,
Mathieu
Signature · 3 lines
Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
Re: [PATCH 1/2] tracepoints: Add helper to test if tracepoint is enabled in a header
In reply to Mathieu DesnoyersQuoted 39 lines · Mathieu Desnoyers
Quoted 30 lines · ----- On Sep 24, 2020, at 2:30 PM, rostedt [email protected] wrote:
Quoted 18 lines · On Thu, 24 Sep 2020 13:42:25 -0400 (EDT) Mathieu Desnoyers…
Quoted 18 lines
Signed-off-by Steven Rostedt (VMware) <[email protected]>---
Documentation/trace/tracepoints.rst| 25 ++++++++++++++++++++++include/linux/tracepoint-defs.h| 33 +++++++++++++++++++++++++++++2 files changed, 58 insertions(+)Documentation/trace/tracepoints.rst +3
-0@@ -146,3 +146,28 @@ with jump labels and avoid conditional branches. define tracepoints. Check http://lwn.net/Articles/379903, http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362 for a series of articles with more details. + +If you require calling a tracepoint from a header file, it is not +recommended to call one directly or to use the trace_<tracepoint>_enabled()Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 154
Quoted 3 lines
Documentation/trace/tracepoints.rst +3
-0+function call, as tracepoints in header files can have side effects if a +header is included from a file that has CREATE_TRACE_POINTS set. Instead, +include tracepoint-defs.h and use trace_enabled().Tracepoints per-se have no issues being used from header files. The
TRACE_EVENTinfrastructure seems to be the cause of this problem. We should fix trace events rather than require all users to use weird work-arounds thorough the kernel code base.Quoted 3 lines
That's like saying "let's solve include hell". Note, in the past there has also been issues with the headers included also having issues including other headers and cause a dependency loop.
Quoted 5 lines
AFAIU, there are a few scenarios we care about here:
1) Includes done by
tracepoint.h(directly and indirectly). Some additional includes may have crept in and bloatedtracepoint.hsince its original implementation. We should identify and fix those.
I was recalling that tracepoint.h transiently included linux/mm.h, but that appears to not be the case. Either I'm misremembering, or I at some point fixed the issue by switching to forward declarations instead.
Quoted 107 lines
2) Includes done by trace event headers when
CREATE_TRACE_POINTSis defined.define_trace.halready takes care of #undef/#defineCREATE_TRACE_POINTSto prevent recursion, so it should not be an issue.3) Includes done by a compile unit after #define
CREATE_TRACE_POINTS, but which are not meant to create trace point probes. For this case, requiring to #undefCREATE_TRACE_POINTSafter all the relevant headers are included should solve it.So what additional issues am I missing here ?
Quoted 3 lines
But the magic of trace events (for both perf and ftrace, sorry if you refused to use it),
I cannot not use it to this day because changes I needed to upstream in order to make it useful to me were rejected.
Quoted 4 lines
is that people who add tracepoints do not need to know
how tracepoints work. There's no adding of registering of them, or anything else. The formats and everything they record appear in the tracefs file system.That's all fine by me.
Quoted 3 lines
How are your trace events created? All manual, or do you have helper macros.
I suspect you mean LTTng's trace events. Those are created with helper macros (
LTTNG_TRACEPOINT_EVENT) which have a few improvements overTRACE_EVENT, namely:- Ability to create arrays of events (because lots of semicolons are removed), removing the need to dynamically register each event at init time,
- Ability to do pre-filtering of events, before they hit the ring buffer, allowed by combiningTP_structandTP_fast_assigninto a single structuredTP_FIELDS.LTTng also has an include pass which uses
TRACE_EVENTfrom the kernel to perform tracepoint prototype signature validation ofLTTNG_TRACEPOINT_EVENTagainstTRACE_EVENTprototypes.Quoted 1 line
Would these be safe if a bunch were included?
Can you elaborate on this question ? I have a hard time figuring out what scenario(s) you are after.
Quoted 8 lines
Quoted 5 lines
I am not against the idea of a
tracepoint_enabled(tp), but I am against the motivation behind this patch and the new tracepoint user requirements it documents.It removes the open coded code that has been in the kernel for the last 4 years.
There are indeed many cases where a
tracepoint_enabled()macro makes sense. In some situations we encounter in user-space for lttng-ust, there is need to prepare data before it is passed as tracepoint arguments. Having this "enabled" macros allows to only prepare the data when needed.Quoted 22 lines
Quoted 13 lines
Quoted 13 lines
Documentation/trace/tracepoints.rst +11
-0+ +In a C file:: + + void do_trace_foo_bar_wrapper(args) + { + trace_foo_bar(args); + } + +In the header file:: + + DECLEARE_TRACEPOINT(foo_bar);Quoted 8 lines
Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 170
Quoted 5 lines
Documentation/trace/tracepoints.rst +5
-0+ + static inline void some_inline_function() + { + [..] + if (trace_enabled(foo_bar))Is it
trace_enabled()ortracepoint_enabled()? There is a mismatch between the commit message/code and the documentation.Quoted 8 lines
Yes, it should be
tracepoint_enabled(). Thanks for catching that.Anyway, this shouldn't affect you in any way, as it's just adding wrappers around locations that have been doing this for years.
If you want, I can change the name to
trace_event_enabled()and put the code intrace_events-defs.hinstead. Which would simply includetracepoints-defs.hand have the exact same code.I'm ok with
tracepoint_enabled(). However, I would have placed it intracepoint.hrather thantracepoint-defs.h, and we should figure out why people complain thattracepoint.his including headers too eagerly.Thanks,
Mathieu
Quoted 2 lines
-- Steve
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
Steven Rostedt
maintainerPATCH 2/2 mm/page_ref: Convert the open coded tracepoint enabled to the new helper
In reply to Steven RostedtFrom: "Steven Rostedt (VMware)" <[email protected]>
As more use cases of checking if a tracepoint is enabled in a header are coming to fruition, a helper macro has been added a static_branch to check if a tracepoint is enabled in a header or not. Convert the page_ref logic over to the helper macro.
--- include/linux/page_ref.h | 42 ++++++++++++++++++++-------------------- 1 file changed, 21 insertions(+), 21 deletions(-)
include/linux/page_ref.h +21 -21
@@ -7,13 +7,13 @@#include <linux/page-flags.h> #include <linux/tracepoint-defs.h> -extern struct tracepoint __tracepoint_page_ref_set; -extern struct tracepoint __tracepoint_page_ref_mod; -extern struct tracepoint __tracepoint_page_ref_mod_and_test; -extern struct tracepoint __tracepoint_page_ref_mod_and_return; -extern struct tracepoint __tracepoint_page_ref_mod_unless; -extern struct tracepoint __tracepoint_page_ref_freeze; -extern struct tracepoint __tracepoint_page_ref_unfreeze; +DECLARE_TRACEPOINT (page_ref_set); +DECLARE_TRACEPOINT (page_ref_mod); +DECLARE_TRACEPOINT (page_ref_mod_and_test); +DECLARE_TRACEPOINT (page_ref_mod_and_return); +DECLARE_TRACEPOINT (page_ref_mod_unless); +DECLARE_TRACEPOINT (page_ref_freeze); +DECLARE_TRACEPOINT (page_ref_unfreeze);#ifdef CONFIG_DEBUG_PAGE_REF
@@ -24,7 +24,7 @@ extern struct tracepoint __tracepoint_page_ref_unfreeze;* * See trace_##name##_enabled(void) in include/linux/tracepoint.h */ -#define page_ref_tracepoint_active (t) static_key_false(&(t).key) +#define page_ref_tracepoint_active (t) tracepoint_enabled(t)extern void __page_ref_set (struct page *page,int v);extern void __page_ref_mod (struct page *page,int v);
@@ -75,7 +75,7 @@ static inline int page_count(struct page *page)static inline void set_page_count (struct page *page,int v) {atomic_set (&page->_refcount, v); -if (page_ref_tracepoint_active (__tracepoint_page_ref_set)) +if (page_ref_tracepoint_active (page_ref_set))__page_ref_set (page, v); }
@@ -91,14 +91,14 @@ static inline void init_page_count(struct page *page)static inline void page_ref_add (struct page *page,int nr) {atomic_add (nr, &page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod)) +if (page_ref_tracepoint_active (page_ref_mod))__page_ref_mod (page, nr); }static inline void page_ref_sub (struct page *page,int nr) {atomic_sub (nr, &page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod)) +if (page_ref_tracepoint_active (page_ref_mod))__page_ref_mod (page, -nr); }
@@ -106,7 +106,7 @@ static inline int page_ref_sub_return(struct page *page, int nr) {int ret =atomic_sub_return (nr, &page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod_and_return)) +if (page_ref_tracepoint_active (page_ref_mod_and_return))__page_ref_mod_and_return (page, -nr, ret);return ret; }
@@ -114,14 +114,14 @@ static inline int page_ref_sub_return(struct page *page, int nr)static inline void page_ref_inc (struct page *page) {atomic_inc (&page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod)) +if (page_ref_tracepoint_active (page_ref_mod))__page_ref_mod (page,1 ); }static inline void page_ref_dec (struct page *page) {atomic_dec (&page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod)) +if (page_ref_tracepoint_active (page_ref_mod))__page_ref_mod (page,-1 ); }
@@ -129,7 +129,7 @@ static inline int page_ref_sub_and_test(struct page *page, int nr) {int ret =atomic_sub_and_test (nr, &page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod_and_test)) +if (page_ref_tracepoint_active (page_ref_mod_and_test))__page_ref_mod_and_test (page, -nr, ret);return ret; }
@@ -138,7 +138,7 @@ static inline int page_ref_inc_return(struct page *page) {int ret =atomic_inc_return (&page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod_and_return)) +if (page_ref_tracepoint_active (page_ref_mod_and_return))__page_ref_mod_and_return (page,1 , ret);return ret; }
@@ -147,7 +147,7 @@ static inline int page_ref_dec_and_test(struct page *page) {int ret =atomic_dec_and_test (&page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod_and_test)) +if (page_ref_tracepoint_active (page_ref_mod_and_test))__page_ref_mod_and_test (page,-1 , ret);return ret; }
@@ -156,7 +156,7 @@ static inline int page_ref_dec_return(struct page *page) {int ret =atomic_dec_return (&page->_refcount); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod_and_return)) +if (page_ref_tracepoint_active (page_ref_mod_and_return))__page_ref_mod_and_return (page,-1 , ret);return ret; }
@@ -165,7 +165,7 @@ static inline int page_ref_add_unless(struct page *page, int nr, int u) {int ret =atomic_add_unless (&page->_refcount, nr, u); -if (page_ref_tracepoint_active (__tracepoint_page_ref_mod_unless)) +if (page_ref_tracepoint_active (page_ref_mod_unless))__page_ref_mod_unless (page, nr, ret);return ret; }
@@ -174,7 +174,7 @@ static inline int page_ref_freeze(struct page *page, int count) {int ret =likely (atomic_cmpxchg (&page->_refcount, count,0 ) == count); -if (page_ref_tracepoint_active (__tracepoint_page_ref_freeze)) +if (page_ref_tracepoint_active (page_ref_freeze))__page_ref_freeze (page, count, ret);return ret; }
@@ -185,7 +185,7 @@ static inline void page_ref_unfreeze(struct page *page, int count)VM_BUG_ON (count ==0 );atomic_set_release (&page->_refcount, count); -if (page_ref_tracepoint_active (__tracepoint_page_ref_unfreeze)) +if (page_ref_tracepoint_active (page_ref_unfreeze))__page_ref_unfreeze (page, count); }
Signature · 1 line
2.28.0