Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754522AbYFTTge (ORCPT ); Fri, 20 Jun 2008 15:36:34 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1751471AbYFTTg0 (ORCPT ); Fri, 20 Jun 2008 15:36:26 -0400 Received: from mx1.redhat.com ([66.187.233.31]:33098 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751419AbYFTTgZ (ORCPT ); Fri, 20 Jun 2008 15:36:25 -0400 Message-ID: <485C064E.5020705@redhat.com> Date: Fri, 20 Jun 2008 15:34:38 -0400 From: Masami Hiramatsu User-Agent: Thunderbird 2.0.0.14 (X11/20080501) MIME-Version: 1.0 To: Mathieu Desnoyers CC: Peter Zijlstra , Steven Rostedt , "Frank Ch. Eigler" , Ingo Molnar , LKML , systemtap-ml , Hideo AOKI Subject: Re: [RFC][Patch 2/2] markers: example of irq regular kernel markers References: <485BE2C6.1080901@redhat.com> <20080620174529.GB10943@Krystal> In-Reply-To: <20080620174529.GB10943@Krystal> X-Enigmail-Version: 0.95.6 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4679 Lines: 163 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 >> --- >> 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 >> + >> +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 >> #include >> #include >> +#include >> >> #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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/