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

[Steven Rostedt]

On Tue, 6 May 2014 19:35:32 +0000 (UTC)
Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/