| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <485C064E.5020705@redhat.com>
Date: Fri, 20 Jun 2008 15:34:38 -0400
From: Masami Hiramatsu <mhiramat@...hat.com>
To: Mathieu Desnoyers <mathieu.desnoyers@...ymtl.ca>
CC: Peter Zijlstra <peterz@...radead.org>,
Steven Rostedt <rostedt@...dmis.org>,
"Frank Ch. Eigler" <fche@...hat.com>, Ingo Molnar <mingo@...e.hu>,
LKML <linux-kernel@...r.kernel.org>,
systemtap-ml <systemtap@...rces.redhat.com>,
Hideo AOKI <haoki@...hat.com>
Subject: Re: [RFC][Patch 2/2] markers: example of irq regular kernel markers
Hi Mathieu,
Mathieu Desnoyers wrote:
> * Masami Hiramatsu (mhiramat@...hat.com) wrote:
>> Add trace points of irq handle events ported from LTTng's markers.
>>
>> Signed-off-by: Masami Hiramatsu <mhiramat@...hat.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@...hat.com
>>
>
--
Masami Hiramatsu
Software Engineer
Hitachi Computer Products (America) Inc.
Software Solutions Division
e-mail: mhiramat@...hat.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists