[PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Li Zefan]

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 <lizf@cn.fujitsu.com>
---
 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

[PATCH 2/2] tracing/events: Add kexec tracepoints

[Li Zefan]

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

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Daniel Walker]

On Wed, 2009-09-09 at 09:15 +0800, Li Zefan wrote:
> +       TP_STRUCT__entry(
> +               __string(       msg,    msg     )
> +       ),

Why the funny spacing here?

Daniel

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Li Zefan]

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.

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Daniel Walker]

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

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Steven Rostedt]

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

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Daniel Walker]

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

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Steven Rostedt]

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

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Daniel Walker]

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

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Eric W. Biederman]

Li Zefan <lizf@cn.fujitsu.com> 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

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Steven Rostedt]

On Tue, 2009-09-08 at 18:27 -0700, Eric W. Biederman wrote:
> Li Zefan <lizf@cn.fujitsu.com> 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.

Re: [PATCH 2/2] tracing/events: Add kexec tracepoints

[Eric W. Biederman]

Steven Rostedt <rostedt@goodmis.org> writes:

> On Tue, 2009-09-08 at 18:27 -0700, Eric W. Biederman wrote:
>> Li Zefan <lizf@cn.fujitsu.com> 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

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Steven Rostedt]

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 <lizf@cn.fujitsu.com>
> ---
>  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
>   *

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Li Zefan]

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

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Steven Rostedt]

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

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Li Zefan]

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

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Steven Rostedt]

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

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[KOSAKI Motohiro]

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

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Li Zefan]

>>>>>>> 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 <tglx@linutronix.de>
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.

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Steven Rostedt]

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 <tglx@linutronix.de>
> 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.
> 

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[KOSAKI Motohiro]

> 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 <tglx@linutronix.de>
> > 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.
> > 
> 

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[Steven Rostedt]

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

Re: [PATCH 1/2] tracing: Add sysctl to enable/disable tracing on oops

[KOSAKI Motohiro]

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