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

[Masami Hiramatsu <mhiramat@redhat.com>]

Hi Mathieu,

Mathieu Desnoyers wrote:
> * 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);

why would you complicate it? I think.

trace_mark(subsystem_event, "arg %d task %p", arg, current);

is enough for user-defined markers.

> 
> 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.

I thought that someone complained against those format strings in
kernel code. Thus I removed it from DEFINE_TRACE.

even though, I think you can do that by adding below string table
to LTTng module.

const char *lookup_table[MAX_MARKERS][2] = {
{"irq_entry", "%d %d"}, // or "(int irq_id, int kernel_mode)", "%d %d"
...
};


> 
> 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.

Hmm, IMHO, there seems no difference between

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

and

trace_irq_entry(int irq_id, int kernel_mode)
{
	trace_mark(irq_entry, "%d %d", irq_id, kernel_mode);
}

for me. If so, we'd better use latter because of simplicity:-)

Thank you,


> 
> 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
>>
> 

-- 
Masami Hiramatsu

Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division

e-mail: mhiramat@redhat.com