Re: [PATCH 04/10][RFC] tracing: Remove per event trace registering

[Mathieu Desnoyers <compudj@krystal.dyndns.org>]

* Steven Rostedt (rostedt@goodmis.org) wrote:
> On Thu, 2010-04-29 at 09:36 -0400, Mathieu Desnoyers wrote:
> 
> > > Instead of calling register_trace_##name that is created for each
> > > tracepoint, we now call the tracepoint_probe_register() directly in the
> > > C file with the generated probe.
> > > 
> > > Both the probe and the tracepoint are created from the same data. I'm
> > > not seeing where you want to add this check.
> > 
> > So if they are created from the same data, we can expect this test to
> > always pass, which is good (and expected).
> > 
> > I'd add this extra check before casting the callback to (void *) before
> > it is passed to tracepoint_probe_register(). Let's just call this
> > internal preprocessing macro integrity check. As long as it does not add
> > a runtime cost, there is no reason not to put this extra check.
> 
> The problem is, the cast is now performed in a C file for all events.
> There's no way to know what to cast it to there. This is out of the
> automation of the macro.
> 
> We use to have the cast check by creating code that would create the
> "register_trace_##call", and the typecheck was doing by passing the data
> to this function. But we removed this code out of the per event, it was
> adding lots of text footprint, and moved it to one single function that
> handles all events. It is just expected that the callback created
> matches the function it was done.
> 
> If you are overly paranoid, we could create a special function that
> tests that the callback format that is created matches the tracepoint
> that is created, and make it so GCC sees that nothing calls it and
> removes it at final link. But I still see this as a waste.
> 
> 
> The tracepoint is created in include/linux/tracepoint.h:
> 
> #define TRACE_EVENT(name, proto, args, struct, assign, print)	\
> 	DECLARE_TRACE(name, PARAMS(proto), PARAMS(args))

Can we add something like this to DECLARE_TRACE ? (not convinced it is
valid though)

static inline void check_trace_##name(cb_type)
{
	BUILD_BUG_ON(!__same_type(void (*probe)(TP_PROTO(proto), void *data),
				  cb_type));
}

> 
> The callback is created in include/trace/ftrace.h:
> 
> #undef TRACE_EVENT
> #define TRACE_EVENT(name, proto, args, tstuct, assign, print)	\
> 	DECLARE_EVENT_CLASS(name,				\
> 				PARAMS(proto),			\
> 				PARAMS(args),			\
> 				PARAMS(tstruct),		\
> 				PARAMS(assign),			\
> 				PARAMS(print));			\
> 	DEFINE_EVENT(name, name, PARAMS(proto), PARAMS(args));
> 
> #undef DECLARE_EVENT_CLASS
> #define DECLARE_EVENT_CLASS(call, proto, args, tstruct, assign, print)	\
> 									\
> static notrace void							\
> ftrace_raw_event_##call(proto,						\
> 			struct ftrace_event_call *event_call)		\
> [...]
> 

Either within this callback, or in a dummy static function after, we
could add:

check_trace_##call(ftrace_raw_event_##call);

So.. you are the preprocessor expert, do you think this could fly ? ;)

Thanks,

Mathieu

> 
> Thus the "proto" field of the TRACE_EVENT() is used to make both the
> tracepoint and the callback. We add the struct ftrace_event_call
> *event_call which is the data we pass to the callback.
> 
> Now, where this gets called is in kernel/trace/trace_events.c:
> 
> 	tracepoint_probe_register(call->name,
> 				  call->class->probe,
> 				  call);
> 
> This is where we lose the typecheck. So my question is... where do you
> want to put in a check?
> 
> -- Steve
> 
> 	
> 
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com