Use preempt_schedule_irq to prevent infinite irq-entry and
eventual stack overflow problems with fast-paced IRQ sources.
This kind of problems has been observed on the PASemi Electra IDE
controller. We have to make sure we are soft-disabled before calling
preempt_schedule_irq and hard disable interrupts after that
to avoid unrecoverable exceptions.
This patch also moves the "clrrdi r9,r1,THREAD_SHIFT" out of
the #ifdef CONFIG_PPC_BOOK3E scope, since r9 is clobbered
and has to be restored in both cases.
Signed-off-by: Valentine Barshak <redacted>
---
arch/powerpc/kernel/entry_64.S | 25 ++++++-------------------
1 file changed, 6 insertions(+), 19 deletions(-)
diff -pruN linux-2.6.orig/arch/powerpc/kernel/entry_64.S linux-2.6.new/arch/powerpc/kernel/entry_64.S
@@ -660,33 +660,20 @@ do_work:bnerestore/*herewearepreemptingthecurrenttask*/1:-#ifdef CONFIG_TRACE_IRQFLAGS-bl.trace_hardirqs_on-/*Note:wejustclobberedr10whichusedtocontaintheprevious-*MSRbeforethehard-disablingdonebythecallerofdo_work.-*Wedon't have that value anymore, but it doesn'tmatteras-*wewillhard-enableunconditionally,wecanjustreloadthe-*currentMSRintor10-*/-mfmsrr10-#endif /* CONFIG_TRACE_IRQFLAGS */-lir0,1+/*ensurewearesoft-disabled*/+lir0,0stbr0,PACASOFTIRQEN(r13)-stbr0,PACAHARDIRQEN(r13)+bl.preempt_schedule_irq+/*hard-disableinterrupts*/#ifdef CONFIG_PPC_BOOK3E-wrteei1-bl.preempt_schedulewrteei0#else-orir10,r10,MSR_EE-mtmsrdr10,1/*reenableinterrupts*/-bl.preempt_schedulemfmsrr10-clrrdir9,r1,THREAD_SHIFT-rldiclr10,r10,48,1/*disableinterruptsagain*/+rldiclr10,r10,48,1rotldir10,r10,16mtmsrdr10,1#endif /* CONFIG_PPC_BOOK3E */+clrrdir9,r1,THREAD_SHIFTldr4,TI_FLAGS(r9)andi.r0,r4,_TIF_NEED_RESCHEDbne1b
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-26 23:55:35
On Mon, 2009-10-19 at 22:28 +0400, Valentine Barshak wrote:
Use preempt_schedule_irq to prevent infinite irq-entry and
eventual stack overflow problems with fast-paced IRQ sources.
This kind of problems has been observed on the PASemi Electra IDE
controller. We have to make sure we are soft-disabled before calling
preempt_schedule_irq and hard disable interrupts after that
to avoid unrecoverable exceptions.
This patch also moves the "clrrdi r9,r1,THREAD_SHIFT" out of
the #ifdef CONFIG_PPC_BOOK3E scope, since r9 is clobbered
and has to be restored in both cases.
So I _think_ that the irqs on/off accounting for lockdep isn't quite
right. What do you think of this slightly modified version ? I've only
done a quick boot test on a G5 with lockdep enabled and a played a bit,
nothing shows up so far but it's definitely not conclusive.
The main difference is that I call trace_hardirqs_off to "advertise"
the fact that we are soft-disabling (it could be a dup, but at this
stage this is no big deal, but it's not always, like in syscall return
the kernel thinks we have interrupts enabled and could thus get out
of sync without that).
I also mark the PACA hard disable to reflect the MSR:EE state before
calling into preempt_schedule_irq().
---
[PATCH v2] powerpc: Use preempt_schedule_irq instead of preempt_schedule when returning from exceptions
Use preempt_schedule_irq to prevent infinite irq-entry and
eventual stack overflow problems with fast-paced IRQ sources.
This kind of problems has been observed on the PASemi Electra IDE
controller. We have to make sure we are soft-disabled before calling
preempt_schedule_irq and hard disable interrupts after that
to avoid unrecoverable exceptions.
This patch also moves the "clrrdi r9,r1,THREAD_SHIFT" out of
the #ifdef CONFIG_PPC_BOOK3E scope, since r9 is clobbered
and has to be restored in both cases.
Signed-off-by: Valentine Barshak <redacted>
Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
arch/powerpc/kernel/entry_64.S | 38 +++++++++++++++++++++-----------------
1 files changed, 21 insertions(+), 17 deletions(-)
@@ -659,34 +659,38 @@ do_work:crandceq,cr1*4+eq,eqbnerestore/*herewearepreemptingthecurrenttask*/-1:+/*ensurewearesoft-disabled+**/+lir0,0+stbr0,PACASOFTIRQEN(r13)+/*TracetheIRQstatechange*/#ifdef CONFIG_TRACE_IRQFLAGS-bl.trace_hardirqs_on-/*Note:wejustclobberedr10whichusedtocontaintheprevious-*MSRbeforethehard-disablingdonebythecallerofdo_work.-*Wedon't have that value anymore, but it doesn'tmatteras-*wewillhard-enableunconditionally,wecanjustreloadthe-*currentMSRintor10-*/+bl.trace_hardirqs_off+#endif+1:/*Andmakesurewearehard-enabled*/+#ifdef CONFIG_PPC_BOOK3E+wrteei1+#elsemfmsrr10-#endif /* CONFIG_TRACE_IRQFLAGS */+orir10,r10,MSR_EE+mtmsrdr10,1+#endiflir0,1-stbr0,PACASOFTIRQEN(r13)stbr0,PACAHARDIRQEN(r13)+/*CalltheschedulerwithsoftIRQsoff*/+bl.preempt_schedule_irq+/*hard-disableinterruptsagain*/#ifdef CONFIG_PPC_BOOK3E-wrteei1-bl.preempt_schedulewrteei0#else-orir10,r10,MSR_EE-mtmsrdr10,1/*reenableinterrupts*/-bl.preempt_schedulemfmsrr10-clrrdir9,r1,THREAD_SHIFT-rldiclr10,r10,48,1/*disableinterruptsagain*/+rldiclr10,r10,48,1rotldir10,r10,16mtmsrdr10,1#endif /* CONFIG_PPC_BOOK3E */+lir0,0+stbr0,PACAHARDIRQEN(r13)+clrrdir9,r1,THREAD_SHIFTldr4,TI_FLAGS(r9)andi.r0,r4,_TIF_NEED_RESCHEDbne1b
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-27 05:41:30
So I _think_ that the irqs on/off accounting for lockdep isn't quite
right. What do you think of this slightly modified version ? I've only
done a quick boot test on a G5 with lockdep enabled and a played a bit,
nothing shows up so far but it's definitely not conclusive.
The main difference is that I call trace_hardirqs_off to "advertise"
the fact that we are soft-disabling (it could be a dup, but at this
stage this is no big deal, but it's not always, like in syscall return
the kernel thinks we have interrupts enabled and could thus get out
of sync without that).
I also mark the PACA hard disable to reflect the MSR:EE state before
calling into preempt_schedule_irq().
Allright, second thought :-)
It's probably simpler to just keep hardirqs off. Code is smaller and
simpler and the scheduler will re-enable them soon enough anyways.
This version of the patch also spaces the code a bit and adds comments
which makes them (the code and the patch) more readable.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[PATCH v3] powerpc/ppc64: Use preempt_schedule_irq instead of preempt_schedule
Based on an original patch by Valentine Barshak [off-list ref]
Use preempt_schedule_irq to prevent infinite irq-entry and
eventual stack overflow problems with fast-paced IRQ sources.
This kind of problems has been observed on the PASemi Electra IDE
controller. We have to make sure we are soft-disabled before calling
preempt_schedule_irq and hard disable interrupts after that
to avoid unrecoverable exceptions.
This patch also moves the "clrrdi r9,r1,THREAD_SHIFT" out of
the #ifdef CONFIG_PPC_BOOK3E scope, since r9 is clobbered
and has to be restored in both cases.
---
arch/powerpc/kernel/entry_64.S | 41 ++++++++++++++++++++-------------------
1 files changed, 21 insertions(+), 20 deletions(-)
@@ -658,42 +658,43 @@ do_work:cmpdir0,0crandceq,cr1*4+eq,eqbnerestore-/*herewearepreemptingthecurrenttask*/-1:-#ifdef CONFIG_TRACE_IRQFLAGS-bl.trace_hardirqs_on-/*Note:wejustclobberedr10whichusedtocontaintheprevious-*MSRbeforethehard-disablingdonebythecallerofdo_work.-*Wedon't have that value anymore, but it doesn'tmatteras-*wewillhard-enableunconditionally,wecanjustreloadthe-*currentMSRintor10++/*Herewearepreemptingthecurrenttask.+*+*Ensureinterruptsaresoft-disabled.Wealsoproperlymark+*thePACAtoreflectthefactthattheyarehard-disabled+*andtracethechange*/-mfmsrr10-#endif /* CONFIG_TRACE_IRQFLAGS */-lir0,1+lir0,0stbr0,PACASOFTIRQEN(r13)stbr0,PACAHARDIRQEN(r13)+TRACE_DISABLE_INTS++/*CalltheschedulerwithsoftIRQsoff*/+1:bl.preempt_schedule_irq++/*Hard-disableinterruptsagain (andupdatePACA)*/#ifdef CONFIG_PPC_BOOK3E-wrteei1-bl.preempt_schedulewrteei0#else-orir10,r10,MSR_EE-mtmsrdr10,1/*reenableinterrupts*/-bl.preempt_schedulemfmsrr10-clrrdir9,r1,THREAD_SHIFT-rldiclr10,r10,48,1/*disableinterruptsagain*/+rldiclr10,r10,48,1rotldir10,r10,16mtmsrdr10,1#endif /* CONFIG_PPC_BOOK3E */+lir0,0+stbr0,PACAHARDIRQEN(r13)++/*Re-testflagsandeventuallyloop*/+clrrdir9,r1,THREAD_SHIFTldr4,TI_FLAGS(r9)andi.r0,r4,_TIF_NEED_RESCHEDbne1bbrestoreuser_work:-#endif+#endif /* CONFIG_PREEMPT */+/*Enableinterrupts*/#ifdef CONFIG_PPC_BOOK3Ewrteei1
On Wed, 2009-10-28 at 22:19 +0300, Valentine wrote:
quoted
I'm just not sure that we need to clear HARDIRQEN here, since we don't
really hard-disable the the interrupts.
We do, or rather, we come in with the interrupts hard disabled, no ?
Yes, looks like the interrupts are disabled at this point (before
preempt_schedule_irq) most of the times, but we don't hard-disable them
here. We just soft-disable them to make preempt_schedule_irq happy. Even
if an interrupt fires, it will be hard-disabled and the hardirq flag
will be cleared by the exception handler right away. I just think that
there's no need to clear hardirq flag if we don't clear MSR_EE.
Thanks,
Val.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-28 22:40:22
On Thu, 2009-10-29 at 00:28 +0300, Valentine wrote:
Benjamin Herrenschmidt wrote:
quoted
On Wed, 2009-10-28 at 22:19 +0300, Valentine wrote:
quoted
I'm just not sure that we need to clear HARDIRQEN here, since we don't
really hard-disable the the interrupts.
We do, or rather, we come in with the interrupts hard disabled, no ?
Yes, looks like the interrupts are disabled at this point (before
preempt_schedule_irq) most of the times, but we don't hard-disable them
here. We just soft-disable them to make preempt_schedule_irq happy. Even
if an interrupt fires, it will be hard-disabled and the hardirq flag
will be cleared by the exception handler right away. I just think that
there's no need to clear hardirq flag if we don't clear MSR_EE.
My point is that MSR_EE _is_ already clear... isn't it ? So either we
set it back, or we clear HARDIRQEN to reflect it. It will be re-enable
as soon as preempt_schedule_irq() calls local_irq_enable() which is soon
enough anyways.
Also that avoids perf interrupt sneaking in since those act as NMIs in
that regard and -will- get in even when soft disabled.
Cheers,
Ben.
So I _think_ that the irqs on/off accounting for lockdep isn't quite
right. What do you think of this slightly modified version ? I've only
done a quick boot test on a G5 with lockdep enabled and a played a bit,
nothing shows up so far but it's definitely not conclusive.
The main difference is that I call trace_hardirqs_off to "advertise"
the fact that we are soft-disabling (it could be a dup, but at this
stage this is no big deal, but it's not always, like in syscall return
the kernel thinks we have interrupts enabled and could thus get out
of sync without that).
I also mark the PACA hard disable to reflect the MSR:EE state before
calling into preempt_schedule_irq().
Allright, second thought :-)
It's probably simpler to just keep hardirqs off. Code is smaller and
simpler and the scheduler will re-enable them soon enough anyways.
This version of the patch also spaces the code a bit and adds comments
which makes them (the code and the patch) more readable.
This one seems to work fine on pasemi and another maple-compatible board.
quoted hunk
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
[PATCH v3] powerpc/ppc64: Use preempt_schedule_irq instead of preempt_schedule
Based on an original patch by Valentine Barshak [off-list ref]
Use preempt_schedule_irq to prevent infinite irq-entry and
eventual stack overflow problems with fast-paced IRQ sources.
This kind of problems has been observed on the PASemi Electra IDE
controller. We have to make sure we are soft-disabled before calling
preempt_schedule_irq and hard disable interrupts after that
to avoid unrecoverable exceptions.
This patch also moves the "clrrdi r9,r1,THREAD_SHIFT" out of
the #ifdef CONFIG_PPC_BOOK3E scope, since r9 is clobbered
and has to be restored in both cases.
---
arch/powerpc/kernel/entry_64.S | 41 ++++++++++++++++++++-------------------
1 files changed, 21 insertions(+), 20 deletions(-)
@@ -658,42 +658,43 @@ do_work:cmpdir0,0crandceq,cr1*4+eq,eqbnerestore-/*herewearepreemptingthecurrenttask*/-1:-#ifdef CONFIG_TRACE_IRQFLAGS-bl.trace_hardirqs_on-/*Note:wejustclobberedr10whichusedtocontaintheprevious-*MSRbeforethehard-disablingdonebythecallerofdo_work.-*Wedon't have that value anymore, but it doesn'tmatteras-*wewillhard-enableunconditionally,wecanjustreloadthe-*currentMSRintor10++/*Herewearepreemptingthecurrenttask.+*+*Ensureinterruptsaresoft-disabled.Wealsoproperlymark+*thePACAtoreflectthefactthattheyarehard-disabled+*andtracethechange*/-mfmsrr10-#endif /* CONFIG_TRACE_IRQFLAGS */-lir0,1+lir0,0stbr0,PACASOFTIRQEN(r13)stbr0,PACAHARDIRQEN(r13)
I'm just not sure that we need to clear HARDIRQEN here, since we don't
really hard-disable the the interrupts.
Thanks,
Val.
On Thu, 2009-10-29 at 00:28 +0300, Valentine wrote:
quoted
Benjamin Herrenschmidt wrote:
quoted
On Wed, 2009-10-28 at 22:19 +0300, Valentine wrote:
quoted
I'm just not sure that we need to clear HARDIRQEN here, since we don't
really hard-disable the the interrupts.
We do, or rather, we come in with the interrupts hard disabled, no ?
Yes, looks like the interrupts are disabled at this point (before
preempt_schedule_irq) most of the times, but we don't hard-disable them
here. We just soft-disable them to make preempt_schedule_irq happy. Even
if an interrupt fires, it will be hard-disabled and the hardirq flag
will be cleared by the exception handler right away. I just think that
there's no need to clear hardirq flag if we don't clear MSR_EE.
My point is that MSR_EE _is_ already clear... isn't it ?
Yes, the MSR_EE is cleared before we jump to do_work. I'm OK with
clearing the hardirqenable flag. I just assumed that the hardirq flag
was supposed to reflect the MSR_EE state, so it looked a bit odd
clearing the MSR_EE at one place and then reflecting the change at another.
Anyway, the patch works fine.
Thanks,
Val.
So either we
set it back, or we clear HARDIRQEN to reflect it. It will be re-enable
as soon as preempt_schedule_irq() calls local_irq_enable() which is soon
enough anyways.
Also that avoids perf interrupt sneaking in since those act as NMIs in
that regard and -will- get in even when soft disabled.
Cheers,
Ben.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-10-29 00:49:42
Yes, the MSR_EE is cleared before we jump to do_work. I'm OK with
clearing the hardirqenable flag. I just assumed that the hardirq flag
was supposed to reflect the MSR_EE state, so it looked a bit odd
clearing the MSR_EE at one place and then reflecting the change at another.
Yeah well, it is supposed to reflect EE in the "general case", it's just
that in the exception entry/exit, we take shortcuts when turning EE off
for short amount of times without reflecting it in the PACA. This is
why, in this case, since we are going back to C code, I want to have it
"fixed up" to reflect reality.
Cheers,
Ben.
Anyway, the patch works fine.
Thanks,
Val.
So either we
quoted
set it back, or we clear HARDIRQEN to reflect it. It will be re-enable
as soon as preempt_schedule_irq() calls local_irq_enable() which is soon
enough anyways.
Also that avoids perf interrupt sneaking in since those act as NMIs in
that regard and -will- get in even when soft disabled.
Cheers,
Ben.
Yes, the MSR_EE is cleared before we jump to do_work. I'm OK with
clearing the hardirqenable flag. I just assumed that the hardirq flag
was supposed to reflect the MSR_EE state, so it looked a bit odd
clearing the MSR_EE at one place and then reflecting the change at another.
Yeah well, it is supposed to reflect EE in the "general case", it's just
that in the exception entry/exit, we take shortcuts when turning EE off
for short amount of times without reflecting it in the PACA. This is
why, in this case, since we are going back to C code, I want to have it
"fixed up" to reflect reality.
Ben, this one works fine. Are you going to pick it?
Thanks,
Val.
From: Benjamin Herrenschmidt <benh@kernel.crashing.org> Date: 2009-11-06 22:49:36
On Sat, 2009-11-07 at 01:38 +0300, Valentine wrote:
Benjamin Herrenschmidt wrote:
quoted
quoted
Yes, the MSR_EE is cleared before we jump to do_work. I'm OK with
clearing the hardirqenable flag. I just assumed that the hardirq flag
was supposed to reflect the MSR_EE state, so it looked a bit odd
clearing the MSR_EE at one place and then reflecting the change at another.
Yeah well, it is supposed to reflect EE in the "general case", it's just
that in the exception entry/exit, we take shortcuts when turning EE off
for short amount of times without reflecting it in the PACA. This is
why, in this case, since we are going back to C code, I want to have it
"fixed up" to reflect reality.
Ben, this one works fine. Are you going to pick it?
Already upstream:
Gitweb:
http://git.kernel.org/linus/4f917ba3d5ee9c98d60fa357e799942df8412de3
Commit: 4f917ba3d5ee9c98d60fa357e799942df8412de3
Parent: 01deab98e3ad8ff27243a8d5f8dd746c7110ae4f
Author: Benjamin Herrenschmidt [off-list ref]
AuthorDate: Mon Oct 26 19:41:17 2009 +0000
Committer: Benjamin Herrenschmidt [off-list ref]
CommitDate: Tue Oct 27 16:42:43 2009 +1100
powerpc/ppc64: Use preempt_schedule_irq instead of preempt_schedule
Based on an original patch by Valentine Barshak [off-list ref]
Use preempt_schedule_irq to prevent infinite irq-entry and
eventual stack overflow problems with fast-paced IRQ sources.
.../...
Now, it might be a good idea to do a -stable variant of it for 2.6.31
and back, but that will have to be a separate patch due to the new
Book3E churn in .32
Cheers,
Ben.