[PATCH 1/3 v4] Add tracing_off_event() to stop tracing when a bug or warning occur

[PATCH 1/3 v4] Add tracing_off_event() to stop tracing when a bug or warning occur

[Chase Douglas]

The tracing_off_event() function calls tracing_off() to stop tracing
when an event occurs. By default, only BUG-type events stop tracing,
while WARNING type events do not. This is controlled through the
tracing_off={none,warn,bug} commandline parameter.

Call this function from bug and warning event handlers to enable a user
to debug their kernel by starting a trace, hitting an event, and then
retrieving trace info knowing that the trace was stopped right after the
event was hit.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 include/linux/kernel.h     |    7 +++++++
 kernel/trace/ring_buffer.c |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 9365227..80c67ad 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -478,16 +478,23 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
  *
  * Most likely, you want to use tracing_on/tracing_off.
  */
+enum trace_off_event {
+	TRACE_EVENT_BUG = 0,
+	TRACE_EVENT_WARN,
+};
+
 #ifdef CONFIG_RING_BUFFER
 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);
+void tracing_off_event(enum trace_off_event event);
 int tracing_is_on(void);
 #else
 static inline void tracing_on(void) { }
 static inline void tracing_off(void) { }
 static inline void tracing_off_permanent(void) { }
+static inline void tracing_off_event(enum trace_off_event event) { }
 static inline int tracing_is_on(void) { return 0; }
 #endif
 #ifdef CONFIG_TRACING
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 41ca394..f6be195 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -194,6 +194,40 @@ void tracing_off_permanent(void)
 	set_bit(RB_BUFFERS_DISABLED_BIT, &ring_buffer_flags);
 }
 
+static enum trace_off_event tracing_off_event_ctrl = TRACE_EVENT_BUG;
+
+/**
+ * tracing_off_event - turn off tracing depending on event type
+ * @event: type of event that occurred
+ *
+ * This function checks the event type to determine whether tracing should be
+ * disabled. Useful for disabling tracing on bugs or warnings.
+ */
+void tracing_off_event(enum trace_off_event event)
+{
+	if (event <= tracing_off_event_ctrl)
+		tracing_off();
+}
+EXPORT_SYMBOL_GPL(tracing_off_event);
+
+static int __init tracing_off_event_setup(char *str)
+{
+	if (!strcmp("none", str))
+		tracing_off_event_ctrl = -1;
+	else if (!strcmp("bug", str))
+		tracing_off_event_ctrl = TRACE_EVENT_BUG;
+	else if (!strcmp("warn", str))
+		tracing_off_event_ctrl = TRACE_EVENT_WARN;
+	else {
+		printk(KERN_NOTICE "Invalid value passed for tracing_off parameter\n");
+		return 1;
+	}
+
+	return 0;
+}
+
+__setup("tracing_off=", tracing_off_event_setup);
+
 /**
  * tracing_is_on - show state of ring buffers enabled
  */
-- 
1.7.0

[PATCH 2/3] Add tracing_off_event() calls to BUG() and WARN() paths

[Chase Douglas]

