login
Search
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 ++++- ...
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 --
Well, we could, but if it is broken once, it is broken everywhere. -- Steve --
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 --
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 --
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
--
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 --
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. --
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 --
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
--
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 --
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 --
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 --
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 --
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 ...
