Re: [PATCH v2 3/3] ftrace: add tracepoint for itimer

[Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>]

* Thomas Gleixner (tglx@linutronix.de) wrote:
> On Mon, 6 Jul 2009, Mathieu Desnoyers wrote:
> > * Xiao Guangrong (xiaoguangrong@cn.fujitsu.com) wrote:
> > > +
> > > +TRACE_EVENT(itimer_state,
> > > +
> > 
> > Hrm, I'd reserve the use of the *_state namespace for something
> > different if possible.
> 
> See below.
> 
> > In LTTng, we have the ability to list a set of data structures at trace
> > start (we call this state dump). In this case, we could add a list of
> > all active timers at trace start. This is useful if the trace started
> > after timer inits.
> 
> Can you please spare us the gory details of LTTng. We already know
> that it can do everything plus brewing coffee, so no need to advertise
> it in every tracing related mail.
> 
> > So having "_state" without much meaning here might eventually be
> > confusing.
> > 
> > "_action" or "_call" (like IPCs) might be more appropriate ?
> 
> Err, where is this an action or a call ? Care to look at the call site
> of this tracepoint. It's at the end of do_setitimer() and records the
> _state_ of the itimer _after_ the syscall was handled.
> 

Hrm, right. The patch context misled me. I thought it was at the end of
it_real_fn(), but it's actually in do_setitimer().

OK, looking at it from it's actual context, it makes perfect sense to
name it that way.

And it could even be used as-is to iterate on all timers at trace start
as we do in this tracer I won't name. ;-)

BTW, I was merely pointing out the risk of a name clash here. And I
had to refer to the tree where this piece of code exists to support the
point. My intent was not to advertise anything.

> > Mathieu
> 
> Can you please remove the hundred lines you are not commenting on next
> time ?
>  

OK,

Thanks,

Mathieu

> > > +	TP_PROTO(int which, struct itimerval *value, cputime_t expires),
> 
> Thanks,
> 
> 	tglx

-- 
Mathieu Desnoyers
OpenPGP key fingerprint: 8CD5 52C3 8E3C 4140 715F  BA06 3F25 A8FE 3BAE 9A68