lkml.org 
[lkml]   [2010]   [May]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/9 - v2][RFC] tracing: Let tracepoints have data?passed to tracepoint callbacks
From
Date
On Fri, 2010-05-07 at 10:39 -0400, Mathieu Desnoyers wrote:
> * Steven Rostedt (rostedt@goodmis.org) wrote:
> >
> >
> > "Frederic Weisbecker" <fweisbec@gmail.com> wrote:
> >
> > >On Mon, May 03, 2010 at 11:40:47PM -0400, Steven Rostedt wrote:
> [...]
> > >>
> > >> 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.
>
> How about changing the callback prototypes to match the call arguments (changing
> the type expected in register/unregister_trace, as well as an additional "check
> type" that I proposed for Ftrace) ?

This can not happen!!!! As I said before, the register is done in C,
there is no macro that will help here. We create the call back with the
macro, but the registering is in kernel/trace/trace_events.c. One
register for ___ALL___ events!!!

Thus there is no check.

Understand this yet?


>
> Otherwise, you basically expect here that:
>
> void fct(void *foo, void *bar, etc etc) (N parameters expected)
> {
>
> }
>
> called by:
>
> fct(foo, bar, etc etc, foobar) (N + 1 parameters)
>
> will always work.
>
> Can you show me where the C standard says it is safe to do so ?

No, but it seems safe in the kernel ;-)

But that said. There is another option that will conform to this, and
that is to add flags to registering tracepoints. I already wrote a patch
for this in trying to do some other work (that I threw away).


So here's the proposal.

Change struct tracepoint_func to...

struct tracepoint_func {
void *func;
void *data;
unsigned int flags;
};


The flags is set when registered. If a function is registered with data,
then the flags field will be set. Then the calling of the function can
be:

if ((it_func_ptr)->flags & TP_FL_DATA)
((void(*)(proto, void *))(it_func)(args, __data);
else
((void(*)(proto))(it_func)(args);

This would comply with the C standard.

-- Steve




\
 
 \ /
  Last update: 2010-05-07 16:57    [W:1.613 / U:0.008 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site