Re: [RFC PATCH 1/2] Marker probes in futex.c

[Peter Zijlstra <a.p.zijlstra@chello.nl>]

On Thu, 2008-04-17 at 15:19 -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> On Wed, Apr 16, 2008 at 05:51:24PM +0200, Peter Zijlstra wrote:
> > [...]
> > > > > > What is wrong with a few simple hooks like:
> > > > > >   trace_futex_wait(uaddr, fshares, val, abs_time, bitset);
> > > > > > and then deal with that.
> > [...]
> > > Yes, but then you would have to create new code for each event you want
> > > to trace. In the end, it would increase the icache footprint
> > > considerably and would also make addition of new events cumbersome.
> > > [...]
> 
> That, plus the new hand-written function (trace_futex_wait) would
> still need to manage the packaging of the arguments for consumption by
> separately compiled pieces.  It is desirable not to require such
> hand-written functions to *also* be declared in headers for these
> event consumers to compile against.

*blink* so all this is so you don't have to put a declarion in a header
file?

How about we put these premanent markers in a header - Mathieu says
there are <200. Surely that's not too much trouble.

Then you can keep this trace_mark() (perhaps trace_printf() is a better
name) around for the ad-hoc debug hacks.

> > So I'm not sure what adding all these character strings buy you.
> 
> The main thing is type checking by engaging gcc's printf format
> checking logic.  In my original markers proposal, the types were
> encoded into the function name, sort of as in C++:
> 
>   trace_mark_nnnnn(futex_wake_called, uaddr, fshares, val, abs_time, bitset);
> 
> where each "n" stands for some integral value, and could be chosen
> amongst a small number of other types (say -- "s": char* string, "p":
> void*, "l":64-bit long).  Then, type checking could be done by the
> core compiler for both event producers and consumers.  One downside
> was that the trace_mark_* permutations themselves would have to be
> generated by some shell/perl script [1], and some deemed this probably
> unacceptable.  I'm still not sure...
> 
> [1] some systemtap archaeology:
> http://sourceware.org/git/?p=systemtap.git;a=commit;h=b171146c8e8d4fa749b8829c47750750dc19f11c
> 
> 
> > >+       trace_mark(futex_wake_called, "uaddr:%p fshared:%p nr_wake:%d "
> > > +                       "bitset:%d",
> > > +                       uaddr, fshared, nr_wake, bitset);
> > 
> > > +       INIT_FUTEX_DEBUG_PROBE(futex_wake_called,
> > > +                       "uaddr:%p fshared:%p nr_wake:%d bitset:%d"),
> > 
> > Why the need to duplicate it; that's utter madness.
> 
> This second instance is optional and is used as a consistency check
> for the event consumer to hook up exactly to the intended producer.
> The string could be empty.

So instead of writing normal C code and placing a declarion in a header,
you've come up with a scheme that needs to duplicate a text string to
check integrity. Sounds like a real good way to confuse people.