Re: [RFA][PATCH v2] tracing: Add trace_<tracepoint>_enabled() function

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

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