* [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers
@ 2020-09-25 21:12 Steven Rostedt
2020-09-25 21:12 ` [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header Steven Rostedt
` (3 more replies)
0 siblings, 4 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-09-25 21:12 UTC (permalink / raw)
To: linux-kernel
Cc: Yafang Shao, Axel Rasmussen, Andrew Morton, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim
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(-)
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header
2020-09-25 21:12 [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
@ 2020-09-25 21:12 ` Steven Rostedt
2020-09-25 21:36 ` Axel Rasmussen
2020-10-20 11:59 ` Vlastimil Babka
2020-09-25 21:12 ` [PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper Steven Rostedt
` (2 subsequent siblings)
3 siblings, 2 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-09-25 21:12 UTC (permalink / raw)
To: linux-kernel
Cc: Yafang Shao, Axel Rasmussen, Andrew Morton, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper
2020-09-25 21:12 [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
2020-09-25 21:12 ` [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header Steven Rostedt
@ 2020-09-25 21:12 ` Steven Rostedt
2020-10-20 12:10 ` Vlastimil Babka
2020-09-25 21:12 ` [PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it Steven Rostedt
2020-09-25 21:26 ` [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
3 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-09-25 21:12 UTC (permalink / raw)
To: linux-kernel
Cc: Yafang Shao, Axel Rasmussen, Andrew Morton, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim, Michal Nazarewicz, Minchan Kim,
Mel Gorman, Kirill A. Shutemov, Sergey Senozhatsky,
Arnd Bergmann
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it
2020-09-25 21:12 [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
2020-09-25 21:12 ` [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header Steven Rostedt
2020-09-25 21:12 ` [PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper Steven Rostedt
@ 2020-09-25 21:12 ` Steven Rostedt
2020-09-26 6:55 ` kernel test robot
2020-09-25 21:26 ` [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
3 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-09-25 21:12 UTC (permalink / raw)
To: linux-kernel
Cc: Yafang Shao, Axel Rasmussen, Andrew Morton, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim, Andi Kleen, Peter Zijlstra,
Thomas Gleixner
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers
2020-09-25 21:12 [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
` (2 preceding siblings ...)
2020-09-25 21:12 ` [PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it Steven Rostedt
@ 2020-09-25 21:26 ` Steven Rostedt
2020-09-25 22:23 ` Mathieu Desnoyers
3 siblings, 1 reply; 12+ messages in thread
From: Steven Rostedt @ 2020-09-25 21:26 UTC (permalink / raw)
To: linux-kernel
Cc: Yafang Shao, Axel Rasmussen, Andrew Morton, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim, Mathieu Desnoyers
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(-)
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header
2020-09-25 21:12 ` [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header Steven Rostedt
@ 2020-09-25 21:36 ` Axel Rasmussen
2020-09-25 21:59 ` Steven Rostedt
2020-10-20 11:59 ` Vlastimil Babka
1 sibling, 1 reply; 12+ messages in thread
From: Axel Rasmussen @ 2020-09-25 21:36 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Yafang Shao, Andrew Morton, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim
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
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header
2020-09-25 21:36 ` Axel Rasmussen
@ 2020-09-25 21:59 ` Steven Rostedt
0 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-09-25 21:59 UTC (permalink / raw)
To: Axel Rasmussen
Cc: LKML, Yafang Shao, Andrew Morton, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers
2020-09-25 21:26 ` [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
@ 2020-09-25 22:23 ` Mathieu Desnoyers
0 siblings, 0 replies; 12+ messages in thread
From: Mathieu Desnoyers @ 2020-09-25 22:23 UTC (permalink / raw)
To: Steven Rostedt
Cc: linux-kernel, Yafang Shao, Axel Rasmussen, Andrew Morton,
Vlastimil Babka, Michel Lespinasse, Daniel Jordan,
Davidlohr Bueso, Linux MM, Ingo Molnar, Joonsoo Kim
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it
2020-09-25 21:12 ` [PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it Steven Rostedt
@ 2020-09-26 6:55 ` kernel test robot
0 siblings, 0 replies; 12+ messages in thread
From: kernel test robot @ 2020-09-26 6:55 UTC (permalink / raw)
To: Steven Rostedt, linux-kernel
Cc: kbuild-all, clang-built-linux, Yafang Shao, Axel Rasmussen,
Andrew Morton, Linux Memory Management List, Vlastimil Babka,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Ingo Molnar
[-- 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 --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header
2020-09-25 21:12 ` [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header Steven Rostedt
2020-09-25 21:36 ` Axel Rasmussen
@ 2020-10-20 11:59 ` Vlastimil Babka
2020-10-20 20:43 ` Steven Rostedt
1 sibling, 1 reply; 12+ messages in thread
From: Vlastimil Babka @ 2020-10-20 11:59 UTC (permalink / raw)
To: Steven Rostedt, linux-kernel
Cc: Yafang Shao, Axel Rasmussen, Andrew Morton, Michel Lespinasse,
Daniel Jordan, Davidlohr Bueso, Linux MM, Ingo Molnar,
Joonsoo Kim
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>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper
2020-09-25 21:12 ` [PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper Steven Rostedt
@ 2020-10-20 12:10 ` Vlastimil Babka
0 siblings, 0 replies; 12+ messages in thread
From: Vlastimil Babka @ 2020-10-20 12:10 UTC (permalink / raw)
To: Steven Rostedt, linux-kernel
Cc: Yafang Shao, Axel Rasmussen, Andrew Morton, Michel Lespinasse,
Daniel Jordan, Davidlohr Bueso, Linux MM, Ingo Molnar,
Joonsoo Kim, Michal Nazarewicz, Minchan Kim, Mel Gorman,
Kirill A. Shutemov, Sergey Senozhatsky, Arnd Bergmann
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!
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header
2020-10-20 11:59 ` Vlastimil Babka
@ 2020-10-20 20:43 ` Steven Rostedt
0 siblings, 0 replies; 12+ messages in thread
From: Steven Rostedt @ 2020-10-20 20:43 UTC (permalink / raw)
To: Vlastimil Babka
Cc: linux-kernel, Yafang Shao, Axel Rasmussen, Andrew Morton,
Michel Lespinasse, Daniel Jordan, Davidlohr Bueso, Linux MM,
Ingo Molnar, Joonsoo Kim
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2020-10-20 20:43 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-25 21:12 [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
2020-09-25 21:12 ` [PATCH 1/3 v2] tracepoints: Add helper to test if tracepoint is enabled in a header Steven Rostedt
2020-09-25 21:36 ` Axel Rasmussen
2020-09-25 21:59 ` Steven Rostedt
2020-10-20 11:59 ` Vlastimil Babka
2020-10-20 20:43 ` Steven Rostedt
2020-09-25 21:12 ` [PATCH 2/3 v2] mm/page_ref: Convert the open coded tracepoint enabled to the new helper Steven Rostedt
2020-10-20 12:10 ` Vlastimil Babka
2020-09-25 21:12 ` [PATCH 3/3 v2] x86: Use tracepoint_enabled() for msr tracepoints instead of open coding it Steven Rostedt
2020-09-26 6:55 ` kernel test robot
2020-09-25 21:26 ` [PATCH 0/3 v2] tracing/mm: Add tracepoint_enabled() helper function for headers Steven Rostedt
2020-09-25 22:23 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).