[PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[Frederic Weisbecker]

Impact: Make the branch tracer use raw irq save/restore

When the branch tracer inserts an event through probe_likely_condition(),
it calls local_irq_save() and then results in a trace recursion.

local_irq_save() -> trace_hardirqs_off() -> trace_hardirqs_off_caller()
	-> unlikely()

The trace_branch.c file is protected by DISABLE_BRANCH_PROFILING but that
doesn't prevent from external call to functions that use unlikely().

My box crashed each time I tried to set this tracer (sudden and hard reboot).

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
---
 kernel/trace/trace_branch.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_branch.c b/kernel/trace/trace_branch.c
index 44bd395..23f9b02 100644
--- a/kernel/trace/trace_branch.c
+++ b/kernel/trace/trace_branch.c
@@ -41,7 +41,7 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect)
 	if (unlikely(!tr))
 		return;
 
-	local_irq_save(flags);
+	raw_local_irq_save(flags);
 	cpu = raw_smp_processor_id();
 	if (atomic_inc_return(&tr->data[cpu]->disabled) != 1)
 		goto out;
@@ -73,7 +73,7 @@ probe_likely_condition(struct ftrace_branch_data *f, int val, int expect)
 
  out:
 	atomic_dec(&tr->data[cpu]->disabled);
-	local_irq_restore(flags);
+	raw_local_irq_restore(flags);
 }
 
 static inline
-- 
1.5.2.5

Re: [PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[Ingo Molnar]

* Frederic Weisbecker <fweisbec@gmail.com> wrote:

> Impact: Make the branch tracer use raw irq save/restore
> 
> When the branch tracer inserts an event through 
> probe_likely_condition(), it calls local_irq_save() and then results 
> in a trace recursion.
> 
> local_irq_save() -> trace_hardirqs_off() -> trace_hardirqs_off_caller()
> 	-> unlikely()
> 
> The trace_branch.c file is protected by DISABLE_BRANCH_PROFILING but 
> that doesn't prevent from external call to functions that use 
> unlikely().
> 
> My box crashed each time I tried to set this tracer (sudden and hard 
> reboot).

So this patch fixes that problem, or are there other problems left?

	Ingo

Re: [PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[Frédéric Weisbecker]

2008/11/16 Ingo Molnar <mingo@elte.hu>:
>
> * Frederic Weisbecker <fweisbec@gmail.com> wrote:
>
>> Impact: Make the branch tracer use raw irq save/restore
>>
>> When the branch tracer inserts an event through
>> probe_likely_condition(), it calls local_irq_save() and then results
>> in a trace recursion.
>>
>> local_irq_save() -> trace_hardirqs_off() -> trace_hardirqs_off_caller()
>>       -> unlikely()
>>
>> The trace_branch.c file is protected by DISABLE_BRANCH_PROFILING but
>> that doesn't prevent from external call to functions that use
>> unlikely().
>>
>> My box crashed each time I tried to set this tracer (sudden and hard
>> reboot).
>
> So this patch fixes that problem, or are there other problems left?
>
>        Ingo
>


I forgot to answer to all. Yes that fixes it.

Re: [PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[Ingo Molnar]

* Frédéric Weisbecker <fweisbec@gmail.com> wrote:

> 2008/11/16 Ingo Molnar <mingo@elte.hu>:
> >
> > * Frederic Weisbecker <fweisbec@gmail.com> wrote:
> >
> >> Impact: Make the branch tracer use raw irq save/restore
> >>
> >> When the branch tracer inserts an event through
> >> probe_likely_condition(), it calls local_irq_save() and then results
> >> in a trace recursion.
> >>
> >> local_irq_save() -> trace_hardirqs_off() -> trace_hardirqs_off_caller()
> >>       -> unlikely()
> >>
> >> The trace_branch.c file is protected by DISABLE_BRANCH_PROFILING but
> >> that doesn't prevent from external call to functions that use
> >> unlikely().
> >>
> >> My box crashed each time I tried to set this tracer (sudden and hard
> >> reboot).
> >
> > So this patch fixes that problem, or are there other problems left?
> >
> >        Ingo
> >
> 
> 
> I forgot to answer to all. Yes that fixes it.

cool - i was seeing the crashes too in automated testing, just havent 
had time to debug it yet.

One small detail:

Regarding the "Impact" line, it's cool that you started adding them. 
I'd like to ask you to tweak them a little bit in the future: please 
try to describe the "practical impact" via them - not just a 
repetition of the source code change that the patch does.

For example, for this patch, instead of this impact-line:

  Impact: Make the branch tracer use raw irq save/restore

a better one is:

  Impact: fix crash when enabling the branch-tracer

(i changed it in this patch, no need to resubmit. And it's not well 
documented anyway.)

Thanks,

	Ingo

Re: [PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[Frédéric Weisbecker]

2008/11/16 Ingo Molnar <mingo@elte.hu>:
> Regarding the "Impact" line, it's cool that you started adding them.
> I'd like to ask you to tweak them a little bit in the future: please
> try to describe the "practical impact" via them - not just a
> repetition of the source code change that the patch does.
>
> For example, for this patch, instead of this impact-line:
>
>  Impact: Make the branch tracer use raw irq save/restore
>
> a better one is:
>
>  Impact: fix crash when enabling the branch-tracer
>
> (i changed it in this patch, no need to resubmit. And it's not well
> documented anyway.)
>
> Thanks,


Ok. Yeah I had some trouble with this "impact" tag, finding it hard to
not overlap its message with this of the patch's subject but I better
understand now...
Perhaps this tag should be explained in
Documentation/development-process/5.Posting and SubmittingPatches...
(cc-ed Jonathan for the development-process documents)....

Re: [PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[Jonathan Corbet]

On Sun, 16 Nov 2008 15:01:24 +0100
"Frédéric Weisbecker" <fweisbec@gmail.com> wrote:

> Ok. Yeah I had some trouble with this "impact" tag, finding it hard to
> not overlap its message with this of the patch's subject but I better
> understand now...
> Perhaps this tag should be explained in
> Documentation/development-process/5.Posting and SubmittingPatches...
> (cc-ed Jonathan for the development-process documents)....

I'll happily do so.  I will confess, though, that I don't really
understand how that tag differs from the information which simply
belongs in the patch title and changelog.  

jon

Re: [PATCH 3/4] tracing/branch-tracer: Fix a trace recursion on branch tracer

[Ingo Molnar]

* Jonathan Corbet <corbet@lwn.net> wrote:

> On Sun, 16 Nov 2008 15:01:24 +0100
> "Frédéric Weisbecker" <fweisbec@gmail.com> wrote:
> 
> > Ok. Yeah I had some trouble with this "impact" tag, finding it hard to
> > not overlap its message with this of the patch's subject but I better
> > understand now...
> > Perhaps this tag should be explained in
> > Documentation/development-process/5.Posting and SubmittingPatches...
> > (cc-ed Jonathan for the development-process documents)....

this is really only an experimental tag at this point, and not 
mandatory in any way.

> I'll happily do so.  I will confess, though, that I don't really 
> understand how that tag differs from the information which simply 
> belongs in the patch title and changelog.

The impact line (invented by hpa half a year ago and used by the x86 
maintainers for the past couple of months) tries to give a one-line 
"impact of change" risk summary.

Here's a small description about it:

   http://lkml.org/lkml/2008/10/28/67

the impact line is a secondary subject line, it quantifies practical 
impact of changes/bugfixes.

The subject line is often controlled by other principles (subsystem 
tags, scope and subject of change, etc.), and the actual changelog is 
often very verbose and talks about the change, not just the problem it 
solves.

The impact-line also documents the _intended_ impact of a change - 
making it easier to see it later whether a change was contemplated to 
have side-effects originally. If a change says "Impact: cleanup" and 
it turns out to break some systems later on, the discrepancy is very 
clear and well documented. We had cases already where it helped.

It also helps when maintaining changes: it makes it clear whether a 
patch should be backported to -stable, etc. Filtering through 
thousands of changes in search for fixes that matter can be quite hard 
- the impact line helps speed up that workflow too.

The definition and usage if it is still a bit in flux, so i'm not sure 
we want to codify it in Documentation/SubmittingPatches. But if you 
could come up with a description it would at minimum be a nice URL to 
point contributors to :-)

	Ingo