This change adds tracing_off_event() calls, which stop debug tracing,
when a BUG() or WARN() function is called. The stoppage depends on
commandline paramenter tracing_off={bug,warn,none}. The default is
"bug", so only the BUG() paths will stop tracing.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 kernel/panic.c |    4 +++-
 lib/bug.c      |    2 ++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 13d966b..f0ff321 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -319,7 +319,7 @@ int oops_may_print(void)
  */
 void oops_enter(void)
 {
-	tracing_off();
+	tracing_off_event(TRACE_EVENT_BUG);
 	/* can't trust the integrity of the kernel anymore: */
 	debug_locks_off();
 	do_oops_enter_exit();
@@ -369,6 +369,8 @@ static void warn_slowpath_common(const char *file, int line, void *caller, struc
 {
 	const char *board;
 
+	tracing_off_event(TRACE_EVENT_WARN);
+
 	printk(KERN_WARNING "------------[ cut here ]------------\n");
 	printk(KERN_WARNING "WARNING: at %s:%d %pS()\n", file, line, caller);
 	board = dmi_get_system_info(DMI_PRODUCT_NAME);
diff --git a/lib/bug.c b/lib/bug.c
index 300e41a..457c1eb 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -154,6 +154,8 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 		warning = (bug->flags & BUGFLAG_WARNING) != 0;
 	}
 
+	tracing_off_event(warning ? TRACE_EVENT_WARN : TRACE_EVENT_BUG);
+
 	if (warning) {
 		/* this is a WARN_ON rather than BUG/BUG_ON */
 		if (file)
-- 
1.7.0

Re: [PATCH 2/3] Add tracing_off_event() calls to BUG() and WARN() paths

[Frederic Weisbecker]

On Wed, Apr 14, 2010 at 12:20:15PM -0400, Chase Douglas wrote:
> This change adds tracing_off_event() calls, which stop debug tracing,
> when a BUG() or WARN() function is called. The stoppage depends on
> commandline paramenter tracing_off={bug,warn,none}. The default is
> "bug", so only the BUG() paths will stop tracing.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>


ACK

But I can't find where we call oops_enter() from panic()
I see it called in x86 from oops_begin() that is called
in various places like die(), but couldn't find how it
ends up beeing called from panic().

Re: [PATCH 2/3] Add tracing_off_event() calls to BUG() and WARN() paths

[Chase Douglas]

On Thu, Apr 15, 2010 at 1:57 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Wed, Apr 14, 2010 at 12:20:15PM -0400, Chase Douglas wrote:
>> This change adds tracing_off_event() calls, which stop debug tracing,
>> when a BUG() or WARN() function is called. The stoppage depends on
>> commandline paramenter tracing_off={bug,warn,none}. The default is
>> "bug", so only the BUG() paths will stop tracing.
>>
>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>
>
> ACK
>
> But I can't find where we call oops_enter() from panic()
> I see it called in x86 from oops_begin() that is called
> in various places like die(), but couldn't find how it
> ends up beeing called from panic().

I did not test the panic() path, but when that happens the system goes
down anyways. You won't be able to get the ftrace unless you enable
the ftrace on oops option, in which case the end of the trace will
likely be close enough to what you are looking for anyways.

-- Chase

Re: [PATCH 2/3] Add tracing_off_event() calls to BUG() and WARN() paths

[Frederic Weisbecker]

On Thu, Apr 15, 2010 at 02:49:05PM -0700, Chase Douglas wrote:
> On Thu, Apr 15, 2010 at 1:57 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Wed, Apr 14, 2010 at 12:20:15PM -0400, Chase Douglas wrote:
> >> This change adds tracing_off_event() calls, which stop debug tracing,
> >> when a BUG() or WARN() function is called. The stoppage depends on
> >> commandline paramenter tracing_off={bug,warn,none}. The default is
> >> "bug", so only the BUG() paths will stop tracing.
> >>
> >> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >
> >
> > ACK
> >
> > But I can't find where we call oops_enter() from panic()
> > I see it called in x86 from oops_begin() that is called
> > in various places like die(), but couldn't find how it
> > ends up beeing called from panic().
> 
> I did not test the panic() path, but when that happens the system goes
> down anyways. You won't be able to get the ftrace unless you enable
> the ftrace on oops option, in which case the end of the trace will
> likely be close enough to what you are looking for anyways.


Yeah but there is also a sysrq that can trigger an ftrace dump.
So if you forgot ftrace dump on oops and you want to use the sysrq
then you don't want the tracing to continue between the panic and
the time for your fingers to reach the keyboard (that notwithstanding
the time to remember the right combination...which I always forget).

Re: [PATCH 2/3] Add tracing_off_event() calls to BUG() and WARN() paths

[Chase Douglas]

On Thu, Apr 15, 2010 at 4:15 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Thu, Apr 15, 2010 at 02:49:05PM -0700, Chase Douglas wrote:
>> On Thu, Apr 15, 2010 at 1:57 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> > On Wed, Apr 14, 2010 at 12:20:15PM -0400, Chase Douglas wrote:
>> >> This change adds tracing_off_event() calls, which stop debug tracing,
>> >> when a BUG() or WARN() function is called. The stoppage depends on
>> >> commandline paramenter tracing_off={bug,warn,none}. The default is
>> >> "bug", so only the BUG() paths will stop tracing.
>> >>
>> >> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>> >
>> >
>> > ACK
>> >
>> > But I can't find where we call oops_enter() from panic()
>> > I see it called in x86 from oops_begin() that is called
>> > in various places like die(), but couldn't find how it
>> > ends up beeing called from panic().
>>
>> I did not test the panic() path, but when that happens the system goes
>> down anyways. You won't be able to get the ftrace unless you enable
>> the ftrace on oops option, in which case the end of the trace will
>> likely be close enough to what you are looking for anyways.
>
>
> Yeah but there is also a sysrq that can trigger an ftrace dump.
> So if you forgot ftrace dump on oops and you want to use the sysrq
> then you don't want the tracing to continue between the panic and
> the time for your fingers to reach the keyboard (that notwithstanding
> the time to remember the right combination...which I always forget).

Ahhh, I can see where this would be useful then. I'll just add an
event to panic() like I did the other paths.

-- Chase

[PATCH 3/3] Stop tracing on a schedule bug

[Chase Douglas]

This change adds a tracing_off_event() call to stop tracing on schedule
bugs unless tracing_off=none was specified on the commandline.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 kernel/sched.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/kernel/sched.c b/kernel/sched.c
index 6af210a..439f036 100644
--- a/kernel/sched.c
+++ b/kernel/sched.c
@@ -3590,6 +3590,8 @@ static noinline void __schedule_bug(struct task_struct *prev)
 {
 	struct pt_regs *regs = get_irq_regs();
 
+	tracing_off_event(TRACE_EVENT_BUG);
+
 	printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
 		prev->comm, prev->pid, preempt_count());
 
-- 
1.7.0

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Frederic Weisbecker]

On Wed, Apr 14, 2010 at 12:20:16PM -0400, Chase Douglas wrote:
> This change adds a tracing_off_event() call to stop tracing on schedule
> bugs unless tracing_off=none was specified on the commandline.
> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> ---
>  kernel/sched.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/sched.c b/kernel/sched.c
> index 6af210a..439f036 100644
> --- a/kernel/sched.c
> +++ b/kernel/sched.c
> @@ -3590,6 +3590,8 @@ static noinline void __schedule_bug(struct task_struct *prev)
>  {
>  	struct pt_regs *regs = get_irq_regs();
>  
> +	tracing_off_event(TRACE_EVENT_BUG);
> +
>  	printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
>  		prev->comm, prev->pid, preempt_count());



I would rather call that a TRACE_EVENT_WARN as this is what happens: we
warn but we continue.

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Chase Douglas]

On Thu, Apr 15, 2010 at 2:03 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> On Wed, Apr 14, 2010 at 12:20:16PM -0400, Chase Douglas wrote:
>> This change adds a tracing_off_event() call to stop tracing on schedule
>> bugs unless tracing_off=none was specified on the commandline.
>>
>> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>> ---
>>  kernel/sched.c |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/kernel/sched.c b/kernel/sched.c
>> index 6af210a..439f036 100644
>> --- a/kernel/sched.c
>> +++ b/kernel/sched.c
>> @@ -3590,6 +3590,8 @@ static noinline void __schedule_bug(struct task_struct *prev)
>>  {
>>       struct pt_regs *regs = get_irq_regs();
>>
>> +     tracing_off_event(TRACE_EVENT_BUG);
>> +
>>       printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
>>               prev->comm, prev->pid, preempt_count());
>
>
>
> I would rather call that a TRACE_EVENT_WARN as this is what happens: we
> warn but we continue.

I tend to think of the TRACE_EVENT_* as an indication of severity and
whether we want to stop the trace by default. From a distro
standpoint, the likelihood that we want to continue tracing after a
__schedule_bug is pretty low. It's easiest if we don't have to tell
our users to add a kernel command line, especially since grub in
Ubuntu 10.04 LTS is difficult to interact with for end users.

Now I'm not much of an upstream developer (at least not yet), so if it
makes much more sense to continue by default from a development
standpoint, then perhaps it's better to make it continue by default. I
just think that the frequency of hitting this should be low enough
that it won't trip people up. And when it does, maybe that gives more
incentive to fix the schedule bug :).

