From: Steven Rostedt <srostedt@redhat.com>

This patch removes the register functions of TRACE_EVENT() to enable
and disable tracepoints. The registering of a event is now down
directly in the trace_events.c file. The tracepoint_probe_register()
is now called directly.

The prototypes are no longer type checked, but this should not be
an issue since the tracepoints are created automatically by the
macros. If a prototype is incorrect in the TRACE_EVENT() macro, then
other macros will catch it.

The trace_event_class structure now holds the probes to be called
by the callbacks. This removes needing to have each event have
a separate pointer for the probe.

To handle kprobes and syscalls, since they register probes in a
different manner, a "reg" field is added to the ftrace_event_class
structure. If the "reg" field is assigned, then it will be called for
enabling and disabling of the probe for either ftrace or perf. To let
the reg function know what is happening, a new enum (trace_reg) is
created that has the type of control that is needed.

With this new rework, the 82 kernel events and 616 syscall events
has their footprint dramatically lowered:

   text	   data	    bss	    dec	    hex	filename
5788186	1337252	9351592	16477030	 fb6b66	vmlinux.orig
5792282	1333796	9351592	16477670	 fb6de6	vmlinux.class
5793448	1333780	9351592	16478820	 fb7264	vmlinux.tracepoint
5796926	1337748	9351592	16486266	 fb8f7a	vmlinux.data
5774316	1306580	9351592	16432488	 fabd68	vmlinux.regs

The size went from 16477030 to 16432488, that's a total of 44K
in savings. With tracepoints being continuously added, this is
critical that the footprint becomes minimal.

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
---
 include/linux/ftrace_event.h    |   17 +++++--
 include/linux/syscalls.h        |   29 ++---------
 include/linux/tracepoint.h      |   12 ++++-
 include/trace/ftrace.h          |  110 +++++----------------------------------
 kernel/trace/trace_event_perf.c |   15 ++++-
 ...
[ message continues ]

Have you tried doing a BUILD_BUG_ON() on __typeof__() mismatch between
the type of the callback generated by TRACE_EVENT() and the expected
type ?  This might help catching tricky preprocessor macro errors early.

Thanks,


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

[ message continues ]

Well, we could, but if it is broken once, it is broken everywhere.

-- Steve


--

[ message continues ]

I fear about "subtly" broken things, where trace data could end up being
incorrectly typed and/or corrupted. I think this BUILD_BUG_ON() will
become very useful.

Thanks,


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

[ message continues ]

Actually, I'm not sure what you want to check. What is not checked is
the prototype that is created, to the prototype that is passed to the
tracepoint_probe_register. Other parts are still checked. If you
mis-match the args with the parameters, there are still places that the
compiler will flag it. There really is not much less protection here
than there was before.

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.

-- Steve


--

[ message continues ]

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

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 Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--

[ message continues ]

How can you be sure that the "void *data" type will match the type at
the same position in the generated callback ?

Honestly, I don't think kernel programmers write bug-free code. And I
include myself when I say that. So the best we can do, on top of code
review, is to use all the verification and debugging tools available to
minimize the amount of undetected bugs. Rather than try to find out the
cause of subtly broken tracepoint callbacks with their runtime
side-effects, I strongly prefer to let the compiler find this out as
early as possible.

I also don't trust that these complex TRACE_EVENT() preprocessor macros
will never ever have bugs. That's just doomed to happen one day or
another. Again, call me paranoid if you like, but I think adding this
type checking is justified.

I am providing the type check implementation in a separate email. It
will need to be extended to support the extra data parameter you plan to
add.

Thanks,


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

[ message continues ]

We do it all the time in the kernel with no type checking. Just look at

If it is possible sure, but that's the point. Where do you add the
check? The typecast is in the C code that is constant for all trace


Where do you add the typecheck?? As I said before, if the TRACE_EVENT()
macros are broken, then so will the typecheck, and it will not catch the
errors.

Sure the event macros can have bugs, but if it does then it will have
bugs for all. Because it is automated. If there is a bug, it wont be
because of a missed type being passed in, it would be because of one of

I saw the patch, but how does it help?

