Impact: powerpc bug fix
Now that the core is using in_nmi() (added in e30e08f6, "perf_counter:
fix NMI race in task clock"), we need the powerpc perf_counter_interrupt
to call nmi_enter() and nmi_exit() in those cases where the interrupt
happens when interrupts are soft-disabled. If interrupts were
soft-enabled, we can treat it as a regular interrupt and do
irq_enter/irq_exit around the whole routine. This lets us get rid
of the test_perf_counter_pending() call at the end of
perf_counter_interrupt, thus simplifying things a little.
Signed-off-by: Paul Mackerras <[email protected]>
---
arch/powerpc/kernel/perf_counter.c | 31 +++++++++++++++++--------------
1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/perf_counter.c b/arch/powerpc/kernel/perf_counter.c
index c9d019f..bd76d0f 100644
--- a/arch/powerpc/kernel/perf_counter.c
+++ b/arch/powerpc/kernel/perf_counter.c
@@ -714,7 +714,7 @@ hw_perf_counter_init(struct perf_counter *counter)
* here so there is no possibility of being interrupted.
*/
static void record_and_restart(struct perf_counter *counter, long val,
- struct pt_regs *regs)
+ struct pt_regs *regs, int nmi)
{
s64 prev, delta, left;
int record = 0;
@@ -749,7 +749,7 @@ static void record_and_restart(struct perf_counter *counter, long val,
* Finally record data if requested.
*/
if (record)
- perf_counter_overflow(counter, 1, regs, 0);
+ perf_counter_overflow(counter, nmi, regs, 0);
}
/*
@@ -762,6 +762,17 @@ static void perf_counter_interrupt(struct pt_regs *regs)
struct perf_counter *counter;
long val;
int found = 0;
+ int nmi;
+
+ /*
+ * If interrupts were soft-disabled when this PMU interrupt
+ * occurred, treat it as an NMI.
+ */
+ nmi = !regs->softe;
+ if (nmi)
+ nmi_enter();
+ else
+ irq_enter();
for (i = 0; i < cpuhw->n_counters; ++i) {
counter = cpuhw->counter[i];
@@ -769,7 +780,7 @@ static void perf_counter_interrupt(struct pt_regs *regs)
if ((int)val < 0) {
/* counter has overflowed */
found = 1;
- record_and_restart(counter, val, regs);
+ record_and_restart(counter, val, regs, nmi);
}
}
@@ -796,18 +807,10 @@ static void perf_counter_interrupt(struct pt_regs *regs)
*/
mtspr(SPRN_MMCR0, cpuhw->mmcr[0]);
- /*
- * If we need a wakeup, check whether interrupts were soft-enabled
- * when we took the interrupt. If they were, we can wake stuff up
- * immediately; otherwise we'll have do the wakeup when interrupts
- * get soft-enabled.
- */
- if (test_perf_counter_pending() && regs->softe) {
- irq_enter();
- clear_perf_counter_pending();
- perf_counter_do_pending();
+ if (nmi)
+ nmi_exit();
+ else
irq_exit();
- }
}
void hw_perf_counter_setup(int cpu)
--
1.5.6.3
* Paul Mackerras <[email protected]> wrote:
> Impact: powerpc bug fix
>
> Now that the core is using in_nmi() (added in e30e08f6,
> "perf_counter: fix NMI race in task clock"), we need the powerpc
> perf_counter_interrupt to call nmi_enter() and nmi_exit() in those
> cases where the interrupt happens when interrupts are
> soft-disabled. If interrupts were soft-enabled, we can treat it
> as a regular interrupt and do irq_enter/irq_exit around the whole
> routine. This lets us get rid of the test_perf_counter_pending()
> call at the end of perf_counter_interrupt, thus simplifying things
> a little.
applied, thanks Paul!
I'm wondering, what was the real impact? Was it a crash or some
other misbehavior? This impact line:
Impact: powerpc bug fix
is a bit too generic to be useful in practice. Something like:
Impact: fix stuck NMIs on powerpc
Impact: fix NMI crash on powerpc
would have been more descriptive about the real, hands-on impact of
this patch.
Ingo
Commit-ID: 1d79dfed02582ef84f50e9abf5f29209673404ca
Gitweb: http://git.kernel.org/tip/1d79dfed02582ef84f50e9abf5f29209673404ca
Author: Paul Mackerras <[email protected]>
AuthorDate: Thu, 9 Apr 2009 14:42:56 +1000
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 9 Apr 2009 07:16:53 +0200
perf_counter: powerpc: add nmi_enter/nmi_exit calls
Impact: powerpc bug fix
Now that the core is using in_nmi() (added in e30e08f6, "perf_counter:
fix NMI race in task clock"), we need the powerpc perf_counter_interrupt
to call nmi_enter() and nmi_exit() in those cases where the interrupt
happens when interrupts are soft-disabled.
If interrupts were soft-enabled, we can treat it as a regular interrupt
and do irq_enter/irq_exit around the whole routine. This lets us get rid
of the test_perf_counter_pending() call at the end of
perf_counter_interrupt, thus simplifying things a little.
Signed-off-by: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/powerpc/kernel/perf_counter.c | 31 +++++++++++++++++--------------
1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/perf_counter.c b/arch/powerpc/kernel/perf_counter.c
index c9d019f..bd76d0f 100644
--- a/arch/powerpc/kernel/perf_counter.c
+++ b/arch/powerpc/kernel/perf_counter.c
@@ -714,7 +714,7 @@ hw_perf_counter_init(struct perf_counter *counter)
* here so there is no possibility of being interrupted.
*/
static void record_and_restart(struct perf_counter *counter, long val,
- struct pt_regs *regs)
+ struct pt_regs *regs, int nmi)
{
s64 prev, delta, left;
int record = 0;
@@ -749,7 +749,7 @@ static void record_and_restart(struct perf_counter *counter, long val,
* Finally record data if requested.
*/
if (record)
- perf_counter_overflow(counter, 1, regs, 0);
+ perf_counter_overflow(counter, nmi, regs, 0);
}
/*
@@ -762,6 +762,17 @@ static void perf_counter_interrupt(struct pt_regs *regs)
struct perf_counter *counter;
long val;
int found = 0;
+ int nmi;
+
+ /*
+ * If interrupts were soft-disabled when this PMU interrupt
+ * occurred, treat it as an NMI.
+ */
+ nmi = !regs->softe;
+ if (nmi)
+ nmi_enter();
+ else
+ irq_enter();
for (i = 0; i < cpuhw->n_counters; ++i) {
counter = cpuhw->counter[i];
@@ -769,7 +780,7 @@ static void perf_counter_interrupt(struct pt_regs *regs)
if ((int)val < 0) {
/* counter has overflowed */
found = 1;
- record_and_restart(counter, val, regs);
+ record_and_restart(counter, val, regs, nmi);
}
}
@@ -796,18 +807,10 @@ static void perf_counter_interrupt(struct pt_regs *regs)
*/
mtspr(SPRN_MMCR0, cpuhw->mmcr[0]);
- /*
- * If we need a wakeup, check whether interrupts were soft-enabled
- * when we took the interrupt. If they were, we can wake stuff up
- * immediately; otherwise we'll have do the wakeup when interrupts
- * get soft-enabled.
- */
- if (test_perf_counter_pending() && regs->softe) {
- irq_enter();
- clear_perf_counter_pending();
- perf_counter_do_pending();
+ if (nmi)
+ nmi_exit();
+ else
irq_exit();
- }
}
void hw_perf_counter_setup(int cpu)
Ingo Molnar writes:
> I'm wondering, what was the real impact? Was it a crash or some
> other misbehavior? This impact line:
>
> Impact: powerpc bug fix
>
> is a bit too generic to be useful in practice. Something like:
>
> Impact: fix stuck NMIs on powerpc
> Impact: fix NMI crash on powerpc
>
> would have been more descriptive about the real, hands-on impact of
> this patch.
I was looking at Peter's patches and I noticed he used in_nmi(), and I
wondered "what's that?", so I went looking and found it, and realized
that I needed to be calling nmi_enter/exit for it to work. I never
actually booted a kernel that had the patch to use in_nmi() but not my
patch to call nmi_enter/exit.
The impact would have been that in_nmi() always returned 0, so I
expect that I would have seen deadlocks and/or memory corruption had I
booted a kernel without my fix.
Paul.
Commit-ID: ca8f2d7f019a8547f39ddb9ed0144932f12807f2
Gitweb: http://git.kernel.org/tip/ca8f2d7f019a8547f39ddb9ed0144932f12807f2
Author: Paul Mackerras <[email protected]>
AuthorDate: Thu, 9 Apr 2009 14:42:56 +1000
Committer: Ingo Molnar <[email protected]>
CommitDate: Thu, 9 Apr 2009 07:56:08 +0200
perf_counter: powerpc: add nmi_enter/nmi_exit calls
Impact: fix potential deadlocks on powerpc
Now that the core is using in_nmi() (added in e30e08f6, "perf_counter:
fix NMI race in task clock"), we need the powerpc perf_counter_interrupt
to call nmi_enter() and nmi_exit() in those cases where the interrupt
happens when interrupts are soft-disabled.
If interrupts were soft-enabled, we can treat it as a regular interrupt
and do irq_enter/irq_exit around the whole routine. This lets us get rid
of the test_perf_counter_pending() call at the end of
perf_counter_interrupt, thus simplifying things a little.
Signed-off-by: Paul Mackerras <[email protected]>
Cc: Peter Zijlstra <[email protected]>
LKML-Reference: <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
arch/powerpc/kernel/perf_counter.c | 31 +++++++++++++++++--------------
1 files changed, 17 insertions(+), 14 deletions(-)
diff --git a/arch/powerpc/kernel/perf_counter.c b/arch/powerpc/kernel/perf_counter.c
index c9d019f..bd76d0f 100644
--- a/arch/powerpc/kernel/perf_counter.c
+++ b/arch/powerpc/kernel/perf_counter.c
@@ -714,7 +714,7 @@ hw_perf_counter_init(struct perf_counter *counter)
* here so there is no possibility of being interrupted.
*/
static void record_and_restart(struct perf_counter *counter, long val,
- struct pt_regs *regs)
+ struct pt_regs *regs, int nmi)
{
s64 prev, delta, left;
int record = 0;
@@ -749,7 +749,7 @@ static void record_and_restart(struct perf_counter *counter, long val,
* Finally record data if requested.
*/
if (record)
- perf_counter_overflow(counter, 1, regs, 0);
+ perf_counter_overflow(counter, nmi, regs, 0);
}
/*
@@ -762,6 +762,17 @@ static void perf_counter_interrupt(struct pt_regs *regs)
struct perf_counter *counter;
long val;
int found = 0;
+ int nmi;
+
+ /*
+ * If interrupts were soft-disabled when this PMU interrupt
+ * occurred, treat it as an NMI.
+ */
+ nmi = !regs->softe;
+ if (nmi)
+ nmi_enter();
+ else
+ irq_enter();
for (i = 0; i < cpuhw->n_counters; ++i) {
counter = cpuhw->counter[i];
@@ -769,7 +780,7 @@ static void perf_counter_interrupt(struct pt_regs *regs)
if ((int)val < 0) {
/* counter has overflowed */
found = 1;
- record_and_restart(counter, val, regs);
+ record_and_restart(counter, val, regs, nmi);
}
}
@@ -796,18 +807,10 @@ static void perf_counter_interrupt(struct pt_regs *regs)
*/
mtspr(SPRN_MMCR0, cpuhw->mmcr[0]);
- /*
- * If we need a wakeup, check whether interrupts were soft-enabled
- * when we took the interrupt. If they were, we can wake stuff up
- * immediately; otherwise we'll have do the wakeup when interrupts
- * get soft-enabled.
- */
- if (test_perf_counter_pending() && regs->softe) {
- irq_enter();
- clear_perf_counter_pending();
- perf_counter_do_pending();
+ if (nmi)
+ nmi_exit();
+ else
irq_exit();
- }
}
void hw_perf_counter_setup(int cpu)
* Paul Mackerras <[email protected]> wrote:
> Ingo Molnar writes:
>
> > I'm wondering, what was the real impact? Was it a crash or some
> > other misbehavior? This impact line:
> >
> > Impact: powerpc bug fix
> >
> > is a bit too generic to be useful in practice. Something like:
> >
> > Impact: fix stuck NMIs on powerpc
> > Impact: fix NMI crash on powerpc
> >
> > would have been more descriptive about the real, hands-on impact of
> > this patch.
>
> I was looking at Peter's patches and I noticed he used in_nmi(),
> and I wondered "what's that?", so I went looking and found it, and
> realized that I needed to be calling nmi_enter/exit for it to
> work. I never actually booted a kernel that had the patch to use
> in_nmi() but not my patch to call nmi_enter/exit.
>
> The impact would have been that in_nmi() always returned 0, so I
> expect that I would have seen deadlocks and/or memory corruption
> had I booted a kernel without my fix.
thanks, i've amended the commit with:
Impact: fix potential deadlocks on powerpc
i try to keep the habit of good impact-lines even for development
branches - they make the -stable tagging effort quite a bit more
robust once a piece of code is upstream.
They also give good, standard looking feedback about the kinds of
problems/risks that a given topic has triggered historically.
For example:
earth4:~/tip> git log v2.6.29..v2.6.30-rc1 arch/x86/kernel/apic/ | \
grep Impact: | sort | uniq -c | sort -n
1 Impact: build fix
1 Impact: build fix, cleanup
1 Impact: cleanup, paranoia
1 Impact: cleanup, reduce memory usage for CONFIG_CPUMASK_OFFSTACK=y
1 Impact: cleanup, remove cpumask from stack
1 Impact: fix bug with irq-descriptor moving when logical flat
1 Impact: fix incorrect error message
1 Impact: fix possible race
1 Impact: fix spurious IRQs
1 Impact: get correct smp_affinity as user requested
1 Impact: interface augmentation (not yet used)
1 Impact: make kexec work with x2apic
1 Impact: optimize APIC IPI related barriers
1 Impact: simplification
10 Impact: cleanup
Shows that we had 5-6 runtime problems (mostly misbehavior) in the
APIC code during the last development window.
Or:
earth4:~/tip> git log v2.6.29..v2.6.30-rc1 kernel/sched.c | grep
Impact: | sort | uniq -c | sort -n
1 Impact: cleanup, micro-optimization
1 Impact: cleanup, new schedstat ABI
1 Impact: fix boot crash
1 Impact: fix circular locking
1 Impact: fix function graph trace hang / drop pointless softirq on UP
1 Impact: fix to preempt trace triggering lockdep check_flag failure
1 Impact: more precise avg_overlap metric - better load-balancing
1 Impact: struct rq size optimization
2 Impact: micro-optimization
12 Impact: cleanup
Shows that we had ~4 runtime problems (crashes or lockdep asserts)
in the schedule during the last development window.
The shortlog tends to clone the patch-submission subject lines and
tends to be more detailed and differently structured - so it's a lot
harder to extract this information from it, at a glance.
So the impact line standardizes this kind of risk/impact info - and
while we are still experimenting with exactly how to shape it (you
can see several small variants), it's already pretty useful in the
history too - not just while queueing it up.
Ingo