| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 07 May 2010 08:40:37 -0400
From: Steven Rostedt <rostedt@...dmis.org>
To: Frederic Weisbecker <fweisbec@...il.com>
CC: linux-kernel@...r.kernel.org, Ingo Molnar <mingo@...e.hu>,
Andrew Morton <akpm@...ux-foundation.org>,
Thomas Gleixner <tglx@...utronix.de>,
Peter Zijlstra <peterz@...radead.org>,
Arnaldo Carvalho de Melo <acme@...hat.com>,
Mathieu Desnoyers <compudj@...stal.dyndns.org>,
Lai Jiangshan <laijs@...fujitsu.com>,
Li Zefan <lizf@...fujitsu.com>,
Masami Hiramatsu <mhiramat@...hat.com>,
Christoph Hellwig <hch@....de>,
Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
Subject: Re: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data passed to tracepoint callbacks
"Frederic Weisbecker" <fweisbec@...il.com> wrote:
>On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
>> From: Steven Rostedt <srostedt@...hat.com>
>>
>> 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@...icios.com>
>> Signed-off-by: Steven Rostedt <rostedt@...dmis.org>
>> ---
>> 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.
>
Yeah, I'm hoping it's fine.
>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.
>
OK I'll add a bit of comments to the macros. So much for my job security ;-)
>
>
>
>> + } 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 :)
OK
>
>
>
>> +
>> +#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.
>
I don't know what you mean here?
-- Steve
>Thanks.
>
--
Sent from my Android phone with K-9 Mail. Please excuse my brevity.
Powered by blists - more mailing lists