[PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers

[PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers

[Steven Rostedt]

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/20200922125113.12ef1e03@gandalf.local.home

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/20200924170928.466191266@goodmis.org):

 - 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(-)

[PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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) <rostedt@goodmis.org>
---
 Documentation/trace/tracepoints.rst | 27 +++++++++++++++++++++++
 include/linux/tracepoint-defs.h     | 34 +++++++++++++++++++++++++++++
 2 files changed, 61 insertions(+)

diff --git a/Documentation/trace/tracepoints.rst b/Documentation/trace/tracepoints.rst
index 6e3ce3bf3593..68579ebd1e4c 100644
--- a/Documentation/trace/tracepoints.rst
+++ b/Documentation/trace/tracepoints.rst
@@ -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().
+
+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 (tracepoint_enabled(foo_bar))
+			do_trace_foo_bar_wrapper(args);
+		[..]
+	}
diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
index b29950a19205..60625973faaf 100644
--- a/include/linux/tracepoint-defs.h
+++ b/include/linux/tracepoint-defs.h
@@ -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

[PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

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 <iamjoonsoo.kim@lge.com>
Cc: Michal Nazarewicz <mina86@mina86.com>
Cc: Vlastimil Babka <vbabka@suse.cz>
Cc: Minchan Kim <minchan@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
Cc: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 include/linux/page_ref.h | 42 ++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/include/linux/page_ref.h b/include/linux/page_ref.h
index d27701199a4d..f3318f34fc54 100644
--- a/include/linux/page_ref.h
+++ b/include/linux/page_ref.h
@@ -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);
 }
 
-- 
2.28.0

[PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it

[Steven Rostedt]

From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>

7f47d8cc039f ("x86, tracing, perf: Add trace point for MSR accesses") added
tracing of msr read and write, but because of complexity in having
tracepoints in headers, and even more so for a core header like msr.h, not
to mention the bloat a tracepoint adds to inline functions, a helper
function is needed to be called from the header.

Use the new tracepoint_enabled() macro in tracepoint-defs.h to test if the
tracepoint is active before calling the helper function, instead of open
coding the same logic, which requires knowing the internals of a tracepoint.

Cc: Andi Kleen <ak@linux.intel.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
---
 arch/x86/include/asm/msr.h | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/arch/x86/include/asm/msr.h b/arch/x86/include/asm/msr.h
index 86f20d520a07..fd4a32f0947b 100644
--- a/arch/x86/include/asm/msr.h
+++ b/arch/x86/include/asm/msr.h
@@ -67,15 +67,13 @@ struct saved_msrs {
 #include <asm/atomic.h>
 #include <linux/tracepoint-defs.h>
 
-extern struct tracepoint __tracepoint_read_msr;
-extern struct tracepoint __tracepoint_write_msr;
-extern struct tracepoint __tracepoint_rdpmc;
-#define msr_tracepoint_active(t) static_key_false(&(t).key)
+DECLARE_TRACEPOINT(read_msr);
+DECLARE_TRACEPOINT(write_msr);
+DECLARE_TRACEPOINT(rdpmc);
 extern void do_trace_write_msr(unsigned int msr, u64 val, int failed);
 extern void do_trace_read_msr(unsigned int msr, u64 val, int failed);
 extern void do_trace_rdpmc(unsigned int msr, u64 val, int failed);
 #else
-#define msr_tracepoint_active(t) false
 static inline void do_trace_write_msr(unsigned int msr, u64 val, int failed) {}
 static inline void do_trace_read_msr(unsigned int msr, u64 val, int failed) {}
 static inline void do_trace_rdpmc(unsigned int msr, u64 val, int failed) {}
@@ -128,7 +126,7 @@ static inline unsigned long long native_read_msr(unsigned int msr)
 
 	val = __rdmsr(msr);
 
-	if (msr_tracepoint_active(__tracepoint_read_msr))
+	if (tracepoint_enabled(read_msr))
 		do_trace_read_msr(msr, val, 0);
 
 	return val;
@@ -150,7 +148,7 @@ static inline unsigned long long native_read_msr_safe(unsigned int msr,
 		     _ASM_EXTABLE(2b, 3b)
 		     : [err] "=r" (*err), EAX_EDX_RET(val, low, high)
 		     : "c" (msr), [fault] "i" (-EIO));
-	if (msr_tracepoint_active(__tracepoint_read_msr))
+	if (tracepoint_enabled(read_msr))
 		do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), *err);
 	return EAX_EDX_VAL(val, low, high);
 }
