tracing/mm: Add tracepoint_enabled() helper function for headers

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, as well as bloat the kernel as the trace_<tracepoint>() function is not a small inline, 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 this overhead, a static_branch can be used to only have the tracepoint wrapper get called when the tracepoint is enabled.

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.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
 Documentation/trace/tracepoints.rst | 27 +++++++++++++++++++++++
 include/linux/tracepoint-defs.h     | 34 +++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

Documentation/trace/tracepoints.rst +9 -0

index 6e3ce3bf3593..68579ebd1e4c 100644
@@ -146,3 +146,30 @@ 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, as
+well as the trace_<tracepoint>() is not that small of an inline
+and can bloat the kernel if used by other inlined functions. Instead,
+include tracepoint-defs.h and use tracepoint_enabled().
+

Documentation/trace/tracepoints.rst +8 -0

+
+       static inline void some_inline_function()
+       {
+               [..]
+               if (tracepoint_enabled(foo_bar))
+                       do_trace_foo_bar_wrapper(args);
+               [..]
+       }

include/linux/tracepoint-defs.h +34 -1

index b29950a19205..60625973faaf 100644
@@ -48,4 +48,38 @@ 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 and bloat the kernel. Instead, use
+ * tracepoint_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_bar()", you would need to create a wrapper function
+ * in a C file to call trace_foo_bar():
+ *   void do_trace_foo_bar(args) { trace_foo_bar(args); }
+ * Then in the header file, declare the tracepoint:
+ *   DECLARE_TRACEPOINT(foo_bar);
+ * And call your wrapper:
+ *   static inline void some_inlined_function() {
+ *            [..]
+ *            if (tracepoint_enabled(foo_bar))
+ *                    do_trace_foo_bar(args);
+ *            [..]
+ *   }
+ *
+ * Note: tracepoint_enabled(foo_bar) is equivalent to trace_foo_bar_enabled()
+ *   but is safe to have in headers, where trace_foo_bar_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

Quoted 4 lines

Documentation/trace/tracepoints.rst +4 -0

+In a C file::
+
+       void do_trace_foo_bar_wrapper(args)
+       {

Documentation/trace/tracepoints.rst with jump labels and avoid conditional branches. · line 167

Quoted 6 lines

Documentation/trace/tracepoints.rst +6 -0

+               trace_foo_bar(args);
+       }
+
+In the header file::
+
+       DECLEARE_TRACEPOINT(foo_bar);  

Should be "DECLARE_..."

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, as well as bloat the kernel as the trace_<tracepoint>() function is not a small inline, 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 this overhead, a static_branch can be used to only have the tracepoint wrapper get called when the tracepoint is enabled.

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.

Signed-off-by Steven Rostedt (VMware) <[email protected]>

Nice! I'm late here, but you mentioned a v3, so FWIW:

Acked-by Vlastimil Babka <[email protected]>

From: "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, tracepoint_enabled(), has been added to check if a tracepoint is enabled or not, and can be used with minimal header requirements (avoid "include hell"). Convert the page_ref logic over to the new helper macro.

Cc Joonsoo Kim <[email protected]>

Cc Michal Nazarewicz <[email protected]>

Cc Vlastimil Babka <[email protected]>

Cc Minchan Kim <[email protected]>

Cc Mel Gorman <[email protected]>

Cc "Kirill A. Shutemov" <[email protected]>

Cc Sergey Senozhatsky <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Steven Rostedt (VMware) <[email protected]>

---
  include/linux/page_ref.h | 42 ++++++++++++++++++++--------------------
  1 file changed, 21 insertions(+), 21 deletions(-)

include/linux/page_ref.h +0 -6

index d27701199a4d..f3318f34fc54 100644
@@ -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;

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. Not to mention it also will bloat the code being in commonly used inline functions.

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 not 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") commit 7f47d8cc039f ("x86, tracing, perf: Add trace point for MSR accesses")

does this (back in 2015). 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.

Link https://lore.kernel.org/r/[email protected] open here

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, and third converts over msr.h.

Steven Rostedt (VMware) (3):
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
x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it

----

Changes since v1 (https://lore.kernel.org/r/[email protected]):

- Fixed using "trace_enabled()" instead of "tracepoint_enabled()" (Mathieu Desnoyers reported)

- Reworded to include comments about bloating the kernel when tracepoints are used in commonly used inlined functions.

- Added the msr update as well.

 Documentation/trace/tracepoints.rst | 27 ++++++++++++++++++++++++
 arch/x86/include/asm/msr.h          | 18 +++++++---------
 include/linux/page_ref.h            | 42 ++++++++++++++++++-------------------
 include/linux/tracepoint-defs.h     | 34 ++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 31 deletions(-)

Bah, My cut-and-paste of my "quilt mail --send" chopped off Mathieu's email.

Mathieu, I didn't meant to not Cc you on this. Do you need me to bounce the rest to you or you can get it from lore?

-- Steve

Quoted 58 lines · On Fri, 25 Sep 2020 17:12:06 -0400 Steven Rostedt <[email protected]> wrote:

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. Not to mention it also will bloat the code being in commonly used inline functions.

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 not 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") commit 7f47d8cc039f ("x86, tracing, perf: Add trace point for MSR accesses")

does this (back in 2015). 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.

Link https://lore.kernel.org/r/[email protected] open here

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, and third converts over msr.h.

Steven Rostedt (VMware) (3):
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
x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it

----

Changes since v1 (https://lore.kernel.org/r/[email protected]):

- Fixed using "trace_enabled()" instead of "tracepoint_enabled()" (Mathieu Desnoyers reported)

- Reworded to include comments about bloating the kernel when tracepoints are used in commonly used inlined functions.

- Added the msr update as well.

 Documentation/trace/tracepoints.rst | 27 ++++++++++++++++++++++++
 arch/x86/include/asm/msr.h          | 18 +++++++---------
 include/linux/page_ref.h            | 42 ++++++++++++++++++-------------------
 include/linux/tracepoint-defs.h     | 34 ++++++++++++++++++++++++++++++
 4 files changed, 90 insertions(+), 31 deletions(-)