-- Chase

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1976 bytes --]

On Thu, 15 Apr 2010, Chase Douglas wrote:
> On Thu, Apr 15, 2010 at 2:03 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> > On Wed, Apr 14, 2010 at 12:20:16PM -0400, Chase Douglas wrote:
> >> This change adds a tracing_off_event() call to stop tracing on schedule
> >> bugs unless tracing_off=none was specified on the commandline.
> >>
> >> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >> ---
> >>  kernel/sched.c |    2 ++
> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/kernel/sched.c b/kernel/sched.c
> >> index 6af210a..439f036 100644
> >> --- a/kernel/sched.c
> >> +++ b/kernel/sched.c
> >> @@ -3590,6 +3590,8 @@ static noinline void __schedule_bug(struct task_struct *prev)
> >>  {
> >>       struct pt_regs *regs = get_irq_regs();
> >>
> >> +     tracing_off_event(TRACE_EVENT_BUG);
> >> +
> >>       printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
> >>               prev->comm, prev->pid, preempt_count());
> >
> >
> >
> > I would rather call that a TRACE_EVENT_WARN as this is what happens: we
> > warn but we continue.
> 
> I tend to think of the TRACE_EVENT_* as an indication of severity and
> whether we want to stop the trace by default. From a distro
> standpoint, the likelihood that we want to continue tracing after a
> __schedule_bug is pretty low. It's easiest if we don't have to tell

Well, scheduling while atomic is a BUG, but one of the category which
allows the kernel to continue. So in fact it's treated like a WARN_ON.
So the tracing_off_event() qualifier should be *_WARN.

That's independent of the question whether you want to stop tracing in
that very case. Though I agree that the tracer should stop here.

> our users to add a kernel command line, especially since grub in
> Ubuntu 10.04 LTS is difficult to interact with for end users.

That's a serious PITA caused by the "let's mimic the other OS" crowd
and no excuse for creating a mess in the kernel.
 
Thanks,

	tglx

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 01:01:50AM +0200, Thomas Gleixner wrote:
> > I tend to think of the TRACE_EVENT_* as an indication of severity and
> > whether we want to stop the trace by default. From a distro
> > standpoint, the likelihood that we want to continue tracing after a
> > __schedule_bug is pretty low. It's easiest if we don't have to tell
> 
> Well, scheduling while atomic is a BUG, but one of the category which
> allows the kernel to continue. So in fact it's treated like a WARN_ON.
> So the tracing_off_event() qualifier should be *_WARN.
> 
> That's independent of the question whether you want to stop tracing in
> that very case


Exactly.

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Chase Douglas]

