Re: trace_mark ugliness

!MAILaRCHIVE_VOTE_RePLACE
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
To: Steven Rostedt <rostedt@...>
Cc: Peter Zijlstra <peterz@...>, Ingo Molnar <mingo@...>, Christoph Hellwig <hch@...>, LKML <linux-kernel@...>
Date: Thursday, May 22, 2008 - 1:16 pm

* Steven Rostedt (rostedt@goodmis.org) wrote:

Hi Steven,

Thanks for the fix,


Hrm, hrm, ok, let's brainstorm along these lines. So we would like to
have :
- Multiple tracers
- Each tracer can connect either to one or more different markers
- Each marker should support many tracers connected to it
- Checking for marker/tracer probe compatibility should be done via
  function prototypes.

The main issue here seems to be to support multiple probes connected at
once on a given marker. With the current markers, I deal with this by
taking a pointer on the va_list and go through as many va_start/va_end
as required (one pair for each connected probe). By the way, the probes
does not have to issue va_start/end; marker.c deals with this.

Also, given that I want to support SystemTAP, it adds the following
constraint : we cannot expect the probes to be there at compile-time,
since they can be provided by modules built much later. Therefore, we
have to provide support for dynamic connection of an arbitrary number of
probes on any given marker.

So while I *could* remove the format string easily, it's the variable
argument list which I don't see clearly how to drop while still
providing flexible argument types -and- compile-time type verification.

What currently looks like (this is a simplified pseudo-code) :

void marker_probe_cb(const struct marker *mdata, void *call_private, ...)
{
  va_list args;
  int i;

  preempt_disable();
  for (i = 0; multi[i].func; i++)  {
    va_start(args, call_private);
    multi[i].func(multi[i].probe_private, call_private,
      mdata->format, &args);
    va_end(args);
  }
  preempt_enable();
}

Would have to be changed into specialized functions for each marker,
involving quite a lot of code to be generated, e.g. :

void marker_XXnameXX_probe_cb(const struct marker *mdata,
    int arg1, void *arg2, struct mystruct *arg3)
{
  int i;

  preempt_disable();
  for (i = 0; multi[i].func; i++)
    multi[i].func(multi[i].probe_private, arg1, arg2, arg3);
  preempt_enable();
}

That would imply that the struct marker_probe_closure, currently defined
as :

typedef void marker_probe_func(void *probe_private, void *call_private,
                const char *fmt, va_list *args);

struct marker_probe_closure {
        marker_probe_func *func;        /* Callback */
        void *probe_private;            /* Private probe data */
};

Would have to be duplicated for each marker prototype so we can provide
compile-time check of these prototypes. The registration functions would
also have to be duplicated to take parameters which include all those
various prototypes. They are required so that kernel modules can provide
probes (e.g. systemtap and LTTng).

I don't really see how your proposal deals with these constraints
without duplicating much of the marker code on a per marker basis.
However, if we can find a clever way to do it without the code
duplication, I'm all in.

Ideas/insights are welcome,

Mathieu


-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68
--
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
trace_mark ugliness, Steven Rostedt, (Thu May 22, 10:24 am)
Re: trace_mark ugliness, Mathieu Desnoyers, (Thu May 22, 1:16 pm)
Re: trace_mark ugliness, Peter Zijlstra, (Sun May 25, 7:20 am)
Re: trace_mark ugliness, Mathieu Desnoyers, (Tue May 27, 9:36 am)
Re: trace_mark ugliness, Peter Zijlstra, (Wed May 28, 11:19 am)
Re: trace_mark ugliness, Steven Rostedt, (Thu May 22, 1:52 pm)