@@ -161,7 +159,7 @@ native_write_msr(unsigned int msr, u32 low, u32 high)
 {
 	__wrmsr(msr, low, high);
 
-	if (msr_tracepoint_active(__tracepoint_write_msr))
+	if (tracepoint_enabled(write_msr))
 		do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
 }
 
@@ -181,7 +179,7 @@ native_write_msr_safe(unsigned int msr, u32 low, u32 high)
 		     : "c" (msr), "0" (low), "d" (high),
 		       [fault] "i" (-EIO)
 		     : "memory");
-	if (msr_tracepoint_active(__tracepoint_write_msr))
+	if (tracepoint_enabled(write_msr))
 		do_trace_write_msr(msr, ((u64)high << 32 | low), err);
 	return err;
 }
@@ -248,7 +246,7 @@ static inline unsigned long long native_read_pmc(int counter)
 	DECLARE_ARGS(val, low, high);
 
 	asm volatile("rdpmc" : EAX_EDX_RET(val, low, high) : "c" (counter));
-	if (msr_tracepoint_active(__tracepoint_rdpmc))
+	if (tracepoint_enabled(rdpmc))
 		do_trace_rdpmc(counter, EAX_EDX_VAL(val, low, high), 0);
 	return EAX_EDX_VAL(val, low, high);
 }
-- 
2.28.0

Re: [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers

[Steven Rostedt]

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


On Fri, 25 Sep 2020 17:12:06 -0400
Steven Rostedt <rostedt@goodmis.org> 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/20200922125113.12ef1e03@gandalf.local.home
> 
> 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/20200924170928.466191266@goodmis.org):
> 
>  - 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(-)

Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header

[Axel Rasmussen]

On Fri, Sep 25, 2020 at 2:18 PM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
>
> 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) <rostedt@goodmis.org>
> ---
>  Documentation/trace/tracepoints.rst | 27 +++++++++++++++++++++++
>  include/linux/tracepoint-defs.h     | 34 +++++++++++++++++++++++++++++
>  2 files changed, 61 insertions(+)
>
> diff --git a/Documentation/trace/tracepoints.rst b/Documentation/trace/tracepoints.rst
> index 6e3ce3bf3593..68579ebd1e4c 100644
> --- a/Documentation/trace/tracepoints.rst
> +++ b/Documentation/trace/tracepoints.rst
> @@ -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().
> +
> +In a C file::
> +
> +       void do_trace_foo_bar_wrapper(args)
> +       {
> +               trace_foo_bar(args);
> +       }
> +
> +In the header file::
> +
> +       DECLEARE_TRACEPOINT(foo_bar);

Should be "DECLARE_..."

> +
> +       static inline void some_inline_function()
> +       {
> +               [..]
> +               if (tracepoint_enabled(foo_bar))
> +                       do_trace_foo_bar_wrapper(args);
> +               [..]
> +       }
> diff --git a/include/linux/tracepoint-defs.h b/include/linux/tracepoint-defs.h
> index b29950a19205..60625973faaf 100644
> --- a/include/linux/tracepoint-defs.h
> +++ b/include/linux/tracepoint-defs.h
> @@ -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
>
>

Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header

[Steven Rostedt]

On Fri, 25 Sep 2020 14:36:37 -0700
Axel Rasmussen <axelrasmussen@google.com> wrote:

> > +In a C file::
> > +
> > +       void do_trace_foo_bar_wrapper(args)
> > +       {
> > +               trace_foo_bar(args);
> > +       }
> > +
> > +In the header file::
> > +
> > +       DECLEARE_TRACEPOINT(foo_bar);  
> 
> Should be "DECLARE_..."

But that's the British spelling!

OK, I'll go ahead and fix that in v3.

-- Steve