On Thu, Apr 15, 2010 at 4:01 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 15 Apr 2010, Chase Douglas wrote:
>> On Thu, Apr 15, 2010 at 2:03 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> > On Wed, Apr 14, 2010 at 12:20:16PM -0400, Chase Douglas wrote:
>> >> This change adds a tracing_off_event() call to stop tracing on schedule
>> >> bugs unless tracing_off=none was specified on the commandline.
>> >>
>> >> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>> >> ---
>> >>  kernel/sched.c |    2 ++
>> >>  1 files changed, 2 insertions(+), 0 deletions(-)
>> >>
>> >> diff --git a/kernel/sched.c b/kernel/sched.c
>> >> index 6af210a..439f036 100644
>> >> --- a/kernel/sched.c
>> >> +++ b/kernel/sched.c
>> >> @@ -3590,6 +3590,8 @@ static noinline void __schedule_bug(struct task_struct *prev)
>> >>  {
>> >>       struct pt_regs *regs = get_irq_regs();
>> >>
>> >> +     tracing_off_event(TRACE_EVENT_BUG);
>> >> +
>> >>       printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
>> >>               prev->comm, prev->pid, preempt_count());
>> >
>> >
>> >
>> > I would rather call that a TRACE_EVENT_WARN as this is what happens: we
>> > warn but we continue.
>>
>> I tend to think of the TRACE_EVENT_* as an indication of severity and
>> whether we want to stop the trace by default. From a distro
>> standpoint, the likelihood that we want to continue tracing after a
>> __schedule_bug is pretty low. It's easiest if we don't have to tell
>
> Well, scheduling while atomic is a BUG, but one of the category which
> allows the kernel to continue. So in fact it's treated like a WARN_ON.
> So the tracing_off_event() qualifier should be *_WARN.
>
> That's independent of the question whether you want to stop tracing in
> that very case. Though I agree that the tracer should stop here.

We seem to be agreeing on the functionality. The disagreement seems to
be in the macro name/functionality mapping. However, the name of the
function itself is *_bug. I don't see how things are clearer or more
useful by inserting a *_WARN level macro in a *_bug named function.

Essentially, it makes more sense to me for the macro to represent the
severity of the case, and not be coupled somehow to what the kernel
decides to do outside of the tracing.

Is it the case that you really feel it should be *_WARN, or that the
macro name/functionality should be different?

-- Chase

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3488 bytes --]

On Thu, 15 Apr 2010, Chase Douglas wrote:
> On Thu, Apr 15, 2010 at 4:01 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> > On Thu, 15 Apr 2010, Chase Douglas wrote:
> >> On Thu, Apr 15, 2010 at 2:03 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >> > On Wed, Apr 14, 2010 at 12:20:16PM -0400, Chase Douglas wrote:
> >> >> This change adds a tracing_off_event() call to stop tracing on schedule
> >> >> bugs unless tracing_off=none was specified on the commandline.
> >> >>
> >> >> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
> >> >> ---
> >> >>  kernel/sched.c |    2 ++
> >> >>  1 files changed, 2 insertions(+), 0 deletions(-)
> >> >>
> >> >> diff --git a/kernel/sched.c b/kernel/sched.c
> >> >> index 6af210a..439f036 100644
> >> >> --- a/kernel/sched.c
> >> >> +++ b/kernel/sched.c
> >> >> @@ -3590,6 +3590,8 @@ static noinline void __schedule_bug(struct task_struct *prev)
> >> >>  {
> >> >>       struct pt_regs *regs = get_irq_regs();
> >> >>
> >> >> +     tracing_off_event(TRACE_EVENT_BUG);
> >> >> +
> >> >>       printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
> >> >>               prev->comm, prev->pid, preempt_count());
> >> >
> >> >
> >> >
> >> > I would rather call that a TRACE_EVENT_WARN as this is what happens: we
> >> > warn but we continue.
> >>
> >> I tend to think of the TRACE_EVENT_* as an indication of severity and
> >> whether we want to stop the trace by default. From a distro
> >> standpoint, the likelihood that we want to continue tracing after a
> >> __schedule_bug is pretty low. It's easiest if we don't have to tell
> >
> > Well, scheduling while atomic is a BUG, but one of the category which
> > allows the kernel to continue. So in fact it's treated like a WARN_ON.
> > So the tracing_off_event() qualifier should be *_WARN.
> >
> > That's independent of the question whether you want to stop tracing in
> > that very case. Though I agree that the tracer should stop here.
> 
> We seem to be agreeing on the functionality. The disagreement seems to
> be in the macro name/functionality mapping. However, the name of the
> function itself is *_bug. I don't see how things are clearer or more
> useful by inserting a *_WARN level macro in a *_bug named function.

Care to read what I wrote ? Again:

> > Well, scheduling while atomic is a BUG, but one of the category which
> > allows the kernel to continue. So in fact it's treated like a WARN_ON.
> > So the tracing_off_event() qualifier should be *_WARN.

It does not matter at all whether the function name has "bug" in it or
not. What matters is the semantics of the function. It does _NOT_
raise a BUG. It merily warns and tries to continue. So it follows the
WARN() semantics.

If you feel strong about that send a patch to
s/schedule_bug/schedule_warn/ and I'll ack it.

> Essentially, it makes more sense to me for the macro to represent the
> severity of the case, and not be coupled somehow to what the kernel
> decides to do outside of the tracing.

Essentially you are wrong. The semantic of schedule_bug() is clearly
WARN() and not BUG(). So the tracing off qualifier needs to be
WARN. And it does not matter what you consider as the severity. The
severity is given by the semantics of schedule_bug().

If your extra stupid grub hiding logic prevents an user to change the
trace off level then you need to fix that instead of anything else.

BTW, if interacting with grub is that hard: how does an user start the
tracer at all ?

Thanks,

	tglx

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Thomas Gleixner]

