Messages in this thread Patch in this message | | | Date | Fri, 20 Jun 2008 15:34:38 -0400 | From | Masami Hiramatsu <> | Subject | Re: [RFC][Patch 2/2] markers: example of irq regular kernel markers |
| |
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
| |