Re: systemtap broken by removal of register_timer_hook

[Frederic Weisbecker <fweisbec at gmail dot com>]

On Tue, Apr 30, 2013 at 01:27:20PM -0400, Frank Ch. Eigler wrote:
> Hi -
> 
> > [...]  How about creating trace_tick() in
> > include/trace/events/timer.h and call it from tick_periodic() and
> > tick_sched_handle(). [...]
> 
> Like this?
> 
> 
> From facee64445c0dcc717e99c474c5c7dcdd31b9a74 Mon Sep 17 00:00:00 2001
> From: "Frank Ch. Eigler" <fche@redhat.com>
> Date: Wed, 3 Apr 2013 10:35:21 -0400
> Subject: [PATCH] profiling: add tick tracepoint

I'd rather entitle "timer: Add tick tracepoint", as it doesn't depend
on the profiling stuff.

> 
> Commit ba6fdda4 removed the timer_hook mechanism for modules to listen
> to profiling timer ticks (without having to set up more complicated
> perf mechanisms).  To reduce the impact on out-of-tree users such as
> systemtap, a TRACE_EVENT-flavoured tracepoint is added in its place,
> invoked right beside profile_tick() in kernel/time/tick-*.c.
> Tested with perf and systemtap.
> 
> Cc: Frederic Weisbecker <fweisbec@gmail.com>
> Cc: Ingo Molnar <mingo@kernel.org>
> Cc: Mel Gorman <mgorman@suse.de>
> Signed-off-by: Frank Ch. Eigler <fche@redhat.com>
> ---
>  include/trace/events/timer.h | 20 ++++++++++++++++++++
>  kernel/time/tick-common.c    |  2 ++
>  kernel/time/tick-sched.c     |  2 ++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
> index 425bcfe..ec4c2d0 100644
> --- a/include/trace/events/timer.h
> +++ b/include/trace/events/timer.h
> @@ -323,6 +323,26 @@ TRACE_EVENT(itimer_expire,
>  		  (int) __entry->pid, (unsigned long long)__entry->now)
>  );
>  
> +
> +struct pt_regs;
> +
> +/**
> + * tick - called when the profiling timer ticks
> + * @regs:	pointer to struct pt_regs*
> + */
> +TRACE_EVENT(tick,
> +	TP_PROTO(struct pt_regs *regs),
> +	TP_ARGS(regs),
> +	TP_STRUCT__entry(
> +		__field( struct pt_regs*,	regs	)

I doubt the pointer to the regs is interesting in userland.
How about "void *ip" instead?

> +	),
> +	TP_fast_assign(
> +		__entry->regs	= regs;
> +	),
> +	TP_printk("ip=%p", (void *) instruction_pointer(__entry->regs))
> +);
> +
> +
>  #endif /*  _TRACE_TIMER_H */
>  
>  /* This part must be outside protection */
> diff --git a/kernel/time/tick-common.c b/kernel/time/tick-common.c
> index b1600a6..5f4227f 100644
> --- a/kernel/time/tick-common.c
> +++ b/kernel/time/tick-common.c
> @@ -18,6 +18,7 @@
>  #include <linux/percpu.h>
>  #include <linux/profile.h>
>  #include <linux/sched.h>
> +#include <trace/events/timer.h>
>  
>  #include <asm/irq_regs.h>
>  
> @@ -74,6 +75,7 @@ static void tick_periodic(int cpu)
>  
>  	update_process_times(user_mode(get_irq_regs()));
>  	profile_tick(CPU_PROFILING);
> +	trace_tick(get_irq_regs());

I suggest storing get_irq_regs() in a local variable so the function
doesn't fetch it twice from whatever per cpu area.

>  }
>  
>  /*
> diff --git a/kernel/time/tick-sched.c b/kernel/time/tick-sched.c
> index a19a399..447be56 100644
> --- a/kernel/time/tick-sched.c
> +++ b/kernel/time/tick-sched.c
> @@ -21,6 +21,7 @@
>  #include <linux/sched.h>
>  #include <linux/module.h>
>  #include <linux/irq_work.h>
> +#include <trace/events/timer.h>
>  
>  #include <asm/irq_regs.h>
>  
> @@ -140,6 +141,7 @@ static void tick_sched_handle(struct tick_sched *ts, struct pt_regs *regs)
>  #endif
>  	update_process_times(user_mode(regs));
>  	profile_tick(CPU_PROFILING);
> +	trace_tick(get_irq_regs());

And here you can reuse the regs parameter.

>  }
>  
>  /*
> -- 
> 1.8.2
>