On Fri, 16 Apr 2010, Thomas Gleixner wrote:
> BTW, if interacting with grub is that hard: how does an user start the
> tracer at all ?

Just looked through the other patches and noticed that the patch which
provides the tracing_off(level) stuff is incomplete as it provides
only a command line option to change that tracing off level.

The command line option is merily for tracing which happens to be
started on the command line i.e. _BEFORE_ we have usable user space.

So your grub argument is just crap. If the user cannot change this
setting w/o fiddling with the obscured grub then he can not start the
tracer on the command line either.

But somehow he can start the tracer later when user space is up and
running, but there is no way to change that setting anymore. Therefor
you go through the kernel and impose settings at will.

1) Your patch simply lacks an interface to change that setting via
   debugfs/tracing/wtf

   WTF should I reboot my machine to change that setting from the
   default BUG to WARN or NONE ? There is no reason at all.

2) tracing off can be done via filters on functions and/or events
   already - so I doubt that the tracing_off_event(level) is necessary
   at all.

   schedule_bug() definitely deserves a separate trace_schedule_bug()
   event which can be used to stop the tracer by already existing
   functionality.

Thanks,

	tglx

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 02:21:47AM +0200, Thomas Gleixner wrote:
> On Fri, 16 Apr 2010, Thomas Gleixner wrote:
> > BTW, if interacting with grub is that hard: how does an user start the
> > tracer at all ?
> 
> Just looked through the other patches and noticed that the patch which
> provides the tracing_off(level) stuff is incomplete as it provides
> only a command line option to change that tracing off level.
> 
> The command line option is merily for tracing which happens to be
> started on the command line i.e. _BEFORE_ we have usable user space.
> 
> So your grub argument is just crap. If the user cannot change this
> setting w/o fiddling with the obscured grub then he can not start the
> tracer on the command line either.
> 
> But somehow he can start the tracer later when user space is up and
> running, but there is no way to change that setting anymore. Therefor
> you go through the kernel and impose settings at will.
> 
> 1) Your patch simply lacks an interface to change that setting via
>    debugfs/tracing/wtf
> 
>    WTF should I reboot my machine to change that setting from the
>    default BUG to WARN or NONE ? There is no reason at all.
> 
> 2) tracing off can be done via filters on functions and/or events
>    already - so I doubt that the tracing_off_event(level) is necessary
>    at all.



Yeah it works for the function tracer, but not for events (or other tracers),
or I missed this feature somehow.

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Steven Rostedt]

On Fri, 2010-04-16 at 03:49 +0200, Frederic Weisbecker wrote:

> > 2) tracing off can be done via filters on functions and/or events
> >    already - so I doubt that the tracing_off_event(level) is necessary
> >    at all.
> 
> 
> 
> Yeah it works for the function tracer, but not for events (or other tracers),
> or I missed this feature somehow.

It is currently only for the function tracer. It's been on my todo list
for events for quite a while. Perhaps I'll push that up to the top of
the list next week.

-- Steve

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Chase Douglas]

On Thu, Apr 15, 2010 at 5:21 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Fri, 16 Apr 2010, Thomas Gleixner wrote:
>> BTW, if interacting with grub is that hard: how does an user start the
>> tracer at all ?
>
> Just looked through the other patches and noticed that the patch which
> provides the tracing_off(level) stuff is incomplete as it provides
> only a command line option to change that tracing off level.
>
> The command line option is merily for tracing which happens to be
> started on the command line i.e. _BEFORE_ we have usable user space.
>
> So your grub argument is just crap. If the user cannot change this
> setting w/o fiddling with the obscured grub then he can not start the
> tracer on the command line either.
>
> But somehow he can start the tracer later when user space is up and
> running, but there is no way to change that setting anymore. Therefor
> you go through the kernel and impose settings at will.
>
> 1) Your patch simply lacks an interface to change that setting via
>   debugfs/tracing/wtf
>
>   WTF should I reboot my machine to change that setting from the
>   default BUG to WARN or NONE ? There is no reason at all.

You're right, this should be something that may be changed at run
time. I spoke with Steven at LCS and he agrees that there should be an
option in (debugfs)/tracing/options/ for this. I will fix up the first
patch to include this.

> 2) tracing off can be done via filters on functions and/or events
>   already - so I doubt that the tracing_off_event(level) is necessary
>   at all.
>
>   schedule_bug() definitely deserves a separate trace_schedule_bug()
>   event which can be used to stop the tracer by already existing
>   functionality.

Steven said he would be fine with a separate TRACE_EVENT_<blah> macro
for the schedule bug if needed, but I'm not sure we need to go that
far. If it's configurable through debugfs at run time then it serves
my purpose. Unless you feel we should have finer grained control
specifically for scheduling while atomic bugs, I'll just leave it as
TRACE_EVENT_WARN.

-- Chase

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Steven Rostedt]

On Thu, 2010-04-15 at 21:01 -0700, Chase Douglas wrote:

> > 2) tracing off can be done via filters on functions and/or events
> >   already - so I doubt that the tracing_off_event(level) is necessary
> >   at all.
> >
> >   schedule_bug() definitely deserves a separate trace_schedule_bug()
> >   event which can be used to stop the tracer by already existing
> >   functionality.
> 
> Steven said he would be fine with a separate TRACE_EVENT_<blah> macro
> for the schedule bug if needed, but I'm not sure we need to go that
> far. If it's configurable through debugfs at run time then it serves
> my purpose. Unless you feel we should have finer grained control
> specifically for scheduling while atomic bugs, I'll just leave it as
> TRACE_EVENT_WARN.

I actually like Thomas's idea better. I need to add the "stop trace on
event" functionality, and we can insert trace events for bugs, and not
have this whole "stop tracing here" functions. Instead we could just add
tracepoints and have a way to pick and choose where to stop tracing.

add a:

include/trace/events/errors.h

#define TRACE_SYSTEM errors

TRACE_EVENT(sched_bug, ....)

etc,


When I get back home, I'll add this functionality to stop tracing on
events. Perhaps I'll even add "TRACE_SUB_SYSTEM" so in the events
directory, we can have sub layers:

events/errors/BUG/...
events/errors/WARNING/...

etc

-- Steve

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Chase Douglas]

On Fri, Apr 16, 2010 at 9:46 AM, Steven Rostedt <rostedt@goodmis.org> wrote:
> On Thu, 2010-04-15 at 21:01 -0700, Chase Douglas wrote:
>> > 2) tracing off can be done via filters on functions and/or events
>> >   already - so I doubt that the tracing_off_event(level) is necessary
>> >   at all.
>> >
>> >   schedule_bug() definitely deserves a separate trace_schedule_bug()
>> >   event which can be used to stop the tracer by already existing
>> >   functionality.
>>
>> Steven said he would be fine with a separate TRACE_EVENT_<blah> macro
>> for the schedule bug if needed, but I'm not sure we need to go that
>> far. If it's configurable through debugfs at run time then it serves
>> my purpose. Unless you feel we should have finer grained control
>> specifically for scheduling while atomic bugs, I'll just leave it as
>> TRACE_EVENT_WARN.
>
> I actually like Thomas's idea better. I need to add the "stop trace on
> event" functionality, and we can insert trace events for bugs, and not
> have this whole "stop tracing here" functions. Instead we could just add
> tracepoints and have a way to pick and choose where to stop tracing.
>
> add a:
>
> include/trace/events/errors.h
>
> #define TRACE_SYSTEM errors
>
> TRACE_EVENT(sched_bug, ....)
>
> etc,
>
>
> When I get back home, I'll add this functionality to stop tracing on
> events. Perhaps I'll even add "TRACE_SUB_SYSTEM" so in the events
> directory, we can have sub layers:
>
> events/errors/BUG/...
> events/errors/WARNING/...
>
> etc

Ok, I see where this is going now. I agree this sounds like a much
better approach. I'll wait for the implementation of the stop tracing
on event functionality so I know how it will work, and then I can
submit patches for the BUG, WARN, panic, and schedule_bug paths.

-- Chase

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Frederic Weisbecker]

On Fri, Apr 16, 2010 at 12:46:53PM -0400, Steven Rostedt wrote:
> On Thu, 2010-04-15 at 21:01 -0700, Chase Douglas wrote:
> 
> > > 2) tracing off can be done via filters on functions and/or events
> > >   already - so I doubt that the tracing_off_event(level) is necessary
> > >   at all.
> > >
> > >   schedule_bug() definitely deserves a separate trace_schedule_bug()
> > >   event which can be used to stop the tracer by already existing
> > >   functionality.
> > 
> > Steven said he would be fine with a separate TRACE_EVENT_<blah> macro
> > for the schedule bug if needed, but I'm not sure we need to go that
> > far. If it's configurable through debugfs at run time then it serves
> > my purpose. Unless you feel we should have finer grained control
> > specifically for scheduling while atomic bugs, I'll just leave it as
> > TRACE_EVENT_WARN.
> 
> I actually like Thomas's idea better. I need to add the "stop trace on
> event" functionality, and we can insert trace events for bugs, and not
> have this whole "stop tracing here" functions. Instead we could just add
> tracepoints and have a way to pick and choose where to stop tracing.
> 
> add a:
> 
> include/trace/events/errors.h
> 
> #define TRACE_SYSTEM errors
> 
> TRACE_EVENT(sched_bug, ....)
> 
> etc,
> 


Looks good.


 
> When I get back home, I'll add this functionality to stop tracing on
> events. Perhaps I'll even add "TRACE_SUB_SYSTEM" so in the events
> directory, we can have sub layers:
> 
> events/errors/BUG/...
> events/errors/WARNING/...


I agree this could be nice. I'm thinking about the printk all around
the kernel that can be represented as trace events with sub-sub-systems
that could be file paths of the kernel.

I just hope we can find a way to do this that won't break tools.

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Thomas Gleixner]

Steven,

On Fri, 16 Apr 2010, Steven Rostedt wrote:

> On Thu, 2010-04-15 at 21:01 -0700, Chase Douglas wrote:
> 
> > > 2) tracing off can be done via filters on functions and/or events
> > >   already - so I doubt that the tracing_off_event(level) is necessary
> > >   at all.
> > >
> > >   schedule_bug() definitely deserves a separate trace_schedule_bug()
> > >   event which can be used to stop the tracer by already existing
> > >   functionality.
> > 
> > Steven said he would be fine with a separate TRACE_EVENT_<blah> macro
> > for the schedule bug if needed, but I'm not sure we need to go that
> > far. If it's configurable through debugfs at run time then it serves
> > my purpose. Unless you feel we should have finer grained control
> > specifically for scheduling while atomic bugs, I'll just leave it as
> > TRACE_EVENT_WARN.
> 
> I actually like Thomas's idea better. I need to add the "stop trace on
> event" functionality, and we can insert trace events for bugs, and not

I just assumed that it would work for events already. The stop trace on
function filter works perfect and is a very conveniant tool.

> have this whole "stop tracing here" functions. Instead we could just add
> tracepoints and have a way to pick and choose where to stop tracing.
> 
> add a:
> 
> include/trace/events/errors.h
> 
> #define TRACE_SYSTEM errors
> 
> TRACE_EVENT(sched_bug, ....)
> 
> etc,
> 
> 
> When I get back home, I'll add this functionality to stop tracing on
> events. Perhaps I'll even add "TRACE_SUB_SYSTEM" so in the events
> directory, we can have sub layers:
> 
> events/errors/BUG/...
> events/errors/WARNING/...

I like that idea. That's solving the problem in a very elegant way.

Thanks,

	tglx

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Chase Douglas]

> 2) tracing off can be done via filters on functions and/or events
>   already - so I doubt that the tracing_off_event(level) is necessary
>   at all.

I just found what you were meaning here, since there's no
documentation yet about the traceon/traceoff filter command. This
would do exactly what we want for helping us fix bugs.

I'll write some info about the command in ftrace.txt and send a patch
for review so no one else is left out of the party :).

-- Chase

Re: [PATCH 3/3] Stop tracing on a schedule bug

[Chase Douglas]

On Thu, Apr 15, 2010 at 4:50 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> On Thu, 15 Apr 2010, Chase Douglas wrote:
>> On Thu, Apr 15, 2010 at 4:01 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
>> > On Thu, 15 Apr 2010, Chase Douglas wrote:
>> >> On Thu, Apr 15, 2010 at 2:03 PM, Frederic Weisbecker <fweisbec@gmail.com> wrote:
>> >> > On Wed, Apr 14, 2010 at 12:20:16PM -0400, Chase Douglas wrote:
>> >> >> This change adds a tracing_off_event() call to stop tracing on schedule
>> >> >> bugs unless tracing_off=none was specified on the commandline.
>> >> >>
>> >> >> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
>> >> >> ---
>> >> >>  kernel/sched.c |    2 ++
>> >> >>  1 files changed, 2 insertions(+), 0 deletions(-)
>> >> >>
>> >> >> diff --git a/kernel/sched.c b/kernel/sched.c
>> >> >> index 6af210a..439f036 100644
>> >> >> --- a/kernel/sched.c
>> >> >> +++ b/kernel/sched.c
>> >> >> @@ -3590,6 +3590,8 @@ static noinline void __schedule_bug(struct task_struct *prev)
>> >> >>  {
>> >> >>       struct pt_regs *regs = get_irq_regs();
>> >> >>
>> >> >> +     tracing_off_event(TRACE_EVENT_BUG);
>> >> >> +
>> >> >>       printk(KERN_ERR "BUG: scheduling while atomic: %s/%d/0x%08x\n",
>> >> >>               prev->comm, prev->pid, preempt_count());
>> >> >
>> >> >
>> >> >
>> >> > I would rather call that a TRACE_EVENT_WARN as this is what happens: we
>> >> > warn but we continue.
>> >>
>> >> I tend to think of the TRACE_EVENT_* as an indication of severity and
>> >> whether we want to stop the trace by default. From a distro
>> >> standpoint, the likelihood that we want to continue tracing after a
>> >> __schedule_bug is pretty low. It's easiest if we don't have to tell
>> >
>> > Well, scheduling while atomic is a BUG, but one of the category which
>> > allows the kernel to continue. So in fact it's treated like a WARN_ON.
>> > So the tracing_off_event() qualifier should be *_WARN.
>> >
>> > That's independent of the question whether you want to stop tracing in
>> > that very case. Though I agree that the tracer should stop here.
>>
>> We seem to be agreeing on the functionality. The disagreement seems to
>> be in the macro name/functionality mapping. However, the name of the
>> function itself is *_bug. I don't see how things are clearer or more
>> useful by inserting a *_WARN level macro in a *_bug named function.
>
> Care to read what I wrote ? Again:

I misunderstood what you wrote. I thought you were agreeing that the
tracing should stop by default in __schedule_bug. I now realize you
meant that you agree it should be possible to stop it, but it should
not be the default behavior. Sorry for the confusion.

I will change the event to be a *_WARN type.

-- Chase

Re: [PATCH 1/3 v4] Add tracing_off_event() to stop tracing when a bug or warning occur

[Frederic Weisbecker]

On Wed, Apr 14, 2010 at 12:20:14PM -0400, Chase Douglas wrote:
> The tracing_off_event() function calls tracing_off() to stop tracing
> when an event occurs. By default, only BUG-type events stop tracing,
> while WARNING type events do not. This is controlled through the
> tracing_off={none,warn,bug} commandline parameter.
> 
> Call this function from bug and warning event handlers to enable a user
> to debug their kernel by starting a trace, hitting an event, and then
> retrieving trace info knowing that the trace was stopped right after the
> event was hit.


That's a good idea!


> 
> Signed-off-by: Chase Douglas <chase.douglas@canonical.com>


Acked-by: Frederic Weisbecker <fweisbec@gmail.com>

[PATCH 1/3] Add tracing_off_event() to stop tracing when a bug or warning occur

[Chase Douglas]

The tracing_off_event() function calls tracing_off() to stop tracing
when an event occurs. By default, only BUG-type events stop tracing,
while WARNING type events do not. This is controlled through the
tracing_off={none,warn,bug} commandline parameter.

Call this function from bug and warning event handlers to enable a user
to debug their kernel by starting a trace, hitting an event, and then
retrieving trace info knowing that the trace was stopped right after the
event was hit.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 include/linux/kernel.h     |    5 +++++
 kernel/trace/ring_buffer.c |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index 7f07074..107091f 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -478,16 +478,21 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
  *
  * Most likely, you want to use tracing_on/tracing_off.
  */
+#define TRACE_EVENT_BUG 0
+#define TRACE_EVENT_WARN 1
+
 #ifdef CONFIG_RING_BUFFER
 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);
