Currently we always disable tracing on oops, and this patch
adds a sysctl so one can choose to enable it.
Signed-off-by: Li Zefan <[email protected]>
---
include/linux/kernel.h | 2 ++
kernel/panic.c | 3 ++-
kernel/sysctl.c | 10 ++++++++++
kernel/trace/ring_buffer.c | 2 ++
4 files changed, 16 insertions(+), 1 deletions(-)
diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f61039e..5c4528f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -516,12 +516,14 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
* Most likely, you want to use tracing_on/tracing_off.
*/
#ifdef CONFIG_RING_BUFFER
+extern int trace_on_oops;
void tracing_on(void);
void tracing_off(void);
/* trace_off_permanent stops recording with no way to bring it back */
void tracing_off_permanent(void);
int tracing_is_on(void);
#else
+#define trace_on_oops 0
static inline void tracing_on(void) { }
static inline void tracing_off(void) { }
static inline void tracing_off_permanent(void) { }
diff --git a/kernel/panic.c b/kernel/panic.c
index 512ab73..22ec502 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -301,7 +301,8 @@ int oops_may_print(void)
*/
void oops_enter(void)
{
- tracing_off();
+ if (!trace_on_oops)
+ tracing_off();
/* can't trust the integrity of the kernel anymore: */
debug_locks_off();
do_oops_enter_exit();
diff --git a/kernel/sysctl.c b/kernel/sysctl.c
index cdbe8d0..a768c15 100644
--- a/kernel/sysctl.c
+++ b/kernel/sysctl.c
@@ -537,6 +537,16 @@ static struct ctl_table kern_table[] = {
.proc_handler = &stack_trace_sysctl,
},
#endif
+#ifdef CONFIG_RING_BUFFER
+ {
+ .ctl_name = CTL_UNNUMBERED,
+ .procname = "trace_on_oops",
+ .data = &trace_on_oops,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = &proc_dointvec,
+ },
+#endif
#ifdef CONFIG_TRACING
{
.ctl_name = CTL_UNNUMBERED,
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 454e74e..fc10962 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -155,6 +155,8 @@ static unsigned long ring_buffer_flags __read_mostly = RB_BUFFERS_ON;
#define BUF_PAGE_HDR_SIZE offsetof(struct buffer_data_page, data)
+int trace_on_oops;
+
/**
* tracing_on - enable all tracing buffers
*
--
1.6.3
When panic, save in ring buffer the time when crash happened,
the panic message, etc. And the data can be retrieved from
core dump file using flight-recorder, which is going to be a
module of the crash utility.
Also if we implement flight-recorder in perf tool, those trace
outputs can be used by perf tool.
---
include/trace/events/kexec.h | 51 ++++++++++++++++++++++++++++++++++++++++++
kernel/kexec.c | 4 +++
kernel/panic.c | 4 +++
3 files changed, 59 insertions(+), 0 deletions(-)
create mode 100644 include/trace/events/kexec.h
diff --git a/include/trace/events/kexec.h b/include/trace/events/kexec.h
new file mode 100644
index 0000000..c1d740a
--- /dev/null
+++ b/include/trace/events/kexec.h
@@ -0,0 +1,51 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM kexec
+
+#if !defined(_TRACE_KEXEC_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_KEXEC_H
+
+#include <linux/kexec.h>
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(panic,
+
+ TP_PROTO(const char *msg),
+
+ TP_ARGS(msg),
+
+ TP_STRUCT__entry(
+ __string( msg, msg )
+ ),
+
+ TP_fast_assign(
+ __assign_str(msg, msg);
+ ),
+
+ TP_printk("%s", __get_str(msg))
+);
+
+TRACE_EVENT(crash_kexec,
+
+ TP_PROTO(struct kimage *image, struct pt_regs *regs),
+
+ TP_ARGS(image, regs),
+
+ TP_STRUCT__entry(
+ __field( void *, image )
+ __field( void *, ip )
+ ),
+
+ TP_fast_assign(
+ __entry->image = image;
+ __entry->ip = regs ?
+ (void *)instruction_pointer(regs) : NULL;
+ ),
+
+ TP_printk("image=%p, ip=%p", __entry->image, __entry->ip)
+);
+
+#endif /* _TRACE_KEXEC_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
+
diff --git a/kernel/kexec.c b/kernel/kexec.c
index f336e21..3d7eda7 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -38,6 +38,8 @@
#include <asm/system.h>
#include <asm/sections.h>
+#include <trace/events/kexec.h>
+
/* Per cpu memory for storing cpu states in case of system crash. */
note_buf_t* crash_notes;
@@ -1073,6 +1075,8 @@ void crash_kexec(struct pt_regs *regs)
if (mutex_trylock(&kexec_mutex)) {
if (kexec_crash_image) {
struct pt_regs fixed_regs;
+
+ trace_crash_kexec(kexec_crash_image, regs);
crash_setup_regs(&fixed_regs, regs);
crash_save_vmcoreinfo();
machine_crash_shutdown(&fixed_regs);
diff --git a/kernel/panic.c b/kernel/panic.c
index 22ec502..c3baa23 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -23,6 +23,9 @@
#include <linux/nmi.h>
#include <linux/dmi.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/kexec.h>
+
int panic_on_oops;
static unsigned long tainted_mask;
static int pause_on_oops;
@@ -69,6 +72,7 @@ NORET_TYPE void panic(const char * fmt, ...)
va_start(args, fmt);
vsnprintf(buf, sizeof(buf), fmt, args);
va_end(args);
+ trace_panic(buf);
printk(KERN_EMERG "Kernel panic - not syncing: %s\n",buf);
#ifdef CONFIG_DEBUG_BUGVERBOSE
dump_stack();
--
1.6.3
On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> + TP_STRUCT__entry(
> + __string( msg, msg )
> + ),
Why the funny spacing here?
Daniel
Daniel Walker wrote:
> On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
>> + TP_STRUCT__entry(
>> + __string( msg, msg )
>> + ),
>
> Why the funny spacing here?
>
To make the code better-looking.
This is the coding-style we use for the code in
include/trace/events/*.
Run scripts/checkpatch.pl to include/trace/events/foo.h,
you'll get a bunch of ERRORs. We ignore them, because the
script is not applicable here.
Li Zefan <[email protected]> writes:
> When panic, save in ring buffer the time when crash happened,
> the panic message, etc. And the data can be retrieved from
> core dump file using flight-recorder, which is going to be a
> module of the crash utility.
>
> Also if we implement flight-recorder in perf tool, those trace
> outputs can be used by perf tool.
Please let's keep dynamic code out of crash_kexec if we can.
If this is just about timestamsp I don't see the point.
Eric
> ---
> include/trace/events/kexec.h | 51 ++++++++++++++++++++++++++++++++++++++++++
> kernel/kexec.c | 4 +++
> kernel/panic.c | 4 +++
> 3 files changed, 59 insertions(+), 0 deletions(-)
> create mode 100644 include/trace/events/kexec.h
>
> diff --git a/include/trace/events/kexec.h b/include/trace/events/kexec.h
> new file mode 100644
> index 0000000..c1d740a
> --- /dev/null
> +++ b/include/trace/events/kexec.h
> @@ -0,0 +1,51 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM kexec
> +
> +#if !defined(_TRACE_KEXEC_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_KEXEC_H
> +
> +#include <linux/kexec.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(panic,
> +
> + TP_PROTO(const char *msg),
> +
> + TP_ARGS(msg),
> +
> + TP_STRUCT__entry(
> + __string( msg, msg )
> + ),
> +
> + TP_fast_assign(
> + __assign_str(msg, msg);
> + ),
> +
> + TP_printk("%s", __get_str(msg))
> +);
> +
> +TRACE_EVENT(crash_kexec,
> +
> + TP_PROTO(struct kimage *image, struct pt_regs *regs),
> +
> + TP_ARGS(image, regs),
> +
> + TP_STRUCT__entry(
> + __field( void *, image )
> + __field( void *, ip )
> + ),
> +
> + TP_fast_assign(
> + __entry->image = image;
> + __entry->ip = regs ?
> + (void *)instruction_pointer(regs) : NULL;
> + ),
> +
> + TP_printk("image=%p, ip=%p", __entry->image, __entry->ip)
> +);
> +
> +#endif /* _TRACE_KEXEC_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> +
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index f336e21..3d7eda7 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -38,6 +38,8 @@
> #include <asm/system.h>
> #include <asm/sections.h>
>
> +#include <trace/events/kexec.h>
> +
> /* Per cpu memory for storing cpu states in case of system crash. */
> note_buf_t* crash_notes;
>
> @@ -1073,6 +1075,8 @@ void crash_kexec(struct pt_regs *regs)
> if (mutex_trylock(&kexec_mutex)) {
> if (kexec_crash_image) {
> struct pt_regs fixed_regs;
> +
> + trace_crash_kexec(kexec_crash_image, regs);
> crash_setup_regs(&fixed_regs, regs);
> crash_save_vmcoreinfo();
> machine_crash_shutdown(&fixed_regs);
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 22ec502..c3baa23 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -23,6 +23,9 @@
> #include <linux/nmi.h>
> #include <linux/dmi.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/kexec.h>
> +
> int panic_on_oops;
> static unsigned long tainted_mask;
> static int pause_on_oops;
> @@ -69,6 +72,7 @@ NORET_TYPE void panic(const char * fmt, ...)
> va_start(args, fmt);
> vsnprintf(buf, sizeof(buf), fmt, args);
> va_end(args);
> + trace_panic(buf);
> printk(KERN_EMERG "Kernel panic - not syncing: %s\n",buf);
> #ifdef CONFIG_DEBUG_BUGVERBOSE
> dump_stack();
> --
> 1.6.3
On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> Currently we always disable tracing on oops, and this patch
> adds a sysctl so one can choose to enable it.
Hmm, we already have a way to enable it.
# echo 1 > /debug/tracing/tracing_on
-- Steve
>
> Signed-off-by: Li Zefan <[email protected]>
> ---
> include/linux/kernel.h | 2 ++
> kernel/panic.c | 3 ++-
> kernel/sysctl.c | 10 ++++++++++
> kernel/trace/ring_buffer.c | 2 ++
> 4 files changed, 16 insertions(+), 1 deletions(-)
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index f61039e..5c4528f 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -516,12 +516,14 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
> * Most likely, you want to use tracing_on/tracing_off.
> */
> #ifdef CONFIG_RING_BUFFER
> +extern int trace_on_oops;
> void tracing_on(void);
> void tracing_off(void);
> /* trace_off_permanent stops recording with no way to bring it back */
> void tracing_off_permanent(void);
> int tracing_is_on(void);
> #else
> +#define trace_on_oops 0
> static inline void tracing_on(void) { }
> static inline void tracing_off(void) { }
> static inline void tracing_off_permanent(void) { }
> diff --git a/kernel/panic.c b/kernel/panic.c
> index 512ab73..22ec502 100644
> --- a/kernel/panic.c
> +++ b/kernel/panic.c
> @@ -301,7 +301,8 @@ int oops_may_print(void)
> */
> void oops_enter(void)
> {
> - tracing_off();
> + if (!trace_on_oops)
> + tracing_off();
> /* can't trust the integrity of the kernel anymore: */
> debug_locks_off();
> do_oops_enter_exit();
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index cdbe8d0..a768c15 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -537,6 +537,16 @@ static struct ctl_table kern_table[] = {
> .proc_handler = &stack_trace_sysctl,
> },
> #endif
> +#ifdef CONFIG_RING_BUFFER
> + {
> + .ctl_name = CTL_UNNUMBERED,
> + .procname = "trace_on_oops",
> + .data = &trace_on_oops,
> + .maxlen = sizeof(int),
> + .mode = 0644,
> + .proc_handler = &proc_dointvec,
> + },
> +#endif
> #ifdef CONFIG_TRACING
> {
> .ctl_name = CTL_UNNUMBERED,
> diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
> index 454e74e..fc10962 100644
> --- a/kernel/trace/ring_buffer.c
> +++ b/kernel/trace/ring_buffer.c
> @@ -155,6 +155,8 @@ static unsigned long ring_buffer_flags __read_mostly = RB_BUFFERS_ON;
>
> #define BUF_PAGE_HDR_SIZE offsetof(struct buffer_data_page, data)
>
> +int trace_on_oops;
> +
> /**
> * tracing_on - enable all tracing buffers
> *
Steven Rostedt wrote:
> On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
>> Currently we always disable tracing on oops, and this patch
>> adds a sysctl so one can choose to enable it.
>
> Hmm, we already have a way to enable it.
>
> # echo 1 > /debug/tracing/tracing_on
>
What I want is a way to not disable it when an oops happened. :)
On Tue, 2009-09-08 at 18:27 -0700, Eric W. Biederman wrote:
> Li Zefan <[email protected]> writes:
>
> > When panic, save in ring buffer the time when crash happened,
> > the panic message, etc. And the data can be retrieved from
> > core dump file using flight-recorder, which is going to be a
> > module of the crash utility.
> >
> > Also if we implement flight-recorder in perf tool, those trace
> > outputs can be used by perf tool.
>
> Please let's keep dynamic code out of crash_kexec if we can.
What dynamic code? It's a tracepoint not something like the function
tracer. It is activated via a bit test and a jmp. Not very dynamic.
-- Steve
>
> If this is just about timestamsp I don't see the point.
On Wed, 2009-09-09 at 09:37 +0800, Li Zefan wrote:
> Steven Rostedt wrote:
> > On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> >> Currently we always disable tracing on oops, and this patch
> >> adds a sysctl so one can choose to enable it.
> >
> > Hmm, we already have a way to enable it.
> >
> > # echo 1 > /debug/tracing/tracing_on
> >
>
> What I want is a way to not disable it when an oops happened. :)
>
Ah, I misunderstood. May I ask a silly question?
Why?
-- Steve
Steven Rostedt wrote:
> On Wed, 2009-09-09 at 09:37 +0800, Li Zefan wrote:
>> Steven Rostedt wrote:
>>> On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
>>>> Currently we always disable tracing on oops, and this patch
>>>> adds a sysctl so one can choose to enable it.
>>> Hmm, we already have a way to enable it.
>>>
>>> # echo 1 > /debug/tracing/tracing_on
>>>
>> What I want is a way to not disable it when an oops happened. :)
>>
>
> Ah, I misunderstood. May I ask a silly question?
>
> Why?
>
Otherwise we won't get trace output from trae_crash_kexec if
crash_kexec() is not called by panic(). For example:
oops_begin()
->trace_off()
->panic_on_oops
->kexec_should_crash()
->crash_kexec()
Steven Rostedt <[email protected]> writes:
> On Tue, 2009-09-08 at 18:27 -0700, Eric W. Biederman wrote:
>> Li Zefan <[email protected]> writes:
>>
>> > When panic, save in ring buffer the time when crash happened,
>> > the panic message, etc. And the data can be retrieved from
>> > core dump file using flight-recorder, which is going to be a
>> > module of the crash utility.
>> >
>> > Also if we implement flight-recorder in perf tool, those trace
>> > outputs can be used by perf tool.
>>
>> Please let's keep dynamic code out of crash_kexec if we can.
>
> What dynamic code? It's a tracepoint not something like the function
> tracer. It is activated via a bit test and a jmp. Not very dynamic.
The philosophy of the crash dump code is that the kernel is broken
trust as little as possible.
Within a few seconds we should be able to get the time from the kernel
that saves the crash dump so I don't see how adding a trace point
adds anything into that code path except an unnecessary dependency
that might have broken.
Or in short you have a full user space to add features to. Please
don't features to the critical kernel magic that gets you to userspace.
Eric
On Wed, 2009-09-09 at 09:47 +0800, Li Zefan wrote:
> Steven Rostedt wrote:
> > On Wed, 2009-09-09 at 09:37 +0800, Li Zefan wrote:
> >> Steven Rostedt wrote:
> >>> On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> >>>> Currently we always disable tracing on oops, and this patch
> >>>> adds a sysctl so one can choose to enable it.
> >>> Hmm, we already have a way to enable it.
> >>>
> >>> # echo 1 > /debug/tracing/tracing_on
> >>>
> >> What I want is a way to not disable it when an oops happened. :)
> >>
> >
> > Ah, I misunderstood. May I ask a silly question?
> >
> > Why?
> >
>
> Otherwise we won't get trace output from trae_crash_kexec if
> crash_kexec() is not called by panic(). For example:
>
> oops_begin()
> ->trace_off()
> ->panic_on_oops
> ->kexec_should_crash()
> ->crash_kexec()
OK, but I'm not exactly sure what you final goal is here. To have a
something to search for in the ring buffer after the crash? Maybe
instead we can add a "trace_oops" event? Just put it before the
tracing_off call.
-- Steve
> On Wed, 2009-09-09 at 09:47 +0800, Li Zefan wrote:
> > Steven Rostedt wrote:
> > > On Wed, 2009-09-09 at 09:37 +0800, Li Zefan wrote:
> > >> Steven Rostedt wrote:
> > >>> On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> > >>>> Currently we always disable tracing on oops, and this patch
> > >>>> adds a sysctl so one can choose to enable it.
> > >>> Hmm, we already have a way to enable it.
> > >>>
> > >>> # echo 1 > /debug/tracing/tracing_on
> > >>>
> > >> What I want is a way to not disable it when an oops happened. :)
> > >>
> > >
> > > Ah, I misunderstood. May I ask a silly question?
> > >
> > > Why?
> > >
> >
> > Otherwise we won't get trace output from trae_crash_kexec if
> > crash_kexec() is not called by panic(). For example:
> >
> > oops_begin()
> > ->trace_off()
> > ->panic_on_oops
> > ->kexec_should_crash()
> > ->crash_kexec()
>
> OK, but I'm not exactly sure what you final goal is here. To have a
> something to search for in the ring buffer after the crash? Maybe
> instead we can add a "trace_oops" event? Just put it before the
> tracing_off call.
I have another silly question.
Why should we call tracing_off() in oops_enter()?
1. Oops behavior depend on panic_on_oops sysctl.
Should we disable trace both case?
2. Can I think the code treat to save oops in tracing code?
or it have more deeper intention?
Can anyone please explain it?
>>>>>>> Currently we always disable tracing on oops, and this patch
>>>>>>> adds a sysctl so one can choose to enable it.
>>>>>> Hmm, we already have a way to enable it.
>>>>>>
>>>>>> # echo 1 > /debug/tracing/tracing_on
>>>>>>
>>>>> What I want is a way to not disable it when an oops happened. :)
>>>>>
>>>> Ah, I misunderstood. May I ask a silly question?
>>>>
>>>> Why?
>>>>
>>> Otherwise we won't get trace output from trae_crash_kexec if
>>> crash_kexec() is not called by panic(). For example:
>>>
>>> oops_begin()
>>> ->trace_off()
>>> ->panic_on_oops
>>> ->kexec_should_crash()
>>> ->crash_kexec()
>> OK, but I'm not exactly sure what you final goal is here. To have a
>> something to search for in the ring buffer after the crash? Maybe
>> instead we can add a "trace_oops" event? Just put it before the
>> tracing_off call.
>
> I have another silly question.
> Why should we call tracing_off() in oops_enter()?
>
I guess it's because trace outputs generated during oops can
overwrite/mess up those generated before oops?
It was added by this commit, but I can't find trace_printk_on_oops.
commit bdff78707f3ce47e891f3201c9666122a70556ce
Author: Thomas Gleixner <[email protected]>
Date: Fri Jul 24 15:30:45 2009 -0400
trace: stop tracer in oops_enter()
If trace_printk_on_oops is set we lose interesting trace information
when the tracer is enabled across oops handling and printing. We want
the trace which might give us information _WHY_ we oopsed.
On Wed, 2009-09-09 at 10:40 +0800, Li Zefan wrote:
> > I have another silly question.
> > Why should we call tracing_off() in oops_enter()?
> >
>
> I guess it's because trace outputs generated during oops can
> overwrite/mess up those generated before oops?
>
> It was added by this commit, but I can't find trace_printk_on_oops.
That's because Thomas did not bother looking up the actual variable he
was talking about. s/trace_printk_on_oops/ftrace_dump_on_oops/
-- Steve
>
> commit bdff78707f3ce47e891f3201c9666122a70556ce
> Author: Thomas Gleixner <[email protected]>
> Date: Fri Jul 24 15:30:45 2009 -0400
>
> trace: stop tracer in oops_enter()
>
> If trace_printk_on_oops is set we lose interesting trace information
> when the tracer is enabled across oops handling and printing. We want
> the trace which might give us information _WHY_ we oopsed.
>
> > I have another silly question.
> > Why should we call tracing_off() in oops_enter()?
>
> I guess it's because trace outputs generated during oops can
> overwrite/mess up those generated before oops?
>
> It was added by this commit, but I can't find trace_printk_on_oops.
Ah, thanks.
I guess trace_printk_on_oops mean ftrace_dump_on_oops.
> On Wed, 2009-09-09 at 10:40 +0800, Li Zefan wrote:
>
> > > I have another silly question.
> > > Why should we call tracing_off() in oops_enter()?
> > >
> >
> > I guess it's because trace outputs generated during oops can
> > overwrite/mess up those generated before oops?
> >
> > It was added by this commit, but I can't find trace_printk_on_oops.
>
> That's because Thomas did not bother looking up the actual variable he
> was talking about. s/trace_printk_on_oops/ftrace_dump_on_oops/
After a bit thinking, I think ftrace_dump_on_oops and kernel dump with panic have
very similar requirement.
Then, I think we can reuse this infrastructure for kernel dump.
Requirement
- Need to know why the problem occur.
- Need to don't logging oops (panic) internal.
Unfortunately, panic() call panic_notifier after crash_kexec().
it mean tracing_off() was not called when panic on the machine w/ kernel-dump.
NORET_TYPE void panic(const char * fmt, ...)
{
(snip)
crash_kexec(NULL);
(snip)
atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
I think we can insert tracing_off() into panic() directly. it only
bit mask operation, it mean it don't have side effect risk.
Perhaps, There are other kernel dump specific requrement. but I guess
it's very small. Maybe it can be handled by trace_die_handler() or something like.
What do you think?
>
> -- Steve
>
> >
> > commit bdff78707f3ce47e891f3201c9666122a70556ce
> > Author: Thomas Gleixner <[email protected]>
> > Date: Fri Jul 24 15:30:45 2009 -0400
> >
> > trace: stop tracer in oops_enter()
> >
> > If trace_printk_on_oops is set we lose interesting trace information
> > when the tracer is enabled across oops handling and printing. We want
> > the trace which might give us information _WHY_ we oopsed.
> >
>
On Wed, 2009-09-09 at 09:26 +0800, Li Zefan wrote:
> Daniel Walker wrote:
> > On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> >> + TP_STRUCT__entry(
> >> + __string( msg, msg )
> >> + ),
> >
> > Why the funny spacing here?
> >
>
> To make the code better-looking.
>
> This is the coding-style we use for the code in
> include/trace/events/*.
It's part of Linux right? We already have a coding style ..
BTW, you patch had no signed off by ..
Daniel
On Tue, 2009-09-08 at 21:59 -0700, Daniel Walker wrote:
> On Wed, 2009-09-09 at 09:26 +0800, Li Zefan wrote:
> > Daniel Walker wrote:
> > > On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> > >> + TP_STRUCT__entry(
> > >> + __string( msg, msg )
> > >> + ),
> > >
> > > Why the funny spacing here?
> > >
> >
> > To make the code better-looking.
> >
> > This is the coding-style we use for the code in
> > include/trace/events/*.
>
> It's part of Linux right? We already have a coding style ..
It's a special macro. What are you now, part of the style police?
-- Steve
On Wed, 2009-09-09 at 13:19 +0900, KOSAKI Motohiro wrote:
> > On Wed, 2009-09-09 at 10:40 +0800, Li Zefan wrote:
> >
> > > > I have another silly question.
> > > > Why should we call tracing_off() in oops_enter()?
> > > >
> > >
> > > I guess it's because trace outputs generated during oops can
> > > overwrite/mess up those generated before oops?
> > >
> > > It was added by this commit, but I can't find trace_printk_on_oops.
> >
> > That's because Thomas did not bother looking up the actual variable he
> > was talking about. s/trace_printk_on_oops/ftrace_dump_on_oops/
>
> After a bit thinking, I think ftrace_dump_on_oops and kernel dump with panic have
> very similar requirement.
> Then, I think we can reuse this infrastructure for kernel dump.
>
> Requirement
> - Need to know why the problem occur.
> - Need to don't logging oops (panic) internal.
>
> Unfortunately, panic() call panic_notifier after crash_kexec().
> it mean tracing_off() was not called when panic on the machine w/ kernel-dump.
>
> NORET_TYPE void panic(const char * fmt, ...)
> {
> (snip)
> crash_kexec(NULL);
> (snip)
> atomic_notifier_call_chain(&panic_notifier_list, 0, buf);
>
> I think we can insert tracing_off() into panic() directly. it only
> bit mask operation, it mean it don't have side effect risk.
>
> Perhaps, There are other kernel dump specific requrement. but I guess
> it's very small. Maybe it can be handled by trace_die_handler() or something like.
>
> What do you think?
Yes, a tracing_off() should be in the beginning of panic. I had a patch
to do this, but I never sent it out :-/
-- Steve
On Wed, 2009-09-09 at 07:46 -0400, Steven Rostedt wrote:
> On Tue, 2009-09-08 at 21:59 -0700, Daniel Walker wrote:
> > On Wed, 2009-09-09 at 09:26 +0800, Li Zefan wrote:
> > > Daniel Walker wrote:
> > > > On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> > > >> + TP_STRUCT__entry(
> > > >> + __string( msg, msg )
> > > >> + ),
> > > >
> > > > Why the funny spacing here?
> > > >
> > >
> > > To make the code better-looking.
> > >
> > > This is the coding-style we use for the code in
> > > include/trace/events/*.
> >
> > It's part of Linux right? We already have a coding style ..
>
> It's a special macro. What are you now, part of the style police?
I'm just like everyone else, someone who asks questions and makes
comments .. By using a different style than what the rest of Linux uses
your putting yourself at a disadvantage since you can't easily use
checkpatch on that code (even the stuff that is compliant) ..
Not to mention people might start sending you clean ups that you maybe
won't accept. You could always modify checkpatch to recognize your
style.
Daniel
On Wed, 2009-09-09 at 07:12 -0700, Daniel Walker wrote:
> On Wed, 2009-09-09 at 07:46 -0400, Steven Rostedt wrote:
> > On Tue, 2009-09-08 at 21:59 -0700, Daniel Walker wrote:
> > > On Wed, 2009-09-09 at 09:26 +0800, Li Zefan wrote:
> > > > Daniel Walker wrote:
> > > > > On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> > > > >> + TP_STRUCT__entry(
> > > > >> + __string( msg, msg )
> > > > >> + ),
> > > > >
> > > > > Why the funny spacing here?
> > > > >
> > > >
> > > > To make the code better-looking.
> > > >
> > > > This is the coding-style we use for the code in
> > > > include/trace/events/*.
> > >
> > > It's part of Linux right? We already have a coding style ..
> >
> > It's a special macro. What are you now, part of the style police?
>
> I'm just like everyone else, someone who asks questions and makes
> comments .. By using a different style than what the rest of Linux uses
> your putting yourself at a disadvantage since you can't easily use
> checkpatch on that code (even the stuff that is compliant) ..
I'm fine with questions, but yours sounded a bit more cynical
>
> Not to mention people might start sending you clean ups that you maybe
> won't accept. You could always modify checkpatch to recognize your
> style.
Not sure this is worthy of modifying checkpatch. I haven't looked at
what that would take.
Here's an example of what the code currently looks like:
TRACE_EVENT(sched_wait_task,
TP_PROTO(struct rq *rq, struct task_struct *p),
TP_ARGS(rq, p),
TP_STRUCT__entry(
__array( char, comm, TASK_COMM_LEN )
__field( pid_t, pid )
__field( int, prio )
),
TP_fast_assign(
memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
__entry->pid = p->pid;
__entry->prio = p->prio;
),
TP_printk("task %s:%d [%d]",
__entry->comm, __entry->pid, __entry->prio)
);
And here's that same code with normal Linux style:
TRACE_EVENT(sched_wait_task,
TP_PROTO(struct rq *rq, struct task_struct *p),
TP_ARGS(rq, p),
TP_STRUCT__entry(__array(char, comm, TASK_COMM_LEN)
__field(pid_t, pid)
__field(int, prio)),
TP_fast_assign(memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
__entry->pid = p->pid;
__entry->prio = p->prio;
),
TP_printk("task %s:%d [%d]",
__entry->comm, __entry->pid, __entry->prio));
Not as nice to work with. The include/trace/events/ directory is quite
different than the rest of the kernel code. If checkpatch fails on it,
that's fine. Any changes here really need to be examined by eye anyway.
Having the extra spaces actually helps to see what is there.
Note, most changes in include/trace/ftrace.h failed checkpatch
horribly. ;-) checkpatch does not handle crazy macros well.
-- Steve
On Wed, 2009-09-09 at 11:19 -0400, Steven Rostedt wrote:
> On Wed, 2009-09-09 at 07:12 -0700, Daniel Walker wrote:
> > On Wed, 2009-09-09 at 07:46 -0400, Steven Rostedt wrote:
> > > On Tue, 2009-09-08 at 21:59 -0700, Daniel Walker wrote:
> > > > On Wed, 2009-09-09 at 09:26 +0800, Li Zefan wrote:
> > > > > Daniel Walker wrote:
> > > > > > On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> > > > > >> + TP_STRUCT__entry(
> > > > > >> + __string( msg, msg )
> > > > > >> + ),
> > > > > >
> > > > > > Why the funny spacing here?
> > > > > >
> > > > >
> > > > > To make the code better-looking.
> > > > >
> > > > > This is the coding-style we use for the code in
> > > > > include/trace/events/*.
> > > >
> > > > It's part of Linux right? We already have a coding style ..
> > >
> > > It's a special macro. What are you now, part of the style police?
> >
> > I'm just like everyone else, someone who asks questions and makes
> > comments .. By using a different style than what the rest of Linux uses
> > your putting yourself at a disadvantage since you can't easily use
> > checkpatch on that code (even the stuff that is compliant) ..
>
> I'm fine with questions, but yours sounded a bit more cynical
I'm never cynical (not on purpose anyway). I was just saying it like it
is ..
> And here's that same code with normal Linux style:
>
> TRACE_EVENT(sched_wait_task,
> TP_PROTO(struct rq *rq, struct task_struct *p),
> TP_ARGS(rq, p),
> TP_STRUCT__entry(__array(char, comm, TASK_COMM_LEN)
> __field(pid_t, pid)
> __field(int, prio)),
> TP_fast_assign(memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
> __entry->pid = p->pid;
> __entry->prio = p->prio;
> ),
> TP_printk("task %s:%d [%d]",
> __entry->comm, __entry->pid, __entry->prio));
The below is checkpatch clean ..
TRACE_EVENT(sched_wait_task,
TP_PROTO(struct rq *rq, struct task_struct *p),
TP_ARGS(rq, p),
TP_STRUCT__entry(
__array(char, comm, TASK_COMM_LEN)
__field(pid_t, pid)
__field(int, prio)
),
TP_fast_assign(
memcpy(__entry->comm, p->comm, TASK_COMM_LEN);
__entry->pid = p->pid;
__entry->prio = p->prio;
),
TP_printk("task %s:%d [%d]",
__entry->comm, __entry->pid, __entry->prio)
);
That's not radically different from what you currently have .. You could
still align the fields with tabs. You just have to remove the
starting/ending tabs..
Daniel