Re: [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers

[Mathieu Desnoyers]

No worries, I'll get it from lore.

Thanks,

Mathieu

----- Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> 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
> 
> 
> On Fri, 25 Sep 2020 17:12:06 -0400
> Steven Rostedt <rostedt@goodmis.org> 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/20200922125113.12ef1e03@gandalf.local.home
> > 
> > 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/20200924170928.466191266@goodmis.org):
> > 
> >  - 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(-)
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 10133 bytes --]

Hi Steven,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on tip/master]
[also build test ERROR on linux/master tip/perf/core tip/x86/core linus/master v5.9-rc6 next-20200925]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Steven-Rostedt/tracing-mm-Add-tracepoint_enabled-helper-function-for-headers/20200926-051950
base:   https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git 0248dedd12d43035bf53c326633f0610a49d7134
config: x86_64-randconfig-a003-20200925 (attached as .config)
compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project a83eb048cb9a75da7a07a9d5318bbdbf54885c87)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # https://github.com/0day-ci/linux/commit/7a9f7773ebb9b2d4be989415cfa2cee5788201ea
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Steven-Rostedt/tracing-mm-Add-tracepoint_enabled-helper-function-for-headers/20200926-051950
        git checkout 7a9f7773ebb9b2d4be989415cfa2cee5788201ea
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from arch/x86/kernel/asm-offsets.c:9:
   In file included from include/linux/crypto.h:20:
   In file included from include/linux/slab.h:15:
   In file included from include/linux/gfp.h:6:
   In file included from include/linux/mmzone.h:8:
   In file included from include/linux/spinlock.h:51:
   In file included from include/linux/preempt.h:78:
   In file included from arch/x86/include/asm/preempt.h:7:
   In file included from include/linux/thread_info.h:38:
   In file included from arch/x86/include/asm/thread_info.h:53:
   In file included from arch/x86/include/asm/cpufeature.h:5:
   In file included from arch/x86/include/asm/processor.h:22:
>> arch/x86/include/asm/msr.h:129:6: error: implicit declaration of function 'tracepoint_enabled' [-Werror,-Wimplicit-function-declaration]
           if (tracepoint_enabled(read_msr))
               ^
>> arch/x86/include/asm/msr.h:129:25: error: use of undeclared identifier 'read_msr'
           if (tracepoint_enabled(read_msr))
                                  ^
   arch/x86/include/asm/msr.h:151:6: error: implicit declaration of function 'tracepoint_enabled' [-Werror,-Wimplicit-function-declaration]
           if (tracepoint_enabled(read_msr))
               ^
   arch/x86/include/asm/msr.h:151:25: error: use of undeclared identifier 'read_msr'
           if (tracepoint_enabled(read_msr))
                                  ^
   arch/x86/include/asm/msr.h:162:6: error: implicit declaration of function 'tracepoint_enabled' [-Werror,-Wimplicit-function-declaration]
           if (tracepoint_enabled(write_msr))
               ^
>> arch/x86/include/asm/msr.h:162:25: error: use of undeclared identifier 'write_msr'
           if (tracepoint_enabled(write_msr))
                                  ^
   arch/x86/include/asm/msr.h:182:6: error: implicit declaration of function 'tracepoint_enabled' [-Werror,-Wimplicit-function-declaration]
           if (tracepoint_enabled(write_msr))
               ^
   arch/x86/include/asm/msr.h:182:25: error: use of undeclared identifier 'write_msr'
           if (tracepoint_enabled(write_msr))
                                  ^
   arch/x86/include/asm/msr.h:249:6: error: implicit declaration of function 'tracepoint_enabled' [-Werror,-Wimplicit-function-declaration]
           if (tracepoint_enabled(rdpmc))
               ^
