From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:19:49
These are a bunch of small things I've built up from looking
through code trying to track down some rare irq latency issues.
None of them actually fix any long irq latencies, but they
hopefully make the code a bit neater, get rid of some small
glitches, increase watchdog coverage etc.
Ben spotted a bug with the first patch last time I posted,
that's fixed.
Thanks,
Nick
Nicholas Piggin (11):
powerpc/64: irq_work avoid interrupt when called with hardware irqs
enabled
powerpc/pseries: put cede MSR[EE] check under IRQ_SOFT_MASK_DEBUG
powerpc/64s: make PACA_IRQ_HARD_DIS track MSR[EE] closely
powerpc/64s: micro-optimise __hard_irq_enable() for mtmsrd L=1 support
powerpc/64: remove start_tb and accum_tb from thread_struct
powerpc/pseries: lparcfg calculate PURR on demand
powerpc: generic clockevents broadcast receiver call
tick_receive_broadcast
powerpc: allow soft-NMI watchdog to cover timer interrupts with large
decrementers
powerpc: move timer broadcast code under GENERIC_CLOCKEVENTS_BROADCAST
ifdef
powerpc: move a stray NMI IPI case under NMI_IPI ifdef
powerpc/time: account broadcast timer event interrupts separately
arch/powerpc/include/asm/hardirq.h | 1 +
arch/powerpc/include/asm/hw_irq.h | 15 ++-
arch/powerpc/include/asm/plpar_wrappers.h | 8 +-
arch/powerpc/include/asm/processor.h | 4 -
arch/powerpc/include/asm/time.h | 9 --
arch/powerpc/kernel/entry_64.S | 8 ++
arch/powerpc/kernel/exceptions-64s.S | 5 +-
arch/powerpc/kernel/irq.c | 34 +++--
arch/powerpc/kernel/process.c | 18 ---
arch/powerpc/kernel/smp.c | 14 ++-
arch/powerpc/kernel/time.c | 143 +++++++++++++---------
arch/powerpc/platforms/pseries/lparcfg.c | 18 +--
12 files changed, 155 insertions(+), 122 deletions(-)
--
2.17.0
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:19:52
irq_work_raise should not cause a decrementer exception unless it is
called from NMI context. Doing so often just results in an immediate
masked decrementer interrupt:
<...>-550 90d... 4us : update_curr_rt <-dequeue_task_rt
<...>-550 90d... 5us : dbs_update_util_handler <-update_curr_rt
<...>-550 90d... 6us : arch_irq_work_raise <-irq_work_queue
<...>-550 90d... 7us : soft_nmi_interrupt <-soft_nmi_common
<...>-550 90d... 7us : printk_nmi_enter <-soft_nmi_interrupt
<...>-550 90d.Z. 8us : rcu_nmi_enter <-soft_nmi_interrupt
<...>-550 90d.Z. 9us : rcu_nmi_exit <-soft_nmi_interrupt
<...>-550 90d... 9us : printk_nmi_exit <-soft_nmi_interrupt
<...>-550 90d... 10us : cpuacct_charge <-update_curr_rt
The soft_nmi_interrupt here is the call into the watchdog, due to the
decrementer interrupt firing with irqs soft-disabled. This is
harmless, but sub-optimal.
When it's not called from NMI context or with interrupts enabled, mark
the decrementer pending in the irq_happened mask directly, rather than
having the masked decrementer interupt handler do it. This will be
replayed at the next local_irq_enable. See the comment for details.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/time.c | 33 +++++++++++++++++++++++++++++++--
1 file changed, 31 insertions(+), 2 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:19:55
This check does not catch IRQ soft mask bugs, but this option is
slightly more suitable than TRACE_IRQFLAGS.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/plpar_wrappers.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
@@ -39,10 +39,10 @@ static inline long extended_cede_processor(unsigned long latency_hint)set_cede_latency_hint(latency_hint);rc=cede_processor();-#ifdef CONFIG_TRACE_IRQFLAGS-/* Ensure that H_CEDE returns with IRQs on */-if(WARN_ON(!(mfmsr()&MSR_EE)))-__hard_irq_enable();+#ifdef CONFIG_PPC_IRQ_SOFT_MASK_DEBUG+/* Ensure that H_CEDE returns with IRQs on */+if(WARN_ON(!(mfmsr()&MSR_EE)))+__hard_irq_enable();#endifset_cede_latency_hint(old_latency_hint);
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:19:58
When the masked interrupt handler clears MSR[EE] for an interrupt in
the PACA_IRQ_MUST_HARD_MASK set, it does not set PACA_IRQ_HARD_DIS.
This makes them get out of synch.
With that taken into account, it's only low level irq manipulation
(and interrupt entry before reconcile) where they can be out of synch.
This makes the code less surprising.
It also allows the IRQ replay code to rely on the IRQ_HARD_DIS value
and not have to mtmsrd again in this case (e.g., for an external
interrupt that has been masked). The bigger benefit might just be
that there is not such an element of surprise in these two bits of
state.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hw_irq.h | 10 ++++++----
arch/powerpc/kernel/entry_64.S | 8 ++++++++
arch/powerpc/kernel/exceptions-64s.S | 5 ++++-
arch/powerpc/kernel/irq.c | 28 +++++++++++++++++++---------
4 files changed, 37 insertions(+), 14 deletions(-)
@@ -973,6 +973,14 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)orr4,r4,r3stdr4,_TRAP(r1)+/*+*PACA_IRQ_HARD_DISwon't always be set here, so set it now+*toreconciletheIRQstate.Tracingisalreadyaccountedfor.+*/+ldr4,PACAIRQHAPPENED(r13)+orir4,r4,PACA_IRQ_HARD_DIS+stbr4,PACAIRQHAPPENED(r13)+/**Thenfindtherighthandlerandcallit.Interruptsare*stillsoft-disabledandwekeepthemthatway.
@@ -145,8 +145,20 @@ notrace unsigned int __check_irq_replay(void)trace_hardirqs_on();trace_hardirqs_off();+/*+*Wearealwaysharddisabledhere,butPACA_IRQ_HARD_DISmay+*notbeset,whichmeansinterruptshaveonlyjustbeenhard+*disabledaspartofthelocal_irq_restoreorinterruptreturn+*code.Inthatcase,skipthedecrementrcheckbecausit's+*expensivetoreadtheTB.+*+*HARD_DISthengetsclearedhere,butit'sreconciledlater.+*Eitherlocal_irq_disablewillreplaytheinterruptandthat+*willreconcilestatelikeotherhardinterrupts.Orinterrupt+*returwillreplaytheinterruptandinthatcaseitsets+*PACA_IRQ_HARD_DISbyhand(seecommentsinentry_64.S).+*/if(happened&PACA_IRQ_HARD_DIS){-/* Clear bit 0 which we wouldn't clear otherwise */local_paca->irq_happened&=~PACA_IRQ_HARD_DIS;/*
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:20:01
Book3S minimum supported ISA version now requires mtmsrd L=1. This
instruction does not require bits other than RI and EE to be supplied,
so __hard_irq_enable() and __hard_irq_disable() does not have to read
the kernel_msr from paca.
Interrupt entry code already relies on L=1 support.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hw_irq.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:20:03
These fields are only written to.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/processor.h | 4 ----
arch/powerpc/kernel/process.c | 6 +-----
2 files changed, 1 insertion(+), 9 deletions(-)
@@ -264,10 +264,6 @@ struct thread_struct {structthread_fp_state*fp_save_area;intfpexc_mode;/* floating-point exception mode */unsignedintalign_ctl;/* alignment handling control */-#ifdef CONFIG_PPC64-unsignedlongstart_tb;/* Start purr when proc switched in */-unsignedlongaccum_tb;/* Total accumulated purr for process */-#endif#ifdef CONFIG_HAVE_HW_BREAKPOINTstructperf_event*ptrace_bps[HBP_NUM];/*
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:20:07
For SPLPAR, lparcfg provides a sum of PURR registers for all CPUs.
Currently this is done by reading PURR in context switch and timer
interrupt, and storing that into a per-CPU variable. These are summed
to provide the value.
This does not work with all timer schemes (e.g., NO_HZ_FULL), and it
is sub-optimal for performance because it reads the PURR register on
every context switch, although that's been difficult to distinguish
from noise in the contxt_switch microbenchmark.
This patch implements the sum by calling a function on each CPU, to
read and add PURR values of each CPU.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/time.h | 8 --------
arch/powerpc/kernel/process.c | 14 --------------
arch/powerpc/kernel/time.c | 8 --------
arch/powerpc/platforms/pseries/lparcfg.c | 18 ++++++++++--------
4 files changed, 10 insertions(+), 38 deletions(-)
@@ -196,14 +196,6 @@ extern u64 mulhdu(u64, u64);externvoiddiv128_by_32(u64dividend_high,u64dividend_low,unsigneddivisor,structdiv_result*dr);-/* Used to store Processor Utilization register (purr) values */--structcpu_usage{-u64current_tb;/* Holds the current purr register values */-};--DECLARE_PER_CPU(structcpu_usage,cpu_usage_array);-externvoidsecondary_cpu_time_init(void);externvoid__inittime_init(void);
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:20:10
The broadcast tick recipient can call tick_receive_broadcast rather
than re-running the full timer interrupt.
It does not have to check for the next event time, because the sender
already determined the timer has expired. It does not have to test
irq_work_pending, because that's a direct decrementer interrupt and
does not go through the clock events subsystem. And it does not have
to read PURR because that was removed with the previous patch.
This results in no code size change, but both the decrementer and
broadcast path lengths are reduced.
Cc: Srivatsa S. Bhat <redacted>
Cc: Preeti U Murthy <redacted>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hw_irq.h | 1 +
arch/powerpc/include/asm/time.h | 1 -
arch/powerpc/kernel/smp.c | 4 +-
arch/powerpc/kernel/time.c | 84 ++++++++++++++-----------------
4 files changed, 42 insertions(+), 48 deletions(-)
@@ -567,47 +567,16 @@ void arch_irq_work_raise(void)#endif /* CONFIG_IRQ_WORK */-staticvoid__timer_interrupt(void)-{-structpt_regs*regs=get_irq_regs();-u64*next_tb=this_cpu_ptr(&decrementers_next_tb);-structclock_event_device*evt=this_cpu_ptr(&decrementers);-u64now;--trace_timer_interrupt_entry(regs);--if(test_irq_work_pending()){-clear_irq_work_pending();-irq_work_run();-}--now=get_tb_or_rtc();-if(now>=*next_tb){-*next_tb=~(u64)0;-if(evt->event_handler)-evt->event_handler(evt);-__this_cpu_inc(irq_stat.timer_irqs_event);-}else{-now=*next_tb-now;-if(now<=decrementer_max)-set_dec(now);-/* We may have raced with new irq work */-if(test_irq_work_pending())-set_dec(1);-__this_cpu_inc(irq_stat.timer_irqs_others);-}--trace_timer_interrupt_exit(regs);-}-/**timer_interrupt-getscalledwhenthedecrementeroverflows,*withinterruptsdisabled.*/-voidtimer_interrupt(structpt_regs*regs)+voidtimer_interrupt(structpt_regs*regs){-structpt_regs*old_regs;+structclock_event_device*evt=this_cpu_ptr(&decrementers);u64*next_tb=this_cpu_ptr(&decrementers_next_tb);+structpt_regs*old_regs;+u64now;/* Ensure a positive value is written to the decrementer, or else*someCPUswillcontinuetotakedecrementerexceptions.
@@ -638,13 +607,47 @@ void timer_interrupt(struct pt_regs * regs)old_regs=set_irq_regs(regs);irq_enter();+trace_timer_interrupt_entry(regs);++if(test_irq_work_pending()){+clear_irq_work_pending();+irq_work_run();+}++now=get_tb_or_rtc();+if(now>=*next_tb){+*next_tb=~(u64)0;+if(evt->event_handler)+evt->event_handler(evt);+__this_cpu_inc(irq_stat.timer_irqs_event);+}else{+now=*next_tb-now;+if(now<=decrementer_max)+set_dec(now);+/* We may have raced with new irq work */+if(test_irq_work_pending())+set_dec(1);+__this_cpu_inc(irq_stat.timer_irqs_others);+}-__timer_interrupt();+trace_timer_interrupt_exit(regs);irq_exit();set_irq_regs(old_regs);}EXPORT_SYMBOL(timer_interrupt);+voidtimer_broadcast_interrupt(void)+{+u64*next_tb=this_cpu_ptr(&decrementers_next_tb);+structpt_regs*regs=get_irq_regs();++trace_timer_interrupt_entry(regs);+*next_tb=~(u64)0;+tick_receive_broadcast();+__this_cpu_inc(irq_stat.timer_irqs_event);+trace_timer_interrupt_exit(regs);+}+/**Hypervisordecrementerinterruptsshouldn'toccurbutaresometimes*leftpendingonexitfromaKVMguest.Wedon'tneedtodoanything
@@ -992,15 +995,6 @@ static int decrementer_shutdown(struct clock_event_device *dev)return0;}-/* Interrupt handler for the timer broadcast IPI */-voidtick_broadcast_ipi_handler(void)-{-u64*next_tb=this_cpu_ptr(&decrementers_next_tb);--*next_tb=get_tb_or_rtc();-__timer_interrupt();-}-staticvoidregister_decrementer_clockevent(intcpu){structclock_event_device*dec=&per_cpu(decrementers,cpu);
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:20:13
Large decrementers (e.g., POWER9) can take a very long time to wrap,
so when the timer iterrupt handler sets the decrementer to max so as
to avoid taking another decrementer interrupt when hard enabling
interrupts before running timers, it effectively disables the soft
NMI coverage for timer interrupts.
Fix this by using the traditional 31-bit value instead, which wraps
after a few seconds. masked interrupt code does the same thing, and
in normal operation neither of these paths would ever wrap even the
31 bit value.
Note: the SMP watchdog should catch timer interrupt lockups, but it
is preferable for the local soft-NMI to catch them, mainly to avoid
the IPI.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/kernel/time.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
@@ -578,22 +578,29 @@ void timer_interrupt(struct pt_regs *regs)structpt_regs*old_regs;u64now;-/* Ensure a positive value is written to the decrementer, or else-*someCPUswillcontinuetotakedecrementerexceptions.-*/-set_dec(decrementer_max);-/* Some implementations of hotplug will get timer interrupts while*offline,justignoretheseandwealsoneedtoset*decrementers_next_tbasMAXtomakesure__check_irq_replay*don'treplaytimerinterruptwhenreturn,otherwisewe'lltrap*hereinfinitely:(*/-if(!cpu_online(smp_processor_id())){+if(unlikely(!cpu_online(smp_processor_id()))){*next_tb=~(u64)0;+set_dec(decrementer_max);return;}+/* Ensure a positive value is written to the decrementer, or else+*someCPUswillcontinuetotakedecrementerexceptions.Whenthe+*PPC_WATCHDOG(decrementerbased)isconfigured,keepthisatmost+*31bits,whichisabout4secondsonmostsystems,whichgives+*thewatchdogachanceofcatchingtimerinterrupthardlockups.+*/+if(IS_ENABLED(CONFIG_PPC_WATCHDOG))+set_dec(0x7fffffff);+else+set_dec(decrementer_max);+/* Conditionally hard-enable interrupts now that the DEC has been*bumpedtoitsmaximumvalue*/
From: Nicholas Piggin <npiggin@gmail.com> Date: 2018-05-04 17:20:22
These are not local timer interrupts but IPIs. It's good to be able
to see how timer offloading is behaving, so split these out into
their own category.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---
arch/powerpc/include/asm/hardirq.h | 1 +
arch/powerpc/kernel/irq.c | 6 ++++++
arch/powerpc/kernel/time.c | 5 +----
3 files changed, 8 insertions(+), 4 deletions(-)
arch/powerpc/kernel/time.c:646:2: error: implicit declaration of function 'tick_receive_broadcast'; did you mean 'tick_setup_hrtimer_broadcast'? [-Werror=implicit-function-declaration]
tick_receive_broadcast();
^~~~~~~~~~~~~~~~~~~~~~
tick_setup_hrtimer_broadcast
cc1: all warnings being treated as errors
vim +646 arch/powerpc/kernel/time.c
638
639 void timer_broadcast_interrupt(void)
640 {
641 u64 *next_tb = this_cpu_ptr(&decrementers_next_tb);
642 struct pt_regs *regs = get_irq_regs();
643
644 trace_timer_interrupt_entry(regs);
645 *next_tb = ~(u64)0;
> 646 tick_receive_broadcast();
647 __this_cpu_inc(irq_stat.timer_irqs_event);
648 trace_timer_interrupt_exit(regs);
649 }
650
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
From: Michael Ellerman <hidden> Date: 2018-06-04 14:10:45
On Fri, 2018-05-04 at 17:19:25 UTC, Nicholas Piggin wrote:
irq_work_raise should not cause a decrementer exception unless it is
called from NMI context. Doing so often just results in an immediate
masked decrementer interrupt:
<...>-550 90d... 4us : update_curr_rt <-dequeue_task_rt
<...>-550 90d... 5us : dbs_update_util_handler <-update_curr_rt
<...>-550 90d... 6us : arch_irq_work_raise <-irq_work_queue
<...>-550 90d... 7us : soft_nmi_interrupt <-soft_nmi_common
<...>-550 90d... 7us : printk_nmi_enter <-soft_nmi_interrupt
<...>-550 90d.Z. 8us : rcu_nmi_enter <-soft_nmi_interrupt
<...>-550 90d.Z. 9us : rcu_nmi_exit <-soft_nmi_interrupt
<...>-550 90d... 9us : printk_nmi_exit <-soft_nmi_interrupt
<...>-550 90d... 10us : cpuacct_charge <-update_curr_rt
The soft_nmi_interrupt here is the call into the watchdog, due to the
decrementer interrupt firing with irqs soft-disabled. This is
harmless, but sub-optimal.
When it's not called from NMI context or with interrupts enabled, mark
the decrementer pending in the irq_happened mask directly, rather than
having the masked decrementer interupt handler do it. This will be
replayed at the next local_irq_enable. See the comment for details.
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>