Message-ID: <20090324182122.GG31117@elte.hu>
Date:	Tue, 24 Mar 2009 19:21:22 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>,
	Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>
Cc:	akpm@...ux-foundation.org, linux-kernel@...r.kernel.org,
	ltt-dev@...ts.casi.polymtl.ca,
	Frederic Weisbecker <fweisbec@...il.com>,
	"David S. Miller" <davem@...emloft.net>,
	Masami Hiramatsu <mhiramat@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	"Frank Ch. Eigler" <fche@...hat.com>,
	Hideo AOKI <haoki@...hat.com>,
	Takashi Nishiie <t-nishiie@...css.fujitsu.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>
Subject: Re: [patch 6/9] LTTng instrumentation - timer


* Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca> 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' <rostedt@...dmis.org>
> CC: Eduard - Gabriel Munteanu <eduard.munteanu@...ux360.ro>
> ---
>  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 <linux/tracepoint.h>
> +
> +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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists