Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have datapassed to tracepoint callbacks
From: Frederic Weisbecker
Date: Thu May 06 2010 - 23:52:51 EST
On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
> From: Steven Rostedt <srostedt@xxxxxxxxxx>
>
> This patch allows data to be passed to the tracepoint callbacks
> if the tracepoint was created to do so.
>
> The DECLARE_TRACE() now adds two new functions:
>
> register_trace_mytracepoint_data()
> unregister_trace_mytracepoint_data()
>
> These two are the same as the original
>
> register_trace_mytracepoint()
> unregister_trace_mytracepoint()
>
> But now allow you to pass a private data pointer that will
> be passed to the callback handle. For example:
>
> DECLARE_TRACE(mytracepoint, int value, value);
>
> will create a function called trace_mytracepoint()
>
> void trace_mytracepoint(int value);
>
> If the user wants to pass data to register a function to this tracepoint
> and have data also passed to this callback, they can use:
>
> int mycallback(int value, void *data);
>
> register_trace_mytracepoint_data(mycallback, mydata);
>
> Then the mycallback() will receive the "mydata" as the parameter after
> the args.
>
> A more detailed example:
>
> DECLARE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));
>
> /* In the C file */
>
> DEFINE_TRACE(mytracepoint, TP_PROTO(int status), TP_ARGS(status));
>
> [...]
>
> trace_mytacepoint(status);
>
> /* In a file registering this tracepoint */
>
> int my_callback(int status, void *data)
> {
> struct my_struct my_data = data;
> [...]
> }
>
> [...]
> my_data = kmalloc(sizeof(*my_data), GFP_KERNEL);
> init_my_data(my_data);
> register_trace_mytracepoint_data(my_callback, my_data);
>
> The same callback can also be registered to the same tracepoint as long
> as the data registered is the different. Note, the data must also be used
> to unregister the callback:
>
> unregister_trace_mytracepoint_data(my_callback, my_data);
>
> Because of the data parameter, tracepoints declared this way can not have
> no args. That is:
>
> DECLARE_TRACE(mytracepoint, TP_PROTO(void), TP_ARGS());
>
> will cause an error.
>
> If no arguments are needed, a new macro can be used instead:
>
> DECLARE_TRACE_NOARGS(mytracepoint);
>
> Since there are no arguments, the proto and args fields are left out.
>
> This is part of a series to make the tracepoint footprint smaller:
>
> 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
>
> Again, this patch also increases the size of the kernel, but
> lays the ground work for decreasing it.
>
> v2: Made the DECLARE_TRACE() have the ability to pass arguments
> and added a new DECLARE_TRACE_NOARGS() for tracepoints that
> do not need any arguments.
>
> Cc: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Signed-off-by: Steven Rostedt <rostedt@xxxxxxxxxxx>
> ---
> include/linux/tracepoint.h | 94 +++++++++++++++++++++++++-------
> kernel/tracepoint.c | 91 +++++++++++++++++--------------
> samples/tracepoints/tp-samples-trace.h | 4 +-
> 3 files changed, 126 insertions(+), 63 deletions(-)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index 78b4bd3..ee8059a 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -20,12 +20,17 @@
> struct module;
> struct tracepoint;
>
> +struct tracepoint_func {
> + void *func;
> + void *data;
> +};
> +
> struct tracepoint {
> const char *name; /* Tracepoint name */
> int state; /* State. */
> void (*regfunc)(void);
> void (*unregfunc)(void);
> - void **funcs;
> + struct tracepoint_func *funcs;
> } __attribute__((aligned(32))); /*
> * Aligned on 32 bytes because it is
> * globally visible and gcc happily
> @@ -46,14 +51,18 @@ struct tracepoint {
> */
> #define __DO_TRACE(tp, proto, args) \
> do { \
> - void **it_func; \
> + struct tracepoint_func *it_func_ptr; \
> + void *it_func; \
> + void *__data; \
> \
> rcu_read_lock_sched_notrace(); \
> - it_func = rcu_dereference_sched((tp)->funcs); \
> - if (it_func) { \
> + it_func_ptr = rcu_dereference_sched((tp)->funcs); \
> + if (it_func_ptr) { \
> do { \
> - ((void(*)(proto))(*it_func))(args); \
> - } while (*(++it_func)); \
> + it_func = (it_func_ptr)->func; \
> + __data = (it_func_ptr)->data; \
> + ((void(*)(proto))(it_func))(args); \
So, we had a talk about this and we concluded that it is probably fine
on every archs to push one more argument than needed in a function.
But I think it would be nice to add a comment about this. Firstly
because this line breaks all the self-explanation of the code, I mean
I tried hard to find how the non-data callback case was handled :)
Secondly to also to avoid people asking what happens here.
> + } while ((++it_func_ptr)->func); \
> } \
> rcu_read_unlock_sched_notrace(); \
> } while (0)
> @@ -63,23 +72,47 @@ struct tracepoint {
> * not add unwanted padding between the beginning of the section and the
> * structure. Force alignment to the same alignment as the section start.
> */
> -#define DECLARE_TRACE(name, proto, args) \
> +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args) \
> extern struct tracepoint __tracepoint_##name; \
> static inline void trace_##name(proto) \
> { \
> if (unlikely(__tracepoint_##name.state)) \
> __DO_TRACE(&__tracepoint_##name, \
> - TP_PROTO(proto), TP_ARGS(args)); \
> + TP_PROTO(data_proto), \
> + TP_ARGS(data_args)); \
> } \
> static inline int register_trace_##name(void (*probe)(proto)) \
> { \
> - return tracepoint_probe_register(#name, (void *)probe); \
> + return tracepoint_probe_register(#name, (void *)probe, \
> + NULL); \
> + } \
> + static inline int unregister_trace_##name(void (*probe)(proto)) \
> + { \
> + return tracepoint_probe_unregister(#name, (void *)probe,\
> + NULL); \
> } \
> - static inline int unregister_trace_##name(void (*probe)(proto)) \
> + static inline int \
> + register_trace_##name##_data(void (*probe)(data_proto), \
> + void *data) \
> { \
> - return tracepoint_probe_unregister(#name, (void *)probe);\
> + return tracepoint_probe_register(#name, (void *)probe, \
> + data); \
> + } \
> + static inline int \
> + unregister_trace_##name##_data(void (*probe)(data_proto), \
> + void *data) \
> + { \
> + return tracepoint_probe_unregister(#name, (void *)probe,\
> + data); \
> }
>
> +#define DECLARE_TRACE_NOARGS(name) \
> + __DECLARE_TRACE(name, void, , void *__data, __data)
That too, may be, deserves a small comment :)
> +
> +#define DECLARE_TRACE(name, proto, args) \
> + __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), \
> + PARAMS(proto, void *__data), \
> + PARAMS(args, __data))
>
> #define DEFINE_TRACE_FN(name, reg, unreg) \
> static const char __tpstrtab_##name[] \
> @@ -100,19 +133,37 @@ extern void tracepoint_update_probe_range(struct tracepoint *begin,
> struct tracepoint *end);
>
> #else /* !CONFIG_TRACEPOINTS */
> -#define DECLARE_TRACE(name, proto, args) \
> - static inline void _do_trace_##name(struct tracepoint *tp, proto) \
> - { } \
> +#define __DECLARE_TRACE(name, proto, args, data_proto, data_args) \
> static inline void trace_##name(proto) \
> - { } \
> + { \
> + } \
> static inline int register_trace_##name(void (*probe)(proto)) \
> { \
> return -ENOSYS; \
> } \
> - static inline int unregister_trace_##name(void (*probe)(proto)) \
> + static inline int unregister_trace_##name(void (*probe)(proto)) \
> + { \
> + return -ENOSYS; \
> + } \
> + static inline int \
> + register_trace_##name##_data(void (*probe)(data_proto), \
> + void *data) \
> + { \
> + return -ENOSYS; \
> + } \
> + static inline int \
> + unregister_trace_##name##_data(void (*probe)(data_proto), \
> + void *data) \
> { \
> return -ENOSYS; \
> }
> +#define DECLARE_TRACE_NOARGS(name) \
> + __DECLARE_TRACE(name, void, , void *__data, __data)
> +
> +#define DECLARE_TRACE(name, proto, args) \
> + __DECLARE_TRACE(name, PARAMS(proto), PARAMS(args), \
> + PARAMS(proto, void *__data), \
> + PARAMS(args, __data))
It seems that the on and off cases are exactly the same for DECLARE_TRACE*(),
you could provide a single version and let the __DECLARE_TRACE() do
the on/off trick.
Thanks.
--
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/