* [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
@ 2014-05-06 13:44 Steven Rostedt
2014-05-06 19:35 ` Mathieu Desnoyers
0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2014-05-06 13:44 UTC (permalink / raw)
To: LKML
Cc: Andrew Morton, Mathieu Desnoyers, Javi Merino, David Howells,
Ingo Molnar
There are some code paths in the kernel that need to do some preparations
before it calls a tracepoint. As that code is worthless overhead when
the tracepoint is not enabled, it would be prudent to have that code
only run when the tracepoint is active. To accomplish this, all tracepoints
now get a static inline function called "trace_<tracepoint-name>_enabled()"
which returns true when the tracepoint is enabled and false otherwise.
As an added bonus, that function uses the static_key of the tracepoint
such that no branch is needed.
if (trace_mytracepoint_enabled()) {
arg = process_tp_arg();
trace_mytracepoint(arg);
}
Will keep the "process_tp_arg()" (which may be expensive to run) from
being executed when the tracepoint isn't enabled.
It's best to encapsulate the tracepoint itself in the if statement
just to keep races. For example, if you had:
if (trace_mytracepoint_enabled())
arg = process_tp_arg();
trace_mytracepoint(arg);
There's a chance that the tracepoint could be enabled just after the
if statement, and arg will be undefined when calling the tracepoint.
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
include/linux/tracepoint.h | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 9d30ee4..2e2a5f7 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -185,6 +185,11 @@ extern void syscall_unregfunc(void);
static inline void \
check_trace_callback_type_##name(void (*cb)(data_proto)) \
{ \
+ } \
+ static inline bool \
+ trace_##name##_enabled(void) \
+ { \
+ return static_key_false(&__tracepoint_##name.key); \
}
/*
@@ -230,6 +235,11 @@ extern void syscall_unregfunc(void);
} \
static inline void check_trace_callback_type_##name(void (*cb)(data_proto)) \
{ \
+ } \
+ static inline bool \
+ trace_##name##_enabled(void) \
+ { \
+ return false; \
}
#define DEFINE_TRACE_FN(name, reg, unreg)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
2014-05-06 13:44 [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function Steven Rostedt
@ 2014-05-06 19:35 ` Mathieu Desnoyers
2014-05-06 19:48 ` Steven Rostedt
0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-05-06 19:35 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Andrew Morton, Javi Merino, David Howells, Ingo Molnar
----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "LKML" <linux-kernel@vger.kernel.org>
> Cc: "Andrew Morton" <akpm@linux-foundation.org>, "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, "Javi Merino"
> <javi.merino@arm.com>, "David Howells" <dhowells@redhat.com>, "Ingo Molnar" <mingo@kernel.org>
> Sent: Tuesday, May 6, 2014 9:44:07 AM
> Subject: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
>
>
> There are some code paths in the kernel that need to do some preparations
> before it calls a tracepoint. As that code is worthless overhead when
> the tracepoint is not enabled, it would be prudent to have that code
> only run when the tracepoint is active. To accomplish this, all tracepoints
> now get a static inline function called "trace_<tracepoint-name>_enabled()"
> which returns true when the tracepoint is enabled and false otherwise.
>
> As an added bonus, that function uses the static_key of the tracepoint
> such that no branch is needed.
>
> if (trace_mytracepoint_enabled()) {
> arg = process_tp_arg();
> trace_mytracepoint(arg);
> }
>
> Will keep the "process_tp_arg()" (which may be expensive to run) from
> being executed when the tracepoint isn't enabled.
>
> It's best to encapsulate the tracepoint itself in the if statement
> just to keep races. For example, if you had:
>
> if (trace_mytracepoint_enabled())
> arg = process_tp_arg();
> trace_mytracepoint(arg);
>
> There's a chance that the tracepoint could be enabled just after the
> if statement, and arg will be undefined when calling the tracepoint.
I'm OK with the intend, however there seems to be two means to achieve
this, and I'm not sure the proposed solution is safe.
As you point out just above, the trace_mytracepoint_enabled() construct
can easily lead to incorrect code if users are not very careful on how
they use the condition vs the tracepoint itself.
I understand that the reason why we cannot simply put the call
to "process_tp_arg()" within the parameters passed to trace_mytracepoint()
is because trace_mytracepoint() is a static inline, and that the
side-effects of the arguments it receives need to be evaluated whether
the tracepoint is enabled or not.
To overcome this issue, I have used a layer of macro on top of the
trace_*() call in lttng-ust, giving something similar to this:
#define tracepoint(name, ...) \
do { \
if (static_key_false(&__tracepoint_##name.key) \
trace_##name(__VA_ARGS__); \
} while (0)
and the static inline trace_##name declared by __DECLARE_TRACE
simply contains __DO_TRACE(&__tracepoint_##name,
TP_PROTO(data_proto),
TP_ARGS(data_args),
TP_CONDITION(cond),,);
This allow calling a tracepoint with:
tracepoint(mytracepoint, process_tp_arg());
making sure that process_tp_arg() will only be evaluated if
the tracepoint is enabled. It also ensures that it's impossible
to create a C construct that will open a race window where a
tracepoint could be called with an unpopulated parameter, such as:
if (trace_mytracepoint_enabled())
arg = process_tp_arg();
trace_mytracepoint(arg);
Thoughts ?
Thanks,
Mathieu
>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
> ---
> include/linux/tracepoint.h | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 9d30ee4..2e2a5f7 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -185,6 +185,11 @@ extern void syscall_unregfunc(void);
> static inline void \
> check_trace_callback_type_##name(void (*cb)(data_proto)) \
> { \
> + } \
> + static inline bool \
> + trace_##name##_enabled(void) \
> + { \
> + return static_key_false(&__tracepoint_##name.key); \
> }
>
> /*
> @@ -230,6 +235,11 @@ extern void syscall_unregfunc(void);
> } \
> static inline void check_trace_callback_type_##name(void (*cb)(data_proto))
> \
> { \
> + } \
> + static inline bool \
> + trace_##name##_enabled(void) \
> + { \
> + return false; \
> }
>
> #define DEFINE_TRACE_FN(name, reg, unreg)
> --
> 1.8.1.4
>
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
2014-05-06 19:35 ` Mathieu Desnoyers
@ 2014-05-06 19:48 ` Steven Rostedt
2014-05-06 20:53 ` Mathieu Desnoyers
0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2014-05-06 19:48 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: LKML, Andrew Morton, Javi Merino, David Howells, Ingo Molnar
On Tue, 6 May 2014 19:35:32 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> I'm OK with the intend, however there seems to be two means to achieve
> this, and I'm not sure the proposed solution is safe.
I do plan on adding more documentation to this to stress that this
should be done like this. But hey, we're kernel developers, we should
be responsible enough to not require the hand holding.
Perhaps change checkpatch to make sure that any use of
trace_tracepoint_enabled() encompasses the tracepoint.
Then again, if arg is initialized to something that the tracepoint can
handle, that would work too.
>
> As you point out just above, the trace_mytracepoint_enabled() construct
> can easily lead to incorrect code if users are not very careful on how
> they use the condition vs the tracepoint itself.
>
> I understand that the reason why we cannot simply put the call
> to "process_tp_arg()" within the parameters passed to trace_mytracepoint()
> is because trace_mytracepoint() is a static inline, and that the
> side-effects of the arguments it receives need to be evaluated whether
> the tracepoint is enabled or not.
>
> To overcome this issue, I have used a layer of macro on top of the
> trace_*() call in lttng-ust, giving something similar to this:
>
> #define tracepoint(name, ...) \
> do { \
> if (static_key_false(&__tracepoint_##name.key) \
> trace_##name(__VA_ARGS__); \
> } while (0)
>
> and the static inline trace_##name declared by __DECLARE_TRACE
> simply contains __DO_TRACE(&__tracepoint_##name,
> TP_PROTO(data_proto),
> TP_ARGS(data_args),
> TP_CONDITION(cond),,);
>
> This allow calling a tracepoint with:
>
> tracepoint(mytracepoint, process_tp_arg());
>
> making sure that process_tp_arg() will only be evaluated if
> the tracepoint is enabled. It also ensures that it's impossible
> to create a C construct that will open a race window where a
> tracepoint could be called with an unpopulated parameter, such as:
>
> if (trace_mytracepoint_enabled())
> arg = process_tp_arg();
> trace_mytracepoint(arg);
>
> Thoughts ?
>
The first time I thought about using this was with David's code, which
does this:
if (static_key_false(&i2c_trace_msg)) {
int i;
for (i = 0; i < ret; i++)
if (msgs[i].flags & I2C_M_RD)
trace_i2c_reply(adap, &msgs[i], i);
trace_i2c_result(adap, i, ret);
}
That would look rather silly in a tracepoint.
-- Steve
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
2014-05-06 19:48 ` Steven Rostedt
@ 2014-05-06 20:53 ` Mathieu Desnoyers
2014-05-06 21:06 ` Steven Rostedt
0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-05-06 20:53 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Andrew Morton, Javi Merino, David Howells, Ingo Molnar
----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "LKML" <linux-kernel@vger.kernel.org>, "Andrew Morton" <akpm@linux-foundation.org>, "Javi Merino"
> <javi.merino@arm.com>, "David Howells" <dhowells@redhat.com>, "Ingo Molnar" <mingo@kernel.org>
> Sent: Tuesday, May 6, 2014 3:48:45 PM
> Subject: Re: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
>
> On Tue, 6 May 2014 19:35:32 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
>
> > I'm OK with the intend, however there seems to be two means to achieve
> > this, and I'm not sure the proposed solution is safe.
>
> I do plan on adding more documentation to this to stress that this
> should be done like this. But hey, we're kernel developers, we should
> be responsible enough to not require the hand holding.
I like your optimism. ;-)
> Perhaps change checkpatch to make sure that any use of
> trace_tracepoint_enabled() encompasses the tracepoint.
>
> Then again, if arg is initialized to something that the tracepoint can
> handle, that would work too.
True.
>
> >
> > As you point out just above, the trace_mytracepoint_enabled() construct
> > can easily lead to incorrect code if users are not very careful on how
> > they use the condition vs the tracepoint itself.
> >
> > I understand that the reason why we cannot simply put the call
> > to "process_tp_arg()" within the parameters passed to trace_mytracepoint()
> > is because trace_mytracepoint() is a static inline, and that the
> > side-effects of the arguments it receives need to be evaluated whether
> > the tracepoint is enabled or not.
> >
> > To overcome this issue, I have used a layer of macro on top of the
> > trace_*() call in lttng-ust, giving something similar to this:
> >
> > #define tracepoint(name, ...) \
> > do { \
> > if (static_key_false(&__tracepoint_##name.key) \
> > trace_##name(__VA_ARGS__); \
> > } while (0)
> >
> > and the static inline trace_##name declared by __DECLARE_TRACE
> > simply contains __DO_TRACE(&__tracepoint_##name,
> > TP_PROTO(data_proto),
> > TP_ARGS(data_args),
> > TP_CONDITION(cond),,);
> >
> > This allow calling a tracepoint with:
> >
> > tracepoint(mytracepoint, process_tp_arg());
> >
> > making sure that process_tp_arg() will only be evaluated if
> > the tracepoint is enabled. It also ensures that it's impossible
> > to create a C construct that will open a race window where a
> > tracepoint could be called with an unpopulated parameter, such as:
> >
> > if (trace_mytracepoint_enabled())
> > arg = process_tp_arg();
> > trace_mytracepoint(arg);
> >
> > Thoughts ?
> >
>
> The first time I thought about using this was with David's code, which
> does this:
>
> if (static_key_false(&i2c_trace_msg)) {
> int i;
> for (i = 0; i < ret; i++)
> if (msgs[i].flags & I2C_M_RD)
> trace_i2c_reply(adap, &msgs[i], i);
> trace_i2c_result(adap, i, ret);
> }
>
> That would look rather silly in a tracepoint.
Which goes with a mandatory silly question: how do you intend mapping
the single key to two different tracepoints ?
Thanks,
Mathieu
>
> -- Steve
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
2014-05-06 20:53 ` Mathieu Desnoyers
@ 2014-05-06 21:06 ` Steven Rostedt
2014-05-06 21:16 ` Mathieu Desnoyers
0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2014-05-06 21:06 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: LKML, Andrew Morton, Javi Merino, David Howells, Ingo Molnar
On Tue, 6 May 2014 20:53:41 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> > I do plan on adding more documentation to this to stress that this
> > should be done like this. But hey, we're kernel developers, we should
> > be responsible enough to not require the hand holding.
>
> I like your optimism. ;-)
I'm always the optimist :-)
> > The first time I thought about using this was with David's code, which
> > does this:
> >
> > if (static_key_false(&i2c_trace_msg)) {
> > int i;
> > for (i = 0; i < ret; i++)
> > if (msgs[i].flags & I2C_M_RD)
> > trace_i2c_reply(adap, &msgs[i], i);
> > trace_i2c_result(adap, i, ret);
> > }
> >
> > That would look rather silly in a tracepoint.
>
> Which goes with a mandatory silly question: how do you intend mapping
> the single key to two different tracepoints ?
Could always do:
if (trace_i2c_result_enabled() || trace_i2c_reply_enabled()) {
I wounder what the assembly of that would look like.
Still, having "side-effects" in the tracepoint parameters just seems
odd to me.
- Steve
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
2014-05-06 21:06 ` Steven Rostedt
@ 2014-05-06 21:16 ` Mathieu Desnoyers
2014-05-07 3:10 ` [RFA][PATCH v2] " Steven Rostedt
0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-05-06 21:16 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Andrew Morton, Javi Merino, David Howells, Ingo Molnar
----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: "LKML" <linux-kernel@vger.kernel.org>, "Andrew Morton" <akpm@linux-foundation.org>, "Javi Merino"
> <javi.merino@arm.com>, "David Howells" <dhowells@redhat.com>, "Ingo Molnar" <mingo@kernel.org>
> Sent: Tuesday, May 6, 2014 5:06:40 PM
> Subject: Re: [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function
>
> On Tue, 6 May 2014 20:53:41 +0000 (UTC)
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>
[...]
>
> > > The first time I thought about using this was with David's code, which
> > > does this:
> > >
> > > if (static_key_false(&i2c_trace_msg)) {
> > > int i;
> > > for (i = 0; i < ret; i++)
> > > if (msgs[i].flags & I2C_M_RD)
> > > trace_i2c_reply(adap, &msgs[i], i);
> > > trace_i2c_result(adap, i, ret);
> > > }
> > >
> > > That would look rather silly in a tracepoint.
> >
> > Which goes with a mandatory silly question: how do you intend mapping
> > the single key to two different tracepoints ?
>
> Could always do:
>
> if (trace_i2c_result_enabled() || trace_i2c_reply_enabled()) {
>
> I wounder what the assembly of that would look like.
I would expect it to generate two static jump sites back to back.
>
> Still, having "side-effects" in the tracepoint parameters just seems
> odd to me.
I agree that the "enabled" static inline approach is more flexible. So
if we document it well enough, it might be OK in the end.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
* [RFA][PATCH v2] tracing: Add trace_<tracepoint>_enabled() function
2014-05-06 21:16 ` Mathieu Desnoyers
@ 2014-05-07 3:10 ` Steven Rostedt
2014-05-07 11:42 ` Mathieu Desnoyers
0 siblings, 1 reply; 8+ messages in thread
From: Steven Rostedt @ 2014-05-07 3:10 UTC (permalink / raw)
To: LKML
Cc: Mathieu Desnoyers, Andrew Morton, Javi Merino, David Howells,
Ingo Molnar
There are some code paths in the kernel that need to do some preparations
before it calls a tracepoint. As that code is worthless overhead when
the tracepoint is not enabled, it would be prudent to have that code
only run when the tracepoint is active. To accomplish this, all tracepoints
now get a static inline function called "trace_<tracepoint-name>_enabled()"
which returns true when the tracepoint is enabled and false otherwise.
As an added bonus, that function uses the static_key of the tracepoint
such that no branch is needed.
if (trace_mytracepoint_enabled()) {
arg = process_tp_arg();
trace_mytracepoint(arg);
}
Will keep the "process_tp_arg()" (which may be expensive to run) from
being executed when the tracepoint isn't enabled.
It's best to encapsulate the tracepoint itself in the if statement
just to keep races. For example, if you had:
if (trace_mytracepoint_enabled())
arg = process_tp_arg();
trace_mytracepoint(arg);
There's a chance that the tracepoint could be enabled just after the
if statement, and arg will be undefined when calling the tracepoint.
Link: http://lkml.kernel.org/r/20140506094407.507b6435@gandalf.local.home
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
Changes since v1: Added some documentation about using this function.
---
Documentation/trace/tracepoints.txt | 24 ++++++++++++++++++++++++
include/linux/tracepoint.h | 10 ++++++++++
2 files changed, 34 insertions(+)
diff --git a/Documentation/trace/tracepoints.txt b/Documentation/trace/tracepoints.txt
index 6b018b5..a3efac6 100644
--- a/Documentation/trace/tracepoints.txt
+++ b/Documentation/trace/tracepoints.txt
@@ -115,6 +115,30 @@ If the tracepoint has to be used in kernel modules, an
EXPORT_TRACEPOINT_SYMBOL_GPL() or EXPORT_TRACEPOINT_SYMBOL() can be
used to export the defined tracepoints.
+If you need to do a bit of work for a tracepoint parameter, and
+that work is only used for the tracepoint, that work can be encapsulated
+within an if statement with the following:
+
+ if (trace_foo_bar_enabled()) {
+ int i;
+ int tot = 0;
+
+ for (i = 0; i < count; i++)
+ tot += calculate_nuggets();
+
+ trace_foo_bar(tot);
+ }
+
+All trace_<tracepoint>() calls have a matching trace_<tracepoint>_enabled()
+function defined that returns true if the tracepoint is enabled and
+false otherwise. The trace_<tracepoint>() should always be within the
+block of the if (trace_<tracepoint>_enabled()) to prevent races between
+the tracepoint being enabled and the check being seen.
+
+The advantage of using the trace_<tracepoint>_enabled() is that it uses
+the static_key of the tracepoint to allow the if statement to be implemented
+with jump labels and avoid conditional branches.
+
Note: The convenience macro TRACE_EVENT provides an alternative way to
define tracepoints. Check http://lwn.net/Articles/379903,
http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 9d30ee4..2e2a5f7 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -185,6 +185,11 @@ extern void syscall_unregfunc(void);
static inline void \
check_trace_callback_type_##name(void (*cb)(data_proto)) \
{ \
+ } \
+ static inline bool \
+ trace_##name##_enabled(void) \
+ { \
+ return static_key_false(&__tracepoint_##name.key); \
}
/*
@@ -230,6 +235,11 @@ extern void syscall_unregfunc(void);
} \
static inline void check_trace_callback_type_##name(void (*cb)(data_proto)) \
{ \
+ } \
+ static inline bool \
+ trace_##name##_enabled(void) \
+ { \
+ return false; \
}
#define DEFINE_TRACE_FN(name, reg, unreg)
--
1.8.1.4
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFA][PATCH v2] tracing: Add trace_<tracepoint>_enabled() function
2014-05-07 3:10 ` [RFA][PATCH v2] " Steven Rostedt
@ 2014-05-07 11:42 ` Mathieu Desnoyers
0 siblings, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers @ 2014-05-07 11:42 UTC (permalink / raw)
To: Steven Rostedt
Cc: LKML, Andrew Morton, Javi Merino, David Howells, Ingo Molnar
----- Original Message -----
> From: "Steven Rostedt" <rostedt@goodmis.org>
> To: "LKML" <linux-kernel@vger.kernel.org>
> Cc: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>, "Andrew Morton" <akpm@linux-foundation.org>, "Javi Merino"
> <javi.merino@arm.com>, "David Howells" <dhowells@redhat.com>, "Ingo Molnar" <mingo@kernel.org>
> Sent: Tuesday, May 6, 2014 11:10:59 PM
> Subject: [RFA][PATCH v2] tracing: Add trace_<tracepoint>_enabled() function
>
>
> There are some code paths in the kernel that need to do some preparations
> before it calls a tracepoint. As that code is worthless overhead when
> the tracepoint is not enabled, it would be prudent to have that code
> only run when the tracepoint is active. To accomplish this, all tracepoints
> now get a static inline function called "trace_<tracepoint-name>_enabled()"
> which returns true when the tracepoint is enabled and false otherwise.
>
> As an added bonus, that function uses the static_key of the tracepoint
> such that no branch is needed.
>
> if (trace_mytracepoint_enabled()) {
> arg = process_tp_arg();
> trace_mytracepoint(arg);
> }
>
> Will keep the "process_tp_arg()" (which may be expensive to run) from
> being executed when the tracepoint isn't enabled.
>
> It's best to encapsulate the tracepoint itself in the if statement
> just to keep races. For example, if you had:
>
> if (trace_mytracepoint_enabled())
> arg = process_tp_arg();
> trace_mytracepoint(arg);
>
> There's a chance that the tracepoint could be enabled just after the
> if statement, and arg will be undefined when calling the tracepoint.
>
> Link: http://lkml.kernel.org/r/20140506094407.507b6435@gandalf.local.home
>
> Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Acked-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---
> Changes since v1: Added some documentation about using this function.
> ---
> Documentation/trace/tracepoints.txt | 24 ++++++++++++++++++++++++
> include/linux/tracepoint.h | 10 ++++++++++
> 2 files changed, 34 insertions(+)
>
> diff --git a/Documentation/trace/tracepoints.txt
> b/Documentation/trace/tracepoints.txt
> index 6b018b5..a3efac6 100644
> --- a/Documentation/trace/tracepoints.txt
> +++ b/Documentation/trace/tracepoints.txt
> @@ -115,6 +115,30 @@ If the tracepoint has to be used in kernel modules, an
> EXPORT_TRACEPOINT_SYMBOL_GPL() or EXPORT_TRACEPOINT_SYMBOL() can be
> used to export the defined tracepoints.
>
> +If you need to do a bit of work for a tracepoint parameter, and
> +that work is only used for the tracepoint, that work can be encapsulated
> +within an if statement with the following:
> +
> + if (trace_foo_bar_enabled()) {
> + int i;
> + int tot = 0;
> +
> + for (i = 0; i < count; i++)
> + tot += calculate_nuggets();
> +
> + trace_foo_bar(tot);
> + }
> +
> +All trace_<tracepoint>() calls have a matching trace_<tracepoint>_enabled()
> +function defined that returns true if the tracepoint is enabled and
> +false otherwise. The trace_<tracepoint>() should always be within the
> +block of the if (trace_<tracepoint>_enabled()) to prevent races between
> +the tracepoint being enabled and the check being seen.
> +
> +The advantage of using the trace_<tracepoint>_enabled() is that it uses
> +the static_key of the tracepoint to allow the if statement to be implemented
> +with jump labels and avoid conditional branches.
> +
> Note: The convenience macro TRACE_EVENT provides an alternative way to
> define tracepoints. Check http://lwn.net/Articles/379903,
> http://lwn.net/Articles/381064 and http://lwn.net/Articles/383362
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 9d30ee4..2e2a5f7 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -185,6 +185,11 @@ extern void syscall_unregfunc(void);
> static inline void \
> check_trace_callback_type_##name(void (*cb)(data_proto)) \
> { \
> + } \
> + static inline bool \
> + trace_##name##_enabled(void) \
> + { \
> + return static_key_false(&__tracepoint_##name.key); \
> }
>
> /*
> @@ -230,6 +235,11 @@ extern void syscall_unregfunc(void);
> } \
> static inline void check_trace_callback_type_##name(void (*cb)(data_proto))
> \
> { \
> + } \
> + static inline bool \
> + trace_##name##_enabled(void) \
> + { \
> + return false; \
> }
>
> #define DEFINE_TRACE_FN(name, reg, unreg)
> --
> 1.8.1.4
>
>
>
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-05-07 11:42 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-05-06 13:44 [RFA][PATCH] tracing: Add trace_<tracepoint>_enabled() function Steven Rostedt
2014-05-06 19:35 ` Mathieu Desnoyers
2014-05-06 19:48 ` Steven Rostedt
2014-05-06 20:53 ` Mathieu Desnoyers
2014-05-06 21:06 ` Steven Rostedt
2014-05-06 21:16 ` Mathieu Desnoyers
2014-05-07 3:10 ` [RFA][PATCH v2] " Steven Rostedt
2014-05-07 11:42 ` Mathieu Desnoyers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox