| Message ID | 20220420141657.771442-1-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
| Series | powerpc/time: Always set decrementer in timer_interrupt() | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_sparse | fail | 4 of 4 jobs failed. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | fail | 13 of 20 jobs failed. |
| snowpatch_ozlabs/github-powerpc_ppctests | fail | 7 of 8 jobs failed. |
| snowpatch_ozlabs/github-powerpc_clang | fail | 7 of 7 jobs failed. |
| snowpatch_ozlabs/github-powerpc_selftests | fail | 8 of 8 jobs failed. |
Hello, On Thu, Apr 21, 2022 at 12:16:57AM +1000, Michael Ellerman wrote: > This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: > Don't enable MSR[EE] in irq handlers unless perf is in use"). > > Prior to that commit, we always set the decrementer in > timer_interrupt(), to clear the timer interrupt. Otherwise we could end > up continuously taking timer interrupts. > > When high res timers are enabled there is no problem seen with leaving > the decrementer untouched in timer_interrupt(), because it will be > programmed via hrtimer_interrupt() -> tick_program_event() -> > clockevents_program_event() -> decrementer_set_next_event(). > > However with CONFIG_HIGH_RES_TIMERS=n or booting with highres=off, we How difficult is it to detect this condition? Maybe detecting this could be just added? Thanks Michal > see a stall/lockup, because tick_nohz_handler() does not cause a > reprogram of the decrementer, leading to endless timer interrupts. > Example trace: > > [ 1.898617][ T7] Freeing initrd memory: 2624K^M > [ 22.680919][ C1] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:^M > [ 22.682281][ C1] rcu: 0-....: (25 ticks this GP) idle=073/0/0x1 softirq=10/16 fqs=1050 ^M > [ 22.682851][ C1] (detected by 1, t=2102 jiffies, g=-1179, q=476)^M > [ 22.683649][ C1] Sending NMI from CPU 1 to CPUs 0:^M > [ 22.685252][ C0] NMI backtrace for cpu 0^M > [ 22.685649][ C0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.16.0-rc2-00185-g0faf20a1ad16 #145^M > [ 22.686393][ C0] NIP: c000000000016d64 LR: c000000000f6cca4 CTR: c00000000019c6e0^M > [ 22.686774][ C0] REGS: c000000002833590 TRAP: 0500 Not tainted (5.16.0-rc2-00185-g0faf20a1ad16)^M > [ 22.687222][ C0] MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 24000222 XER: 00000000^M > [ 22.688297][ C0] CFAR: c00000000000c854 IRQMASK: 0 ^M > ... > [ 22.692637][ C0] NIP [c000000000016d64] arch_local_irq_restore+0x174/0x250^M > [ 22.694443][ C0] LR [c000000000f6cca4] __do_softirq+0xe4/0x3dc^M > [ 22.695762][ C0] Call Trace:^M > [ 22.696050][ C0] [c000000002833830] [c000000000f6cc80] __do_softirq+0xc0/0x3dc (unreliable)^M > [ 22.697377][ C0] [c000000002833920] [c000000000151508] __irq_exit_rcu+0xd8/0x130^M > [ 22.698739][ C0] [c000000002833950] [c000000000151730] irq_exit+0x20/0x40^M > [ 22.699938][ C0] [c000000002833970] [c000000000027f40] timer_interrupt+0x270/0x460^M > [ 22.701119][ C0] [c0000000028339d0] [c0000000000099a8] decrementer_common_virt+0x208/0x210^M > > Possibly this should be fixed in the lowres timing code, but that would > be a generic change and could take some time and may not backport > easily, so for now make the programming of the decrementer unconditional > again in timer_interrupt() to avoid the stall/lockup. > > Fixes: 0faf20a1ad16 ("powerpc/64s/interrupt: Don't enable MSR[EE] in irq handlers unless perf is in use") > Reported-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/kernel/time.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c > index f5cbfe5efd25..f80cce0e3899 100644 > --- a/arch/powerpc/kernel/time.c > +++ b/arch/powerpc/kernel/time.c > @@ -615,23 +615,22 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt) > return; > } > > - /* Conditionally hard-enable interrupts. */ > - if (should_hard_irq_enable()) { > - /* > - * Ensure a positive value is written to the decrementer, or > - * else some CPUs will continue to take decrementer exceptions. > - * When the PPC_WATCHDOG (decrementer based) is configured, > - * keep this at most 31 bits, which is about 4 seconds on most > - * systems, which gives the watchdog a chance of catching timer > - * interrupt hard lockups. > - */ > - if (IS_ENABLED(CONFIG_PPC_WATCHDOG)) > - set_dec(0x7fffffff); > - else > - set_dec(decrementer_max); > + /* > + * Ensure a positive value is written to the decrementer, or > + * else some CPUs will continue to take decrementer exceptions. > + * When the PPC_WATCHDOG (decrementer based) is configured, > + * keep this at most 31 bits, which is about 4 seconds on most > + * systems, which gives the watchdog a chance of catching timer > + * interrupt hard lockups. > + */ > + if (IS_ENABLED(CONFIG_PPC_WATCHDOG)) > + set_dec(0x7fffffff); > + else > + set_dec(decrementer_max); > > + /* Conditionally hard-enable interrupts. */ > + if (should_hard_irq_enable()) > do_hard_irq_enable(); > - } > > #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC) > if (atomic_read(&ppc_n_lost_interrupts) != 0) > -- > 2.34.1 >
Excerpts from Michael Ellerman's message of April 21, 2022 12:16 am: > This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: > Don't enable MSR[EE] in irq handlers unless perf is in use"). > > Prior to that commit, we always set the decrementer in > timer_interrupt(), to clear the timer interrupt. Otherwise we could end > up continuously taking timer interrupts. > > When high res timers are enabled there is no problem seen with leaving > the decrementer untouched in timer_interrupt(), because it will be > programmed via hrtimer_interrupt() -> tick_program_event() -> > clockevents_program_event() -> decrementer_set_next_event(). > > However with CONFIG_HIGH_RES_TIMERS=n or booting with highres=off, we > see a stall/lockup, because tick_nohz_handler() does not cause a > reprogram of the decrementer, leading to endless timer interrupts. > Example trace: > > [ 1.898617][ T7] Freeing initrd memory: 2624K^M > [ 22.680919][ C1] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:^M > [ 22.682281][ C1] rcu: 0-....: (25 ticks this GP) idle=073/0/0x1 softirq=10/16 fqs=1050 ^M > [ 22.682851][ C1] (detected by 1, t=2102 jiffies, g=-1179, q=476)^M > [ 22.683649][ C1] Sending NMI from CPU 1 to CPUs 0:^M > [ 22.685252][ C0] NMI backtrace for cpu 0^M > [ 22.685649][ C0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.16.0-rc2-00185-g0faf20a1ad16 #145^M > [ 22.686393][ C0] NIP: c000000000016d64 LR: c000000000f6cca4 CTR: c00000000019c6e0^M > [ 22.686774][ C0] REGS: c000000002833590 TRAP: 0500 Not tainted (5.16.0-rc2-00185-g0faf20a1ad16)^M > [ 22.687222][ C0] MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 24000222 XER: 00000000^M > [ 22.688297][ C0] CFAR: c00000000000c854 IRQMASK: 0 ^M > ... > [ 22.692637][ C0] NIP [c000000000016d64] arch_local_irq_restore+0x174/0x250^M > [ 22.694443][ C0] LR [c000000000f6cca4] __do_softirq+0xe4/0x3dc^M > [ 22.695762][ C0] Call Trace:^M > [ 22.696050][ C0] [c000000002833830] [c000000000f6cc80] __do_softirq+0xc0/0x3dc (unreliable)^M > [ 22.697377][ C0] [c000000002833920] [c000000000151508] __irq_exit_rcu+0xd8/0x130^M > [ 22.698739][ C0] [c000000002833950] [c000000000151730] irq_exit+0x20/0x40^M > [ 22.699938][ C0] [c000000002833970] [c000000000027f40] timer_interrupt+0x270/0x460^M > [ 22.701119][ C0] [c0000000028339d0] [c0000000000099a8] decrementer_common_virt+0x208/0x210^M > > Possibly this should be fixed in the lowres timing code, but that would > be a generic change and could take some time and may not backport > easily, so for now make the programming of the decrementer unconditional > again in timer_interrupt() to avoid the stall/lockup. > > Fixes: 0faf20a1ad16 ("powerpc/64s/interrupt: Don't enable MSR[EE] in irq handlers unless perf is in use") > Reported-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> Reviewed-by: Nicholas Piggin <npiggin@gmail.com> > Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> > --- > arch/powerpc/kernel/time.c | 29 ++++++++++++++--------------- > 1 file changed, 14 insertions(+), 15 deletions(-) > > diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c > index f5cbfe5efd25..f80cce0e3899 100644 > --- a/arch/powerpc/kernel/time.c > +++ b/arch/powerpc/kernel/time.c > @@ -615,23 +615,22 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt) > return; > } > > - /* Conditionally hard-enable interrupts. */ > - if (should_hard_irq_enable()) { > - /* > - * Ensure a positive value is written to the decrementer, or > - * else some CPUs will continue to take decrementer exceptions. > - * When the PPC_WATCHDOG (decrementer based) is configured, > - * keep this at most 31 bits, which is about 4 seconds on most > - * systems, which gives the watchdog a chance of catching timer > - * interrupt hard lockups. > - */ > - if (IS_ENABLED(CONFIG_PPC_WATCHDOG)) > - set_dec(0x7fffffff); > - else > - set_dec(decrementer_max); > + /* > + * Ensure a positive value is written to the decrementer, or > + * else some CPUs will continue to take decrementer exceptions. > + * When the PPC_WATCHDOG (decrementer based) is configured, > + * keep this at most 31 bits, which is about 4 seconds on most > + * systems, which gives the watchdog a chance of catching timer > + * interrupt hard lockups. > + */ > + if (IS_ENABLED(CONFIG_PPC_WATCHDOG)) > + set_dec(0x7fffffff); > + else > + set_dec(decrementer_max); > > + /* Conditionally hard-enable interrupts. */ > + if (should_hard_irq_enable()) > do_hard_irq_enable(); > - } > > #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC) > if (atomic_read(&ppc_n_lost_interrupts) != 0) > -- > 2.34.1 > >
Excerpts from Michal Suchánek's message of April 21, 2022 12:28 am: > Hello, > > On Thu, Apr 21, 2022 at 12:16:57AM +1000, Michael Ellerman wrote: >> This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: >> Don't enable MSR[EE] in irq handlers unless perf is in use"). >> >> Prior to that commit, we always set the decrementer in >> timer_interrupt(), to clear the timer interrupt. Otherwise we could end >> up continuously taking timer interrupts. >> >> When high res timers are enabled there is no problem seen with leaving >> the decrementer untouched in timer_interrupt(), because it will be >> programmed via hrtimer_interrupt() -> tick_program_event() -> >> clockevents_program_event() -> decrementer_set_next_event(). >> >> However with CONFIG_HIGH_RES_TIMERS=n or booting with highres=off, we > > How difficult is it to detect this condition? > > Maybe detecting this could be just added? Possibly not too difficult but I'd like to see if we can get this to work in core timer code - https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-April/242212.html I'll resend as a patch and see what flamage I get... Thanks, Nick
Michal Suchánek <msuchanek@suse.de> writes: > Hello, > > On Thu, Apr 21, 2022 at 12:16:57AM +1000, Michael Ellerman wrote: >> This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: >> Don't enable MSR[EE] in irq handlers unless perf is in use"). >> >> Prior to that commit, we always set the decrementer in >> timer_interrupt(), to clear the timer interrupt. Otherwise we could end >> up continuously taking timer interrupts. >> >> When high res timers are enabled there is no problem seen with leaving >> the decrementer untouched in timer_interrupt(), because it will be >> programmed via hrtimer_interrupt() -> tick_program_event() -> >> clockevents_program_event() -> decrementer_set_next_event(). >> >> However with CONFIG_HIGH_RES_TIMERS=n or booting with highres=off, we > > How difficult is it to detect this condition? > > Maybe detecting this could be just added? We could but it would be quite a hack. This patch is just meant as a minimal fix to put the code back the way it's been for the last ~15 years. I think Nick has identified the problem in the core timing code, so fixing that should be the right solution. If that's wrong for some reason then we can do some detection in our timer_interrupt() to avoid the set_dec() when it's unnecessary. cheers
On Thu, 21 Apr 2022 00:16:57 +1000, Michael Ellerman wrote: > This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: > Don't enable MSR[EE] in irq handlers unless perf is in use"). > > Prior to that commit, we always set the decrementer in > timer_interrupt(), to clear the timer interrupt. Otherwise we could end > up continuously taking timer interrupts. > > [...] Applied to powerpc/fixes. [1/1] powerpc/time: Always set decrementer in timer_interrupt() https://git.kernel.org/powerpc/c/d2b9be1f4af5cabed1ee5bb341f887f64b1c1669 cheers
Excerpts from Nicholas Piggin's message of April 21, 2022 12:07 pm: > Excerpts from Michal Suchánek's message of April 21, 2022 12:28 am: >> Hello, >> >> On Thu, Apr 21, 2022 at 12:16:57AM +1000, Michael Ellerman wrote: >>> This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: >>> Don't enable MSR[EE] in irq handlers unless perf is in use"). >>> >>> Prior to that commit, we always set the decrementer in >>> timer_interrupt(), to clear the timer interrupt. Otherwise we could end >>> up continuously taking timer interrupts. >>> >>> When high res timers are enabled there is no problem seen with leaving >>> the decrementer untouched in timer_interrupt(), because it will be >>> programmed via hrtimer_interrupt() -> tick_program_event() -> >>> clockevents_program_event() -> decrementer_set_next_event(). >>> >>> However with CONFIG_HIGH_RES_TIMERS=n or booting with highres=off, we >> >> How difficult is it to detect this condition? >> >> Maybe detecting this could be just added? > > Possibly not too difficult but I'd like to see if we can get this to work > in core timer code - > > https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-April/242212.html > > I'll resend as a patch and see what flamage I get... tglx merged it into his tree, so we could try again after its upstream. https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=62c1256d544747b38e77ca9b5bfe3a26f9592576 I'm kind of worried the patch will explode some strange clock event device in an obscure way so we may wait for a release or two first. Thanks, Nick
Nicholas Piggin <npiggin@gmail.com> writes: > Excerpts from Nicholas Piggin's message of April 21, 2022 12:07 pm: >> Excerpts from Michal Suchánek's message of April 21, 2022 12:28 am: >>> Hello, >>> >>> On Thu, Apr 21, 2022 at 12:16:57AM +1000, Michael Ellerman wrote: >>>> This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: >>>> Don't enable MSR[EE] in irq handlers unless perf is in use"). >>>> >>>> Prior to that commit, we always set the decrementer in >>>> timer_interrupt(), to clear the timer interrupt. Otherwise we could end >>>> up continuously taking timer interrupts. >>>> >>>> When high res timers are enabled there is no problem seen with leaving >>>> the decrementer untouched in timer_interrupt(), because it will be >>>> programmed via hrtimer_interrupt() -> tick_program_event() -> >>>> clockevents_program_event() -> decrementer_set_next_event(). >>>> >>>> However with CONFIG_HIGH_RES_TIMERS=n or booting with highres=off, we >>> >>> How difficult is it to detect this condition? >>> >>> Maybe detecting this could be just added? >> >> Possibly not too difficult but I'd like to see if we can get this to work >> in core timer code - >> >> https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-April/242212.html >> >> I'll resend as a patch and see what flamage I get... > > tglx merged it into his tree, so we could try again after its > upstream. > > https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/commit/?h=timers/core&id=62c1256d544747b38e77ca9b5bfe3a26f9592576 > > I'm kind of worried the patch will explode some strange clock event > device in an obscure way so we may wait for a release or two first. Hah yep, :face-with-cold-sweat: I created an issue so hopefully we don't forget: https://github.com/linuxppc/issues/issues/408 cheers
diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c index f5cbfe5efd25..f80cce0e3899 100644 --- a/arch/powerpc/kernel/time.c +++ b/arch/powerpc/kernel/time.c @@ -615,23 +615,22 @@ DEFINE_INTERRUPT_HANDLER_ASYNC(timer_interrupt) return; } - /* Conditionally hard-enable interrupts. */ - if (should_hard_irq_enable()) { - /* - * Ensure a positive value is written to the decrementer, or - * else some CPUs will continue to take decrementer exceptions. - * When the PPC_WATCHDOG (decrementer based) is configured, - * keep this at most 31 bits, which is about 4 seconds on most - * systems, which gives the watchdog a chance of catching timer - * interrupt hard lockups. - */ - if (IS_ENABLED(CONFIG_PPC_WATCHDOG)) - set_dec(0x7fffffff); - else - set_dec(decrementer_max); + /* + * Ensure a positive value is written to the decrementer, or + * else some CPUs will continue to take decrementer exceptions. + * When the PPC_WATCHDOG (decrementer based) is configured, + * keep this at most 31 bits, which is about 4 seconds on most + * systems, which gives the watchdog a chance of catching timer + * interrupt hard lockups. + */ + if (IS_ENABLED(CONFIG_PPC_WATCHDOG)) + set_dec(0x7fffffff); + else + set_dec(decrementer_max); + /* Conditionally hard-enable interrupts. */ + if (should_hard_irq_enable()) do_hard_irq_enable(); - } #if defined(CONFIG_PPC32) && defined(CONFIG_PPC_PMAC) if (atomic_read(&ppc_n_lost_interrupts) != 0)
This is a partial revert of commit 0faf20a1ad16 ("powerpc/64s/interrupt: Don't enable MSR[EE] in irq handlers unless perf is in use"). Prior to that commit, we always set the decrementer in timer_interrupt(), to clear the timer interrupt. Otherwise we could end up continuously taking timer interrupts. When high res timers are enabled there is no problem seen with leaving the decrementer untouched in timer_interrupt(), because it will be programmed via hrtimer_interrupt() -> tick_program_event() -> clockevents_program_event() -> decrementer_set_next_event(). However with CONFIG_HIGH_RES_TIMERS=n or booting with highres=off, we see a stall/lockup, because tick_nohz_handler() does not cause a reprogram of the decrementer, leading to endless timer interrupts. Example trace: [ 1.898617][ T7] Freeing initrd memory: 2624K^M [ 22.680919][ C1] rcu: INFO: rcu_sched detected stalls on CPUs/tasks:^M [ 22.682281][ C1] rcu: 0-....: (25 ticks this GP) idle=073/0/0x1 softirq=10/16 fqs=1050 ^M [ 22.682851][ C1] (detected by 1, t=2102 jiffies, g=-1179, q=476)^M [ 22.683649][ C1] Sending NMI from CPU 1 to CPUs 0:^M [ 22.685252][ C0] NMI backtrace for cpu 0^M [ 22.685649][ C0] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.16.0-rc2-00185-g0faf20a1ad16 #145^M [ 22.686393][ C0] NIP: c000000000016d64 LR: c000000000f6cca4 CTR: c00000000019c6e0^M [ 22.686774][ C0] REGS: c000000002833590 TRAP: 0500 Not tainted (5.16.0-rc2-00185-g0faf20a1ad16)^M [ 22.687222][ C0] MSR: 8000000000009033 <SF,EE,ME,IR,DR,RI,LE> CR: 24000222 XER: 00000000^M [ 22.688297][ C0] CFAR: c00000000000c854 IRQMASK: 0 ^M ... [ 22.692637][ C0] NIP [c000000000016d64] arch_local_irq_restore+0x174/0x250^M [ 22.694443][ C0] LR [c000000000f6cca4] __do_softirq+0xe4/0x3dc^M [ 22.695762][ C0] Call Trace:^M [ 22.696050][ C0] [c000000002833830] [c000000000f6cc80] __do_softirq+0xc0/0x3dc (unreliable)^M [ 22.697377][ C0] [c000000002833920] [c000000000151508] __irq_exit_rcu+0xd8/0x130^M [ 22.698739][ C0] [c000000002833950] [c000000000151730] irq_exit+0x20/0x40^M [ 22.699938][ C0] [c000000002833970] [c000000000027f40] timer_interrupt+0x270/0x460^M [ 22.701119][ C0] [c0000000028339d0] [c0000000000099a8] decrementer_common_virt+0x208/0x210^M Possibly this should be fixed in the lowres timing code, but that would be a generic change and could take some time and may not backport easily, so for now make the programming of the decrementer unconditional again in timer_interrupt() to avoid the stall/lockup. Fixes: 0faf20a1ad16 ("powerpc/64s/interrupt: Don't enable MSR[EE] in irq handlers unless perf is in use") Reported-by: Miguel Ojeda <miguel.ojeda.sandonis@gmail.com> Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/kernel/time.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-)