>> arch/x86/include/asm/msr.h:249:25: error: use of undeclared identifier 'rdpmc'; did you mean 'rdtsc'?
           if (tracepoint_enabled(rdpmc))
                                  ^~~~~
                                  rdtsc
   arch/x86/include/asm/msr.h:199:43: note: 'rdtsc' declared here
   static __always_inline unsigned long long rdtsc(void)
                                             ^
   10 errors generated.
   make[2]: *** [scripts/Makefile.build:117: arch/x86/kernel/asm-offsets.s] Error 1
   make[2]: Target '__build' not remade because of errors.
   make[1]: *** [Makefile:1198: prepare0] Error 2
   make[1]: Target 'prepare' not remade because of errors.
   make: *** [Makefile:185: __sub-make] Error 2
   make: Target 'prepare' not remade because of errors.

vim +/tracepoint_enabled +129 arch/x86/include/asm/msr.h

   115	
   116	#define native_wrmsr(msr, low, high)			\
   117		__wrmsr(msr, low, high)
   118	
   119	#define native_wrmsrl(msr, val)				\
   120		__wrmsr((msr), (u32)((u64)(val)),		\
   121			       (u32)((u64)(val) >> 32))
   122	
   123	static inline unsigned long long native_read_msr(unsigned int msr)
   124	{
   125		unsigned long long val;
   126	
   127		val = __rdmsr(msr);
   128	
 > 129		if (tracepoint_enabled(read_msr))
   130			do_trace_read_msr(msr, val, 0);
   131	
   132		return val;
   133	}
   134	
   135	static inline unsigned long long native_read_msr_safe(unsigned int msr,
   136							      int *err)
   137	{
   138		DECLARE_ARGS(val, low, high);
   139	
   140		asm volatile("2: rdmsr ; xor %[err],%[err]\n"
   141			     "1:\n\t"
   142			     ".section .fixup,\"ax\"\n\t"
   143			     "3: mov %[fault],%[err]\n\t"
   144			     "xorl %%eax, %%eax\n\t"
   145			     "xorl %%edx, %%edx\n\t"
   146			     "jmp 1b\n\t"
   147			     ".previous\n\t"
   148			     _ASM_EXTABLE(2b, 3b)
   149			     : [err] "=r" (*err), EAX_EDX_RET(val, low, high)
   150			     : "c" (msr), [fault] "i" (-EIO));
   151		if (tracepoint_enabled(read_msr))
   152			do_trace_read_msr(msr, EAX_EDX_VAL(val, low, high), *err);
   153		return EAX_EDX_VAL(val, low, high);
   154	}
   155	
   156	/* Can be uninlined because referenced by paravirt */
   157	static inline void notrace
   158	native_write_msr(unsigned int msr, u32 low, u32 high)
   159	{
   160		__wrmsr(msr, low, high);
   161	
 > 162		if (tracepoint_enabled(write_msr))
   163			do_trace_write_msr(msr, ((u64)high << 32 | low), 0);
   164	}
   165	
   166	/* Can be uninlined because referenced by paravirt */
   167	static inline int notrace
   168	native_write_msr_safe(unsigned int msr, u32 low, u32 high)
   169	{
   170		int err;
   171	
   172		asm volatile("2: wrmsr ; xor %[err],%[err]\n"
   173			     "1:\n\t"
   174			     ".section .fixup,\"ax\"\n\t"
   175			     "3:  mov %[fault],%[err] ; jmp 1b\n\t"
   176			     ".previous\n\t"
   177			     _ASM_EXTABLE(2b, 3b)
   178			     : [err] "=a" (err)
   179			     : "c" (msr), "0" (low), "d" (high),
   180			       [fault] "i" (-EIO)
   181			     : "memory");
   182		if (tracepoint_enabled(write_msr))
   183			do_trace_write_msr(msr, ((u64)high << 32 | low), err);
   184		return err;
   185	}
   186	
   187	extern int rdmsr_safe_regs(u32 regs[8]);
   188	extern int wrmsr_safe_regs(u32 regs[8]);
   189	
   190	/**
   191	 * rdtsc() - returns the current TSC without ordering constraints
   192	 *
   193	 * rdtsc() returns the result of RDTSC as a 64-bit integer.  The
   194	 * only ordering constraint it supplies is the ordering implied by
   195	 * "asm volatile": it will put the RDTSC in the place you expect.  The
   196	 * CPU can and will speculatively execute that RDTSC, though, so the
   197	 * results can be non-monotonic if compared on different CPUs.
   198	 */
   199	static __always_inline unsigned long long rdtsc(void)
   200	{
   201		DECLARE_ARGS(val, low, high);
   202	
   203		asm volatile("rdtsc" : EAX_EDX_RET(val, low, high));
   204	
   205		return EAX_EDX_VAL(val, low, high);
   206	}
   207	
   208	/**
   209	 * rdtsc_ordered() - read the current TSC in program order
   210	 *
   211	 * rdtsc_ordered() returns the result of RDTSC as a 64-bit integer.
   212	 * It is ordered like a load to a global in-memory counter.  It should
   213	 * be impossible to observe non-monotonic rdtsc_unordered() behavior
   214	 * across multiple CPUs as long as the TSC is synced.
   215	 */
   216	static __always_inline unsigned long long rdtsc_ordered(void)
   217	{
   218		DECLARE_ARGS(val, low, high);
   219	
   220		/*
   221		 * The RDTSC instruction is not ordered relative to memory
   222		 * access.  The Intel SDM and the AMD APM are both vague on this
   223		 * point, but empirically an RDTSC instruction can be
   224		 * speculatively executed before prior loads.  An RDTSC
   225		 * immediately after an appropriate barrier appears to be
   226		 * ordered as a normal load, that is, it provides the same
   227		 * ordering guarantees as reading from a global memory location
   228		 * that some other imaginary CPU is updating continuously with a
   229		 * time stamp.
   230		 *
   231		 * Thus, use the preferred barrier on the respective CPU, aiming for
   232		 * RDTSCP as the default.
   233		 */
   234		asm volatile(ALTERNATIVE_2("rdtsc",
   235					   "lfence; rdtsc", X86_FEATURE_LFENCE_RDTSC,
   236					   "rdtscp", X86_FEATURE_RDTSCP)
   237				: EAX_EDX_RET(val, low, high)
   238				/* RDTSCP clobbers ECX with MSR_TSC_AUX. */
   239				:: "ecx");
   240	
   241		return EAX_EDX_VAL(val, low, high);
   242	}
   243	
   244	static inline unsigned long long native_read_pmc(int counter)
   245	{
   246		DECLARE_ARGS(val, low, high);
   247	
   248		asm volatile("rdpmc" : EAX_EDX_RET(val, low, high) : "c" (counter));
 > 249		if (tracepoint_enabled(rdpmc))
   250			do_trace_rdpmc(counter, EAX_EDX_VAL(val, low, high), 0);
   251		return EAX_EDX_VAL(val, low, high);
   252	}
   253	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29309 bytes --]

Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header

