Messages in this thread | | | Subject | Re: cpu_clock() in NMIs | From | Peter Zijlstra <> | Date | Mon, 14 Dec 2009 20:32:50 +0100 |
| |
On Mon, 2009-12-14 at 11:09 -0800, David Miller wrote: > From: Peter Zijlstra <peterz@infradead.org> > Date: Mon, 14 Dec 2009 10:02:39 +0100 > > > I'm not sure, traditionally sched_clock() was always called with IRQs > > disabled, and on eg. x86 that is needed because we read the TSC and then > > scale that value depending on CPUfreq state, which can be changed by a > > CPUfreq interrupt. Allowing NMIs in between isn't really a problem, > > allowing IRQs in is. > > Ok, that looks fine then. > > But, speaking more generally, any local_irq_{disable,save}() done in > an NMI handler has a high probability of being a bug. And it is a > bug if the IRQ disabling is there to prevent re-entry of the code > on the local cpu. > > > Now, the SPARC implementation might be good without IRQs disabled, but > > we should at least look at all other arches before we do what you > > propose below. As it removes the IRQ disable from the callsites whereas > > it previously always had that. > > Here's a quick audit: > > 1) Generic kernel/sched_clock.c implementation does a subtraction > of jiffies with a constant, then does some constant math on it. > Should be OK. > > 2) sparc64 is fine, just reads a register and multiplies with a > boot time calculated value, then shifts down by a constant. > Should be OK. > > 3) ARM mach-map, same situation as sparc64 > > 4) ARM mach-pxa, same situation as sparc64 > > 5) ARM mach-realview, multiplies counter a constant then divides by > one, should be OK. > > 6) ARM mach-sa1100, same as mach-realview > > 7) ARM mach-u300, uses boot time computed multiplier and shift, > should be OK. > > 8) ARM mach-versatile, same as mach-realview > > 9) ARM plat-IOP, same as mach-u300 > > 10) ARM plat-omap, same as mach-u300 > > 11) ARM plat-orion, same as sparc64 > > 12) Blackfin, TS version is same as sparc64 > > non-TS version is unnecessary duplication of generic weak > function version in kernel/sched_clock.c and could be deleted > > 13) CRIS, another dup of kernel/sched_clock.c weak function > > 14) FRV, another dup of kernel/sched_clock.c weak function > > 15) IA64, same as sparc64 for native version. > Paravirt version uses preemption disable, but also relies on > IA64 always setting HAVE_UNSTABLE_SCHED_CLOCK which it does, > and therefore IA64 would still disable interrupts with my change > > 16) m68knommu coldfire, multiplies by a constant then shifts down by > one, should be OK > > 17) mn10300, same as sparc64 > > 18) powerpc, non-__USE_RTC() case is same as sparc64 > > __USE_RTC() case also looks fine > > 19) s390, depends upon preemption being disabled so that > stop_machine does not interrupt the sched_clock() call > > should be OK > > 20) x86 sets HAVE_UNSTABLE_SCHED_CLOCK > > It should be safe.
OK, you convinced me plenty ;-)
I guess we need some debug code to ensure we don't grow any local_irq_save/restore/disable code in NMI paths, and maybe document this some place.
Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
| |