tracing/branch-tracer: Fix a trace recursion on branch tracer

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

---
 kernel/trace/trace_branch.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

index 44bd395..23f9b02 100644
@@ -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

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

Ingo

2008/11/16 Ingo Molnar <[email protected]>:

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

2008/11/16 Ingo Molnar <[email protected]>:

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

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

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