| Message ID | 20220608142723.103523089@infradead.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | cpuidle,rcu: Cleanup the mess | expand |
On Wed, Jun 08, 2022 at 04:27:23PM +0200, Peter Zijlstra wrote: > Hi All! (omg so many) Hi Peter, Sorry for the delay; my plate has also been rather full recently. I'm beginning to page this in now. > These here few patches mostly clear out the utter mess that is cpuidle vs rcuidle. > > At the end of the ride there's only 2 real RCU_NONIDLE() users left > > arch/arm64/kernel/suspend.c: RCU_NONIDLE(__cpu_suspend_exit()); > drivers/perf/arm_pmu.c: RCU_NONIDLE(armpmu_start(event, PERF_EF_RELOAD)); The latter of these is necessary because apparently PM notifiers are called with RCU not watching. Is that still the case today (or at the end of this series)? If so, that feels like fertile land for more issues (yaey...). If not, we should be able to drop this. I can go dig into that some more. > kernel/cfi.c: RCU_NONIDLE({ > > (the CFI one is likely dead in the kCFI rewrite) and there's only a hand full > of trace_.*_rcuidle() left: > > kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, caller_addr); > kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, caller_addr); > kernel/trace/trace_preemptirq.c: trace_preempt_enable_rcuidle(a0, a1); > kernel/trace/trace_preemptirq.c: trace_preempt_disable_rcuidle(a0, a1); > > All of them are in 'deprecated' code that is unused for GENERIC_ENTRY. I think those are also unused on arm64 too? If not, I can go attack that. > I've touched a _lot_ of code that I can't test and likely broken some of it :/ > In particular, the whole ARM cpuidle stuff was quite involved with OMAP being > the absolute 'winner'. > > I'm hoping Mark can help me sort the remaining ARM64 bits as he moves that to > GENERIC_ENTRY. Moving to GENERIC_ENTRY as a whole is going to take a tonne of work (refactoring both arm64 and the generic portion to be more amenable to each other), but we can certainly move closer to that for the bits that matter here. Maybe we want a STRICT_ENTRY option to get rid of all the deprecated stuff that we can select regardless of GENERIC_ENTRY to make that easier. > I've also got a note that says ARM64 can probably do a WFE based > idle state and employ TIF_POLLING_NRFLAG to avoid some IPIs. Possibly; I'm not sure how much of a win that'll be given that by default we'll have a ~10KHz WFE wakeup from the timer, but we could take a peek. Thanks, Mark.
On Tue, Jun 14, 2022 at 12:19:29PM +0100, Mark Rutland wrote: > On Wed, Jun 08, 2022 at 04:27:23PM +0200, Peter Zijlstra wrote: > > Hi All! (omg so many) > > Hi Peter, > > Sorry for the delay; my plate has also been rather full recently. I'm beginning > to page this in now. No worries; we all have too much to do ;-) > > These here few patches mostly clear out the utter mess that is cpuidle vs rcuidle. > > > > At the end of the ride there's only 2 real RCU_NONIDLE() users left > > > > arch/arm64/kernel/suspend.c: RCU_NONIDLE(__cpu_suspend_exit()); > > drivers/perf/arm_pmu.c: RCU_NONIDLE(armpmu_start(event, PERF_EF_RELOAD)); > > The latter of these is necessary because apparently PM notifiers are called > with RCU not watching. Is that still the case today (or at the end of this > series)? If so, that feels like fertile land for more issues (yaey...). If not, > we should be able to drop this. That should be fixed; fingers crossed :-) > > kernel/cfi.c: RCU_NONIDLE({ > > > > (the CFI one is likely dead in the kCFI rewrite) and there's only a hand full > > of trace_.*_rcuidle() left: > > > > kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, caller_addr); > > kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, caller_addr); > > kernel/trace/trace_preemptirq.c: trace_preempt_enable_rcuidle(a0, a1); > > kernel/trace/trace_preemptirq.c: trace_preempt_disable_rcuidle(a0, a1); > > > > All of them are in 'deprecated' code that is unused for GENERIC_ENTRY. > > I think those are also unused on arm64 too? > > If not, I can go attack that. My grep spots: arch/arm64/kernel/entry-common.c: trace_hardirqs_on(); arch/arm64/include/asm/daifflags.h: trace_hardirqs_off(); arch/arm64/include/asm/daifflags.h: trace_hardirqs_off(); The _on thing should be replaced with something like: trace_hardirqs_on_prepare(); lockdep_hardirqs_on_prepare(); instrumentation_end(); rcu_irq_exit(); lockdep_hardirqs_on(CALLER_ADDR0); (as I think you know, since you have some of that already). And something similar for the _off thing, but with _off_finish(). > > I've touched a _lot_ of code that I can't test and likely broken some of it :/ > > In particular, the whole ARM cpuidle stuff was quite involved with OMAP being > > the absolute 'winner'. > > > > I'm hoping Mark can help me sort the remaining ARM64 bits as he moves that to > > GENERIC_ENTRY. > > Moving to GENERIC_ENTRY as a whole is going to take a tonne of work > (refactoring both arm64 and the generic portion to be more amenable to each > other), but we can certainly move closer to that for the bits that matter here. I know ... been there etc.. :-) > Maybe we want a STRICT_ENTRY option to get rid of all the deprecated stuff that > we can select regardless of GENERIC_ENTRY to make that easier. Possible yeah. > > I've also got a note that says ARM64 can probably do a WFE based > > idle state and employ TIF_POLLING_NRFLAG to avoid some IPIs. > > Possibly; I'm not sure how much of a win that'll be given that by default we'll > have a ~10KHz WFE wakeup from the timer, but we could take a peek. Ohh.. I didn't know it woke up *that* often. I just know Will made use of it in things like smp_cond_load_relaxed() which would be somewhat similar to a very shallow idle state that looks at the TIF word.
On Tue, Jun 14, 2022 at 06:58:30PM +0200, Peter Zijlstra wrote: > On Tue, Jun 14, 2022 at 12:19:29PM +0100, Mark Rutland wrote: > > On Wed, Jun 08, 2022 at 04:27:23PM +0200, Peter Zijlstra wrote: > > > Hi All! (omg so many) > > > > Hi Peter, > > > > Sorry for the delay; my plate has also been rather full recently. I'm beginning > > to page this in now. > > No worries; we all have too much to do ;-) > > > > These here few patches mostly clear out the utter mess that is cpuidle vs rcuidle. > > > > > > At the end of the ride there's only 2 real RCU_NONIDLE() users left > > > > > > arch/arm64/kernel/suspend.c: RCU_NONIDLE(__cpu_suspend_exit()); > > > drivers/perf/arm_pmu.c: RCU_NONIDLE(armpmu_start(event, PERF_EF_RELOAD)); > > > > The latter of these is necessary because apparently PM notifiers are called > > with RCU not watching. Is that still the case today (or at the end of this > > series)? If so, that feels like fertile land for more issues (yaey...). If not, > > we should be able to drop this. > > That should be fixed; fingers crossed :-) Cool; I'll try to give that a spin when I'm sat next to some relevant hardware. :) > > > kernel/cfi.c: RCU_NONIDLE({ > > > > > > (the CFI one is likely dead in the kCFI rewrite) and there's only a hand full > > > of trace_.*_rcuidle() left: > > > > > > kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > > kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); > > > kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, caller_addr); > > > kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, caller_addr); > > > kernel/trace/trace_preemptirq.c: trace_preempt_enable_rcuidle(a0, a1); > > > kernel/trace/trace_preemptirq.c: trace_preempt_disable_rcuidle(a0, a1); > > > > > > All of them are in 'deprecated' code that is unused for GENERIC_ENTRY. > > I think those are also unused on arm64 too? > > > > If not, I can go attack that. > > My grep spots: > > arch/arm64/kernel/entry-common.c: trace_hardirqs_on(); > arch/arm64/include/asm/daifflags.h: trace_hardirqs_off(); > arch/arm64/include/asm/daifflags.h: trace_hardirqs_off(); Ah; I hadn't realised those used trace_.*_rcuidle() behind the scenes. That affects local_irq_{enable,disable,restore}() too (which is what the daifflags.h bits are emulating), and also the generic entry code's irqentry_exit(). So it feels to me like we should be fixing those more generally? e.g. say that with a new STRICT_ENTRY[_RCU], we can only call trace_hardirqs_{on,off}() with RCU watching, and alter the definition of those? > The _on thing should be replaced with something like: > > trace_hardirqs_on_prepare(); > lockdep_hardirqs_on_prepare(); > instrumentation_end(); > rcu_irq_exit(); > lockdep_hardirqs_on(CALLER_ADDR0); > > (as I think you know, since you have some of that already). And > something similar for the _off thing, but with _off_finish(). Sure; I knew that was necessary for the outermost parts of entry (and I think that's all handled), I just hadn't realised that trace_hardirqs_{on,off} did the rcuidle thing in the middle. It'd be nice to not have to open-code the whole sequence everywhere for the portions which run after entry and are instrumentable, so (as above) I reckon we want to make trace_hardirqs_{on,off}() not do the rcuidle part unnecessarily (which IIUC is an end-goal anyway)? > > > I've touched a _lot_ of code that I can't test and likely broken some of it :/ > > > In particular, the whole ARM cpuidle stuff was quite involved with OMAP being > > > the absolute 'winner'. > > > > > > I'm hoping Mark can help me sort the remaining ARM64 bits as he moves that to > > > GENERIC_ENTRY. > > > > Moving to GENERIC_ENTRY as a whole is going to take a tonne of work > > (refactoring both arm64 and the generic portion to be more amenable to each > > other), but we can certainly move closer to that for the bits that matter here. > > I know ... been there etc.. :-) > > > Maybe we want a STRICT_ENTRY option to get rid of all the deprecated stuff that > > we can select regardless of GENERIC_ENTRY to make that easier. > > Possible yeah. > > > > I've also got a note that says ARM64 can probably do a WFE based > > > idle state and employ TIF_POLLING_NRFLAG to avoid some IPIs. > > > > Possibly; I'm not sure how much of a win that'll be given that by default we'll > > have a ~10KHz WFE wakeup from the timer, but we could take a peek. > > Ohh.. I didn't know it woke up *that* often. I just know Will made use > of it in things like smp_cond_load_relaxed() which would be somewhat > similar to a very shallow idle state that looks at the TIF word. We'll get some saving, I'm just not sure where that falls on the curve of idle states. FWIW the wakeup *can* be disabled (and it'd be nice to when we have WFxT instructions which take a timeout), it jsut happens to be on by default for reasons. Thanks, Mark.
Hi All! (omg so many) These here few patches mostly clear out the utter mess that is cpuidle vs rcuidle. At the end of the ride there's only 2 real RCU_NONIDLE() users left arch/arm64/kernel/suspend.c: RCU_NONIDLE(__cpu_suspend_exit()); drivers/perf/arm_pmu.c: RCU_NONIDLE(armpmu_start(event, PERF_EF_RELOAD)); kernel/cfi.c: RCU_NONIDLE({ (the CFI one is likely dead in the kCFI rewrite) and there's only a hand full of trace_.*_rcuidle() left: kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, CALLER_ADDR1); kernel/trace/trace_preemptirq.c: trace_irq_enable_rcuidle(CALLER_ADDR0, caller_addr); kernel/trace/trace_preemptirq.c: trace_irq_disable_rcuidle(CALLER_ADDR0, caller_addr); kernel/trace/trace_preemptirq.c: trace_preempt_enable_rcuidle(a0, a1); kernel/trace/trace_preemptirq.c: trace_preempt_disable_rcuidle(a0, a1); All of them are in 'deprecated' code that is unused for GENERIC_ENTRY. I've touched a _lot_ of code that I can't test and likely broken some of it :/ In particular, the whole ARM cpuidle stuff was quite involved with OMAP being the absolute 'winner'. I'm hoping Mark can help me sort the remaining ARM64 bits as he moves that to GENERIC_ENTRY. I've also got a note that says ARM64 can probably do a WFE based idle state and employ TIF_POLLING_NRFLAG to avoid some IPIs. --- arch/alpha/kernel/process.c | 1 arch/alpha/kernel/vmlinux.lds.S | 1 arch/arc/kernel/process.c | 3 + arch/arc/kernel/vmlinux.lds.S | 1 arch/arm/include/asm/vmlinux.lds.h | 1 arch/arm/kernel/process.c | 1 arch/arm/kernel/smp.c | 6 +-- arch/arm/mach-gemini/board-dt.c | 3 + arch/arm/mach-imx/cpuidle-imx6q.c | 4 +- arch/arm/mach-imx/cpuidle-imx6sx.c | 5 ++ arch/arm/mach-omap2/cpuidle34xx.c | 16 ++++++++ arch/arm/mach-omap2/cpuidle44xx.c | 29 +++++++++------ arch/arm/mach-omap2/pm.h | 2 - arch/arm/mach-omap2/pm34xx.c | 14 +++++-- arch/arm/mach-omap2/powerdomain.c | 10 ++--- arch/arm64/kernel/idle.c | 1 arch/arm64/kernel/smp.c | 4 +- arch/arm64/kernel/vmlinux.lds.S | 1 arch/csky/kernel/process.c | 1 arch/csky/kernel/smp.c | 2 - arch/csky/kernel/vmlinux.lds.S | 1 arch/hexagon/kernel/process.c | 1 arch/hexagon/kernel/vmlinux.lds.S | 1 arch/ia64/kernel/process.c | 1 arch/ia64/kernel/vmlinux.lds.S | 1 arch/loongarch/kernel/vmlinux.lds.S | 1 arch/m68k/kernel/vmlinux-nommu.lds | 1 arch/m68k/kernel/vmlinux-std.lds | 1 arch/m68k/kernel/vmlinux-sun3.lds | 1 arch/microblaze/kernel/process.c | 1 arch/microblaze/kernel/vmlinux.lds.S | 1 arch/mips/kernel/idle.c | 8 +--- arch/mips/kernel/vmlinux.lds.S | 1 arch/nios2/kernel/process.c | 1 arch/nios2/kernel/vmlinux.lds.S | 1 arch/openrisc/kernel/process.c | 1 arch/openrisc/kernel/vmlinux.lds.S | 1 arch/parisc/kernel/process.c | 2 - arch/parisc/kernel/vmlinux.lds.S | 1 arch/powerpc/kernel/idle.c | 5 +- arch/powerpc/kernel/vmlinux.lds.S | 1 arch/riscv/kernel/process.c | 1 arch/riscv/kernel/vmlinux-xip.lds.S | 1 arch/riscv/kernel/vmlinux.lds.S | 1 arch/s390/kernel/idle.c | 1 arch/s390/kernel/vmlinux.lds.S | 1 arch/sh/kernel/idle.c | 1 arch/sh/kernel/vmlinux.lds.S | 1 arch/sparc/kernel/leon_pmc.c | 4 ++ arch/sparc/kernel/process_32.c | 1 arch/sparc/kernel/process_64.c | 3 + arch/sparc/kernel/vmlinux.lds.S | 1 arch/um/kernel/dyn.lds.S | 1 arch/um/kernel/process.c | 1 arch/um/kernel/uml.lds.S | 1 arch/x86/coco/tdx/tdcall.S | 15 +------- arch/x86/coco/tdx/tdx.c | 25 +++---------- arch/x86/events/amd/brs.c | 13 ++----- arch/x86/include/asm/irqflags.h | 11 ++--- arch/x86/include/asm/mwait.h | 14 +++---- arch/x86/include/asm/nospec-branch.h | 2 - arch/x86/include/asm/paravirt.h | 6 ++- arch/x86/include/asm/perf_event.h | 2 - arch/x86/include/asm/shared/io.h | 4 +- arch/x86/include/asm/shared/tdx.h | 1 arch/x86/include/asm/special_insns.h | 6 +-- arch/x86/include/asm/xen/hypercall.h | 2 - arch/x86/kernel/paravirt.c | 14 ++++++- arch/x86/kernel/process.c | 65 +++++++++++++++++------------------ arch/x86/kernel/vmlinux.lds.S | 1 arch/x86/xen/enlighten_pv.c | 2 - arch/x86/xen/irq.c | 2 - arch/xtensa/kernel/process.c | 1 arch/xtensa/kernel/vmlinux.lds.S | 1 drivers/acpi/processor_idle.c | 46 ++++++++++++++---------- drivers/base/power/runtime.c | 24 ++++++------ drivers/clk/clk.c | 8 ++-- drivers/cpuidle/cpuidle-arm.c | 1 drivers/cpuidle/cpuidle-big_little.c | 8 +++- drivers/cpuidle/cpuidle-mvebu-v7.c | 7 +++ drivers/cpuidle/cpuidle-psci.c | 10 +++-- drivers/cpuidle/cpuidle-qcom-spm.c | 1 drivers/cpuidle/cpuidle-riscv-sbi.c | 10 +++-- drivers/cpuidle/cpuidle-tegra.c | 21 ++++++++--- drivers/cpuidle/cpuidle.c | 21 +++++------ drivers/cpuidle/dt_idle_states.c | 2 - drivers/cpuidle/poll_state.c | 10 ++++- drivers/idle/intel_idle.c | 29 ++++++++++++--- include/asm-generic/vmlinux.lds.h | 9 +--- include/linux/compiler_types.h | 8 +++- include/linux/cpu.h | 3 - include/linux/cpuidle.h | 33 +++++++++++++++++ include/linux/cpumask.h | 4 +- include/linux/sched/idle.h | 40 ++++++++++++++++----- include/linux/thread_info.h | 18 +++++++++ include/linux/tracepoint.h | 13 ++++++- kernel/cpu_pm.c | 9 ---- kernel/printk/printk.c | 2 - kernel/rcu/tree.c | 9 +--- kernel/sched/idle.c | 47 +++++++------------------ kernel/time/tick-broadcast-hrtimer.c | 29 ++++++--------- kernel/time/tick-broadcast.c | 6 ++- kernel/trace/trace.c | 3 + tools/objtool/check.c | 15 +++++++- 104 files changed, 449 insertions(+), 342 deletions(-)