From mboxrd@z Thu Jan 1 00:00:00 1970 From: mingo@elte.hu (Ingo Molnar) Date: Tue, 24 Mar 2009 19:21:22 +0100 Subject: [ltt-dev] [patch 6/9] LTTng instrumentation - timer In-Reply-To: <20090324160148.754766083@polymtl.ca> References: <20090324155625.420966314@polymtl.ca> <20090324160148.754766083@polymtl.ca> Message-ID: <20090324182122.GG31117@elte.hu> * Mathieu Desnoyers wrote: > Instrument timer activity (timer set, expired, current time > updates) to keep information about the "real time" flow within the > kernel. It can be used by a trace analysis tool to synchronize > information coming from various sources, e.g. to merge traces with > system logs. > > Those tracepoints are used by LTTng. > > About the performance impact of tracepoints (which is comparable > to markers), even without immediate values optimizations, tests > done by Hideo Aoki on ia64 show no regression. His test case was > using hackbench on a kernel where scheduler instrumentation (about > 5 events in code scheduler code) was added. See the "Tracepoints" > patch header for performance result detail. > > Note : do_setitimer instrumentation could potentially be done with > a more generic system call instrumentation. > > CC: 'Steven Rostedt' > CC: Eduard - Gabriel Munteanu > --- > include/trace/timer.h | 24 ++++++++++++++++++++++++ > kernel/itimer.c | 8 ++++++++ > kernel/timer.c | 12 +++++++++++- > 3 files changed, 43 insertions(+), 1 deletion(-) For complete timer instrumentation, hrtimers should be instrumented as well. > return HRTIMER_NORESTART; > @@ -148,6 +154,8 @@ int do_setitimer(int which, struct itime > !timeval_valid(&value->it_interval)) > return -EINVAL; > > + trace_timer_itimer_set(which, value); for consistency, i'd name the timer start tracepoints the following way: trace_timer_init() trace_itimer_init() trace_hrtimer_init() trace_timer_start() trace_itimer_start() trace_hrtimer_start() trace_timer_expire() trace_itimer_expire() trace_hrtimer_expire() trace_timer_cancel() trace_itimer_cancel() trace_hrtimer_cancel() the init methods are missing from your patch entirely - and the rest is partially incomplete as well. Instrumentation of the del_timer() variants is missing. For a complete lifetime analysis of timers this cannot be left out. > +DEFINE_TRACE(timer_set); > +DEFINE_TRACE(timer_timeout); these two should be 'timer_start' and 'timer_expire'. > +DEFINE_TRACE(timer_update_time); This is a misnomer. This is in timer.c but is not a 'timer' tracepoint - this is a time of day tracepoint - and this should be reflected in its naming - so it should be: trace_gtod_update_xtime() or so. > @@ -357,6 +363,7 @@ static void internal_add_timer(struct tv > i = (expires >> (TVR_BITS + 3 * TVN_BITS)) & TVN_MASK; > vec = base->tv5.vec + i; > } > + trace_timer_set(timer); > /* > * Timers are FIFO: > */ this should be trace_timer_start(). > @@ -1121,6 +1128,7 @@ void do_timer(unsigned long ticks) > { > jiffies_64 += ticks; > update_times(ticks); > + trace_timer_update_time(&xtime, &wall_to_monotonic); > } gtod_update_xtime(). The GTOD instrumentation should move into a separate patch, and should probably be extended with NTP correction events as well. > #ifdef __ARCH_WANT_SYS_ALARM > @@ -1202,7 +1210,9 @@ SYSCALL_DEFINE0(getegid) > > static void process_timeout(unsigned long __data) > { > - wake_up_process((struct task_struct *)__data); > + struct task_struct *task = (struct task_struct *)__data; > + trace_timer_timeout(task); > + wake_up_process(task); [ nit: misssing new line after local variable declaration. ] this should be trace_timer_expire(). > Index: linux-2.6-lttng/include/trace/timer.h > =================================================================== > --- /dev/null 1970-01-01 00:00:00.000000000 +0000 > +++ linux-2.6-lttng/include/trace/timer.h 2009-03-24 09:31:51.000000000 -0400 > @@ -0,0 +1,24 @@ > +#ifndef _TRACE_TIMER_H > +#define _TRACE_TIMER_H > + > +#include > + > +DECLARE_TRACE(timer_itimer_expired, > + TPPROTO(struct signal_struct *sig), > + TPARGS(sig)); > +DECLARE_TRACE(timer_itimer_set, > + TPPROTO(int which, struct itimerval *value), > + TPARGS(which, value)); > +DECLARE_TRACE(timer_set, > + TPPROTO(struct timer_list *timer), > + TPARGS(timer)); > +/* > + * xtime_lock is taken when kernel_timer_update_time tracepoint is reached. > + */ > +DECLARE_TRACE(timer_update_time, > + TPPROTO(struct timespec *_xtime, struct timespec *_wall_to_monotonic), > + TPARGS(_xtime, _wall_to_monotonic)); > +DECLARE_TRACE(timer_timeout, > + TPPROTO(struct task_struct *p), > + TPARGS(p)); > +#endif For completeness and consistency, trace_itimer_start() should be split into two cases: if (value) trace_itimer_start(which, value); else trace_itimer_cancel(which); That extra branch does not matter much and the resulting instrumentaton is more consistent across all the timer types. So this patch needs more work, but it's a good first step. It would also be fantastic if you did them via the TRACE_EVENT() mechanism in the tracing tree. It would still be just as useful to LTTNG - but it would also be useful to ftrace that way. Thanks, Ingo