[Vlastimil Babka]

On 9/25/20 11:12 PM, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 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) <rostedt@goodmis.org>

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

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Re: [PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper

[Vlastimil Babka]

On 9/25/20 11:12 PM, Steven Rostedt wrote:
> From: "Steven Rostedt (VMware)" <rostedt@goodmis.org>
> 
> 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 <iamjoonsoo.kim@lge.com>
> Cc: Michal Nazarewicz <mina86@mina86.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>
> Cc: Minchan Kim <minchan@kernel.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
> Cc: Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
> ---
>   include/linux/page_ref.h | 42 ++++++++++++++++++++--------------------
>   1 file changed, 21 insertions(+), 21 deletions(-)
> 
> diff --git a/include/linux/page_ref.h b/include/linux/page_ref.h
> index d27701199a4d..f3318f34fc54 100644
> --- a/include/linux/page_ref.h
> +++ b/include/linux/page_ref.h
> @@ -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

Not that it matters much, but the declarations could be below the #ifdef, AFAICS.

>   
> @@ -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)

The full comment above could now be deleted too?

Acked-by: Vlastimil Babka <vbabka@suse.cz>

Thanks!

Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header

[Steven Rostedt]

On Tue, 20 Oct 2020 13:59:46 +0200
Vlastimil Babka <vbabka@suse.cz> wrote:

> Nice! I'm late here, but you mentioned a v3, so FWIW:
> 
> Acked-by: Vlastimil Babka <vbabka@suse.cz>

You are late ;-)

  afbe7973173a7ce0a68af8b33e44c967582297be

-- Steve