+void tracing_off_event(long event);
 int tracing_is_on(void);
 #else
 static inline void tracing_on(void) { }
 static inline void tracing_off(void) { }
 static inline void tracing_off_permanent(void) { }
+static inline void tracing_off_event(long event) { }
 static inline int tracing_is_on(void) { return 0; }
 #endif
 #ifdef CONFIG_TRACING
diff --git a/kernel/trace/ring_buffer.c b/kernel/trace/ring_buffer.c
index 0287f9f..b148862 100644
--- a/kernel/trace/ring_buffer.c
+++ b/kernel/trace/ring_buffer.c
@@ -193,6 +193,40 @@ void tracing_off_permanent(void)
 	set_bit(RB_BUFFERS_DISABLED_BIT, &ring_buffer_flags);
 }
 
+static long tracing_event_mask = TRACE_EVENT_BUG;
+
+/**
+ * tracing_off_event - turn off tracing depending on event type
+ *
+ * This function checks the event type to determine whether tracing should be
+ * disabled. Useful for disabling tracing on bugs or warnings.
+ */
+void tracing_off_event(long event)
+{
+	if (event <= tracing_event_mask)
+		tracing_off();
+}
+EXPORT_SYMBOL_GPL(tracing_off_event);
+
+static int __init tracing_off_event_setup(char *str)
+{
+	if (!strcmp("none", str))
+		tracing_event_mask = -1;
+	else if (!strcmp("bug", str))
+		tracing_event_mask = TRACE_EVENT_BUG;
+	else if (!strcmp("warn", str))
+		tracing_event_mask = TRACE_EVENT_WARN;
+	else
+	{
+		printk(KERN_NOTICE "Invalid value passed for tracing_off parameter\n");
+		return 1;
+	}
+
+	return 0;
+}
+
+__setup("tracing_off=", tracing_off_event_setup);
+
 /**
  * tracing_is_on - show state of ring buffers enabled
  */
-- 
1.6.3.3

[PATCH 2/3] Add tracing_off_event() calls to BUG() and WARN() paths

[Chase Douglas]

This change adds tracing_off_event() calls, which stop debug tracing,
when a BUG() or WARN() function is called. The stoppage depends on
commandline paramenter tracing_off={bug,warn,none}. The default is
"bug", so only the BUG() paths will stop tracing.

Signed-off-by: Chase Douglas <chase.douglas@canonical.com>
---
 kernel/panic.c |    4 +++-
 lib/bug.c      |    2 ++
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/panic.c b/kernel/panic.c
index 13d966b..f0ff321 100644
--- a/kernel/panic.c
+++ b/kernel/panic.c
@@ -319,7 +319,7 @@ int oops_may_print(void)
  */
 void oops_enter(void)
 {
-	tracing_off();
+	tracing_off_event(TRACE_EVENT_BUG);
 	/* can't trust the integrity of the kernel anymore: */
 	debug_locks_off();
 	do_oops_enter_exit();
@@ -369,6 +369,8 @@ static void warn_slowpath_common(const char *file, int line, void *caller, struc
 {
 	const char *board;
 
+	tracing_off_event(TRACE_EVENT_WARN);
+
 	printk(KERN_WARNING "------------[ cut here ]------------\n");
 	printk(KERN_WARNING "WARNING: at %s:%d %pS()\n", file, line, caller);
 	board = dmi_get_system_info(DMI_PRODUCT_NAME);
diff --git a/lib/bug.c b/lib/bug.c
index 300e41a..457c1eb 100644
--- a/lib/bug.c
+++ b/lib/bug.c
@@ -154,6 +154,8 @@ enum bug_trap_type report_bug(unsigned long bugaddr, struct pt_regs *regs)
 		warning = (bug->flags & BUGFLAG_WARNING) != 0;
 	}
 
+	tracing_off_event(warning ? TRACE_EVENT_WARN : TRACE_EVENT_BUG);
+
 	if (warning) {
 		/* this is a WARN_ON rather than BUG/BUG_ON */
 		if (file)
-- 
1.6.3.3