I use "proto" to make the tracepoint and the callback, so I can add
somewhere this "check_trace_callback_type_##name(proto)", but if the
macros break somehow, that means proto changed between two references of
it, but what keeps proto from breaking at both callback creation and the
typecheck.

Basically, you are saying that somehow the argument "proto" can change
between two uses of it. I don't really see that happening, and I'm not
paranoid enough to think that's an issue. Adding checks that don't
really check anything, honestly I find a waste, and just more confusion
in the macros.



--

[ message continues ]

You can add the call to the static inline type check directly within the

Please don't take this personally. As I said above, I include myself in
the list of people I don't trust to write entirely bug-free code. I'm
just saying that we should not overlook a possibility to detect more
bugs automatically when we have one, especially if this results in no

In the TRACE_EVENT() case, without the extra "void *data" argument,
it is indeed checking that the "proto" of the callback you create is
that same as the "proto" expected by the tracepoint call. However, given
that you plan on adding other parameters besides "proto", then the added
type-checking makes more and more sense.

Thanks,


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

[ message continues ]

Well, one thing, the callback is not going to be the same as the
DECLARE_TRACE() because the prototype ends with "void *data", and the
function being called actually uses the type of that data.

We now will have:

	DEFINE_TRACE(mytracepoint, int myarg, myarg);

	void mycallback(int myarg, struct mystuct *mydata);

	register_trace_mytracepoint_data(mycallback, mydata)

There's no place in DEFINE_TRACE to be able to test the type of data
that is being passed back. I could make the calling function be:

	void mycallback(int myarg, void *data)
	{
		struct mystruct *mydata = data;
	[...]

Because the data is defined uniquely by the caller that registers a

The point being is that this is not about buggy code, but the fact that
the same data is being used in two places, you want to test to make sure

But you can not test it! That's my point.

The first part of proto will be the same, and that's all we can test.
But the data parameter that the DECLARE_TRACE() is going to create will
be void *. Which means we can't test it. This is something that C lacks,
and we could test it in C++ if we did this with templates. The only way
to test it is at runtime with a magic number in the data field.

This is the same as the file->private data. You can't test it at build
time.

Let me explain this again:

	DECLARE_TRACE(name, proto, args);

Will call the function like:

	callback(args, data);

The callback will be at best:

	int callback(proto, void *data);


because the data being passed in is not defined yet. It is defined at
the point of the registering of the callback. You can have two callbacks
registered to the same tracepoint with two different types as the data
field.

So what is it that this check is testing?

-- Steve


--

[ message continues ]

Yep. There would need to be a cast from void * to struct mystruct *
at the beginning of the callback as you propose here. I prefer this cast
to be explicit (as proposed here) rather than hidden within the entire

See my comment above about specifically casting the void *data parameter
rather than relying on casting of the whole callback function pointer

It's making sure that TRACE_EVENT() creates callbacks with the following
signature:

  void callback(proto, void *data)

rather than

  void callback(proto, struct somestruct *data)

and forces the cast to be done within the callback rather than casting
the whole function pointer type to void *, assuming types to match. I
prefer to leave the cast outside of the tracepoint infrastructure, so we
do not obfuscate the fact that an explicit type cast is needed there.

Thanks,


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

[ message continues ]

OK, so you prefer that, I don't, but I also don't care, so I could

Fine, but I hardly see it as obfuscation. But my question again, even if
we do change this. What is this test testing? To me, it is checking that
CPP works.

-- Steve


--

[ message continues ]

It's checking that the macros generated compatible call/callback
prototypes, yes. It comes down to using the compiler type-checking to
double-check that the macros are fine.

Thanks,


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

[ message continues ]

We could add it, but I'm not sure it would add any more protection. If
for some strange reason the prototype got out of sync, would would
prevent the cb_type from getting out of sync with it too, and not cause
this to fail, but still have the same bug.

Honestly, I find this a bit too paranoid. Again, the callback and the
tracepoint are made with the same data. I find it hard to think that it
would break somehow. Yes, perhaps it will break if you modify ftrace.h,
but then if you are doing that, you should know better than to break



Sure, the static function you did could be added, and hope that gcc is
smart enough to get rid of it (add __unused to it). But what are we
really checking here? If CPP works?

-- Steve


--

[ message continues ]

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

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)		\
[...]


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 ...
[ message continues ]