Re: [RFC][Patch 2/2] markers: example of irq regular kernel markers

[Mathieu Desnoyers <mathieu dot desnoyers at polymtl dot ca>]

* Masami Hiramatsu (mhiramat@redhat.com) wrote:
> Add trace points of irq handle events ported from LTTng's markers.
> 
> Signed-off-by: Masami Hiramatsu <mhiramat@redhat.com>
> ---
> I just rewrote LTTng's irq event by using DEFINE_TRACE for example.
> 
>  include/linux/irq_trace.h |    6 ++++++
>  kernel/irq/handle.c       |    6 ++++++
>  2 files changed, 12 insertions(+)
> 
> Index: 2.6.26-rc5-mm3/include/linux/irq_trace.h
> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ 2.6.26-rc5-mm3/include/linux/irq_trace.h	2008-06-16 12:27:51.000000000 -0400
> @@ -0,0 +1,6 @@
> +#include <linux/marker.h>
> +
> +DEFINE_TRACE(irq_entry, (int irq_id, int kernel_mode), irq_id, kernel_mode);
> +
> +DEFINE_TRACE(irq_exit, (void));
> +

All this work look good, thanks Masami! Sorry I did not find time to do
it lately, I've been busy on other things. A small question though :
since LTTng is configurable both as an external module or as an
in-kernel tracer, I wonder if it would really hurt to add the format
strings to DEFINE_TRACE, e.g. :

DEFINE_TRACE(name, prototype, format_string, args...)

which would give :

DEFINE_TRACE(irq_entry, (int irq_id, int kernel_mode), "%d %d",
    irq_id, kernel_mode);

DEFINE_TRACE(irq_exit, (void), MARK_NOARGS);

and calling this in the kernel code :

trace_irq_entry(irq, (regs)?(!user_mode(regs)):(1));
...
trace_irq_exit();

and for quick-and-dirty debug usage, one would add this to kernel code :

trace_mark(subsystem_event, "(int arg, struct task_struct *task)",
 "%d %p", arg, current);

By doing so, we could leave a gcc format string check by passing the
format string to __mark_check_format(). We could extract the field names
from the prototype, so there is no need to duplicate field information
in the format string.

Since the format string information is hidden in a header but kept at
the same location as the trace point definition, I think it reaches
goals of being "neat", efficient for general purpose tracers and to keep
the tracepoint information all in one place so we don't end up adding
stuff to various information consumers whenever a new trace point is
added.

What do you think of these changes ?

Mathieu


> Index: 2.6.26-rc5-mm3/kernel/irq/handle.c
> ===================================================================
> --- 2.6.26-rc5-mm3.orig/kernel/irq/handle.c	2008-06-16 12:27:50.000000000 -0400
> +++ 2.6.26-rc5-mm3/kernel/irq/handle.c	2008-06-16 12:27:51.000000000 -0400
> @@ -15,6 +15,7 @@
>  #include <linux/random.h>
>  #include <linux/interrupt.h>
>  #include <linux/kernel_stat.h>
> +#include <linux/irq_trace.h>
> 
>  #include "internals.h"
> 
> @@ -130,6 +131,9 @@ irqreturn_t handle_IRQ_event(unsigned in
>  {
>  	irqreturn_t ret, retval = IRQ_NONE;
>  	unsigned int status = 0;
> +	struct pt_regs *regs = get_irq_regs();
> +
> +	trace_irq_entry(irq, (regs)?(!user_mode(regs)):(1));
> 
>  	handle_dynamic_tick(action);
> 
> @@ -148,6 +152,8 @@ irqreturn_t handle_IRQ_event(unsigned in
>  		add_interrupt_randomness(irq);
>  	local_irq_disable();
> 
> +	trace_irq_exit();
> +
>  	return retval;
>  }
> 
> -- 
> Masami Hiramatsu
> 
> Software Engineer
> Hitachi Computer Products (America) Inc.
> Software Solutions Division
> 
> e-mail: mhiramat@redhat.com
> 

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