| 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 10:09:31 -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
Hmm, I replied originally from my Google phone, but I don't see it in
LKML. So apologies if this is duplicate.
On Fri, 2010-05-07 at 05:52 +0200, Frederic Weisbecker wrote:
> On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
> > #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.
Yep, I'm hoping this is the case.
>
> 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 documentation here. 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. How would __DECLARE_TRACE() do what
both DECLARE_TRACE() and DECLARE_TRACE_NOARGS() do? It will fail the
compile if proto is "void".
-- Steve
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists