We're seeing some perf-related crashes on powerpc related to having
irqs in an inconsistent state (soft-enable vs. hard-enable
vs. trace-irqs state) when entering perf_event_do_pending().
We're fixing that, but along the way we have struck a question about
what conditions are required on entry to perf_event_do_pending.
Its use of __get_cpu_var implies that it at least needs to be called
with either interrupts or preemption disabled. Does it in fact need
to be called with irqs off? Do we need to call irq_enter()/irq_exit()
around it? Are there any other requirements that people can think of?
Paul.
On Wed, 2010-01-13 at 15:14 +1100, Paul Mackerras wrote:
> We're seeing some perf-related crashes on powerpc related to having
> irqs in an inconsistent state (soft-enable vs. hard-enable
> vs. trace-irqs state) when entering perf_event_do_pending().
> We're fixing that, but along the way we have struck a question about
> what conditions are required on entry to perf_event_do_pending.
>
> Its use of __get_cpu_var implies that it at least needs to be called
> with either interrupts or preemption disabled. Does it in fact need
> to be called with irqs off? Do we need to call irq_enter()/irq_exit()
> around it? Are there any other requirements that people can think of?
Right, so when I wrote it all it required was preempt disabled, but then
I added all that disable stuff (perf_pending_event():
event->pending_disable) and that requires IRQs disabled because it calls
__perf_event_disable() which takes ctx->lock, which is supposed to be an
IRQ safe lock.
On x86 we always run it from a self-ipi, which is I guess why the
generic timer softirq callback never triggered for me, because that
looks broken.
So in short, I think perf_event_do_pending() requires full IRQ context,
if that includes calling irq_enter()/irq_exit() then yes.
Something like the below ought to do I guess..
---
kernel/timer.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/kernel/timer.c b/kernel/timer.c
index 15533b7..c61a794 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1198,6 +1198,7 @@ void update_process_times(int user_tick)
run_local_timers();
rcu_check_callbacks(cpu, user_tick);
printk_tick();
+ perf_event_do_pending();
scheduler_tick();
run_posix_cpu_timers(p);
}
@@ -1209,8 +1210,6 @@ static void run_timer_softirq(struct softirq_action *h)
{
struct tvec_base *base = __get_cpu_var(tvec_bases);
- perf_event_do_pending();
-
hrtimer_run_pending();
if (time_after_eq(jiffies, base->timer_jiffies))
Commit-ID: fe432200abb0d64f409895168d9ad8fbb9d8e6c6
Gitweb: http://git.kernel.org/tip/fe432200abb0d64f409895168d9ad8fbb9d8e6c6
Author: Peter Zijlstra <[email protected]>
AuthorDate: Mon, 18 Jan 2010 09:08:26 +0100
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 21 Jan 2010 13:40:39 +0100
perf: Fix perf_event_do_pending() fallback callsite
Paul questioned the context in which we should call
perf_event_do_pending(). After looking at that I found that it should be
called from IRQ context these days, however the fallback call-site is
placed in softirq context. Ammend this by placing the callback in the IRQ
timer path.
Reported-by: Paul Mackerras <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
LKML-Reference: <1263374859.4244.192.camel@laptop>
Signed-off-by: Ingo Molnar <[email protected]>
---
kernel/timer.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/kernel/timer.c b/kernel/timer.c
index 15533b7..c61a794 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -1198,6 +1198,7 @@ void update_process_times(int user_tick)
run_local_timers();
rcu_check_callbacks(cpu, user_tick);
printk_tick();
+ perf_event_do_pending();
scheduler_tick();
run_posix_cpu_timers(p);
}
@@ -1209,8 +1210,6 @@ static void run_timer_softirq(struct softirq_action *h)
{
struct tvec_base *base = __get_cpu_var(tvec_bases);
- perf_event_do_pending();
-
hrtimer_run_pending();
if (time_after_eq(jiffies, base->timer_jiffies))