More RCU vs idle fixes

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Reported-by Mark Rutland <[email protected]>

Reported-by Sven Schnelle <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

kernel/sched/idle.c +20 -1

@@ -78,7 +78,7 @@ void __weak arch_cpu_idle_dead(void) { }
 void __weak arch_cpu_idle(void)
 {
 	cpu_idle_force_poll = 1;
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /**

@@ -94,9 +94,35 @@ void __cpuidle default_idle_call(void)
 
 		trace_cpu_idle(1, smp_processor_id());
 		stop_critical_timings();
+
+		/*
+		 * arch_cpu_idle() is supposed to enable IRQs, however
+		 * we can't do that because of RCU and tracing.
+		 *
+		 * Trace IRQs enable here, then switch off RCU, and have
+		 * arch_cpu_idle() use raw_local_irq_enable(). Note that
+		 * rcu_idle_enter() relies on lockdep IRQ state, so switch that
+		 * last -- this is very similar to the entry code.
+		 */
+		trace_hardirqs_on_prepare();
+		lockdep_hardirqs_on_prepare(_THIS_IP_);
 		rcu_idle_enter();
+		lockdep_hardirqs_on(_THIS_IP_);
+
 		arch_cpu_idle();
+
+		/*
+		 * OK, so IRQs are enabled here, but RCU needs them disabled to
+		 * turn itself back on.. funny thing is that disabling IRQs
+		 * will cause tracing, which needs RCU. Jump through hoops to

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Reported-by Mark Rutland <[email protected]>

Reported-by Sven Schnelle <[email protected]>

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 arch/alpha/kernel/process.c      |    2 +-
 arch/arm/kernel/process.c        |    2 +-
 arch/arm64/kernel/process.c      |    2 +-
 arch/csky/kernel/process.c       |    2 +-
 arch/h8300/kernel/process.c      |    2 +-
 arch/hexagon/kernel/process.c    |    2 +-
 arch/ia64/kernel/process.c       |    2 +-
 arch/microblaze/kernel/process.c |    2 +-
 arch/mips/kernel/idle.c          |   12 ++++++------
 arch/nios2/kernel/process.c      |    2 +-
 arch/openrisc/kernel/process.c   |    2 +-
 arch/parisc/kernel/process.c     |    2 +-
 arch/powerpc/kernel/idle.c       |    4 ++--
 arch/riscv/kernel/process.c      |    2 +-
 arch/s390/kernel/idle.c          |    2 +-
 arch/sh/kernel/idle.c            |    2 +-
 arch/sparc/kernel/leon_pmc.c     |    4 ++--
 arch/sparc/kernel/process_32.c   |    2 +-
 arch/sparc/kernel/process_64.c   |    4 ++--
 arch/um/kernel/process.c         |    2 +-
 arch/x86/include/asm/mwait.h     |    2 --
 arch/x86/kernel/process.c        |   12 +++++++-----
 kernel/sched/idle.c              |   28 +++++++++++++++++++++++++++-
 23 files changed, 62 insertions(+), 36 deletions(-)

arch/alpha/kernel/process.c +1 -1

@@ -57,7 +57,7 @@ EXPORT_SYMBOL(pm_power_off);
 void arch_cpu_idle(void)
 {
 	wtint(0);
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 void arch_cpu_idle_dead(void)

arch/arm/kernel/process.c +1 -1

@@ -71,7 +71,7 @@ void arch_cpu_idle(void)
 		arm_pm_idle();
 	else
 		cpu_do_idle();
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 void arch_cpu_idle_prepare(void)

arch/arm64/kernel/process.c +1 -1

@@ -126,7 +126,7 @@ void arch_cpu_idle(void)
 	 * tricks
 	 */
 	cpu_do_idle();
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 #ifdef CONFIG_HOTPLUG_CPU

arch/csky/kernel/process.c +1 -1

@@ -102,6 +102,6 @@ void arch_cpu_idle(void)
 #ifdef CONFIG_CPU_PM_STOP
 	asm volatile("stop\n");
 #endif
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 #endif

arch/h8300/kernel/process.c +1 -1

@@ -57,7 +57,7 @@ asmlinkage void ret_from_kernel_thread(v
  */
 void arch_cpu_idle(void)
 {
-	local_irq_enable();
+	raw_local_irq_enable();
 	__asm__("sleep");
 }
 

arch/hexagon/kernel/process.c +1 -1

@@ -44,7 +44,7 @@ void arch_cpu_idle(void)
 {
 	__vmwait();
 	/*  interrupts wake us up, but irqs are still disabled */
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /*

arch/ia64/kernel/process.c +1 -1

@@ -239,7 +239,7 @@ void arch_cpu_idle(void)
 	if (mark_idle)
 		(*mark_idle)(1);
 
-	safe_halt();
+	raw_safe_halt();
 
 	if (mark_idle)
 		(*mark_idle)(0);

arch/microblaze/kernel/process.c +1 -1

@@ -149,5 +149,5 @@ int dump_fpu(struct pt_regs *regs, elf_f
 
 void arch_cpu_idle(void)
 {
-       local_irq_enable();
+       raw_local_irq_enable();
 }

arch/mips/kernel/idle.c +6 -6

@@ -33,19 +33,19 @@ static void __cpuidle r3081_wait(void)
 {
 	unsigned long cfg = read_c0_conf();
 	write_c0_conf(cfg | R30XX_CONF_HALT);
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 static void __cpuidle r39xx_wait(void)
 {
 	if (!need_resched())
 		write_c0_conf(read_c0_conf() | TX39_CONF_HALT);
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 void __cpuidle r4k_wait(void)
 {
-	local_irq_enable();
+	raw_local_irq_enable();
 	__r4k_wait();
 }
 

@@ -64,7 +64,7 @@ void __cpuidle r4k_wait_irqoff(void)
 		"	.set	arch=r4000	\n"
 		"	wait			\n"
 		"	.set	pop		\n");
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /*

@@ -84,7 +84,7 @@ static void __cpuidle rm7k_wait_irqoff(v
 		"	wait						\n"
 		"	mtc0	$1, $12		# stalls until W stage	\n"
 		"	.set	pop					\n");
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /*

@@ -257,7 +257,7 @@ void arch_cpu_idle(void)
 	if (cpu_wait)
 		cpu_wait();
 	else
-		local_irq_enable();
+		raw_local_irq_enable();
 }
 
 #ifdef CONFIG_CPU_IDLE

arch/nios2/kernel/process.c +1 -1

@@ -33,7 +33,7 @@ EXPORT_SYMBOL(pm_power_off);
 
 void arch_cpu_idle(void)
 {
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /*

arch/openrisc/kernel/process.c +1 -1

@@ -79,7 +79,7 @@ void machine_power_off(void)
  */
 void arch_cpu_idle(void)
 {
-	local_irq_enable();
+	raw_local_irq_enable();
 	if (mfspr(SPR_UPR) & SPR_UPR_PMP)
 		mtspr(SPR_PMR, mfspr(SPR_PMR) | SPR_PMR_DME);
 }

arch/parisc/kernel/process.c +1 -1

@@ -169,7 +169,7 @@ void __cpuidle arch_cpu_idle_dead(void)
 
 void __cpuidle arch_cpu_idle(void)
 {
-	local_irq_enable();
+	raw_local_irq_enable();
 
 	/* nop on real hardware, qemu will idle sleep. */
 	asm volatile("or %%r10,%%r10,%%r10\n":::);

arch/powerpc/kernel/idle.c +2 -2

@@ -52,9 +52,9 @@ void arch_cpu_idle(void)
 		 * interrupts enabled, some don't.
 		 */
 		if (irqs_disabled())
-			local_irq_enable();
+			raw_local_irq_enable();
 	} else {
-		local_irq_enable();
+		raw_local_irq_enable();
 		/*
 		 * Go into low thread priority and possibly
 		 * low power mode.

arch/riscv/kernel/process.c +1 -1

@@ -36,7 +36,7 @@ extern asmlinkage void ret_from_kernel_t
 void arch_cpu_idle(void)
 {
 	wait_for_interrupt();
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 void show_regs(struct pt_regs *regs)

arch/s390/kernel/idle.c +1 -1

@@ -123,7 +123,7 @@ void arch_cpu_idle_enter(void)
 void arch_cpu_idle(void)
 {
 	enabled_wait();
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 void arch_cpu_idle_exit(void)

arch/sh/kernel/idle.c +1 -1

@@ -22,7 +22,7 @@ static void (*sh_idle)(void);
 void default_idle(void)
 {
 	set_bl_bit();
-	local_irq_enable();
+	raw_local_irq_enable();
 	/* Isn't this racy ? */
 	cpu_sleep();
 	clear_bl_bit();

arch/sparc/kernel/leon_pmc.c +2 -2

@@ -50,7 +50,7 @@ static void pmc_leon_idle_fixup(void)
 	register unsigned int address = (unsigned int)leon3_irqctrl_regs;
 
 	/* Interrupts need to be enabled to not hang the CPU */
-	local_irq_enable();
+	raw_local_irq_enable();
 
 	__asm__ __volatile__ (
 		"wr	%%g0, %%asr19\n"

@@ -66,7 +66,7 @@ static void pmc_leon_idle_fixup(void)
 static void pmc_leon_idle(void)
 {
 	/* Interrupts need to be enabled to not hang the CPU */
-	local_irq_enable();
+	raw_local_irq_enable();
 
 	/* For systems without power-down, this will be no-op */
 	__asm__ __volatile__ ("wr	%g0, %asr19\n\t");

arch/sparc/kernel/process_32.c +1 -1

@@ -74,7 +74,7 @@ void arch_cpu_idle(void)
 {
 	if (sparc_idle)
 		(*sparc_idle)();
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /* XXX cli/sti -> local_irq_xxx here, check this works once SMP is fixed. */

arch/sparc/kernel/process_64.c +2 -2

@@ -62,11 +62,11 @@ void arch_cpu_idle(void)
 {
 	if (tlb_type != hypervisor) {
 		touch_nmi_watchdog();
-		local_irq_enable();
+		raw_local_irq_enable();
 	} else {
 		unsigned long pstate;
 
-		local_irq_enable();
+		raw_local_irq_enable();
 
                 /* The sun4v sleeping code requires that we have PSTATE.IE cleared over
                  * the cpu sleep hypervisor call.

arch/um/kernel/process.c +1 -1

@@ -217,7 +217,7 @@ void arch_cpu_idle(void)
 {
 	cpu_tasks[current_thread_info()->cpu].pid = os_getpid();
 	um_idle_sleep();
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 int __cant_sleep(void) {

arch/x86/include/asm/mwait.h +0 -2

@@ -88,8 +88,6 @@ static inline void __mwaitx(unsigned lon
 
 static inline void __sti_mwait(unsigned long eax, unsigned long ecx)
 {
-	trace_hardirqs_on();
-
 	mds_idle_clear_cpu_buffers();
 	/* "mwait %eax, %ecx;" */
 	asm volatile("sti; .byte 0x0f, 0x01, 0xc9;"

arch/x86/kernel/process.c +7 -5

@@ -685,7 +685,7 @@ void arch_cpu_idle(void)
  */
 void __cpuidle default_idle(void)
 {
-	safe_halt();
+	raw_safe_halt();
 }
 #if defined(CONFIG_APM_MODULE) || defined(CONFIG_HALTPOLL_CPUIDLE_MODULE)
 EXPORT_SYMBOL(default_idle);

@@ -736,6 +736,8 @@ void stop_this_cpu(void *dummy)
 /*
  * AMD Erratum 400 aware idle routine. We handle it the same way as C3 power
  * states (local apic timer and TSC stop).
+ *
+ * XXX this function is completely buggered vs RCU and tracing.
  */
 static void amd_e400_idle(void)
 {

@@ -757,9 +759,9 @@ static void amd_e400_idle(void)
 	 * The switch back from broadcast mode needs to be called with
 	 * interrupts disabled.
 	 */
-	local_irq_disable();
+	raw_local_irq_disable();
 	tick_broadcast_exit();
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /*

@@ -801,9 +803,9 @@ static __cpuidle void mwait_idle(void)
 		if (!need_resched())
 			__sti_mwait(0, 0);
 		else
-			local_irq_enable();
+			raw_local_irq_enable();
 	} else {
-		local_irq_enable();
+		raw_local_irq_enable();
 	}
 	__current_clr_polling();
 }

kernel/sched/idle.c +27 -1

@@ -78,7 +78,7 @@ void __weak arch_cpu_idle_dead(void) { }
 void __weak arch_cpu_idle(void)
 {
 	cpu_idle_force_poll = 1;
-	local_irq_enable();
+	raw_local_irq_enable();
 }
 
 /**

@@ -94,9 +94,35 @@ void __cpuidle default_idle_call(void)
 
 		trace_cpu_idle(1, smp_processor_id());
 		stop_critical_timings();
+
+		/*
+		 * arch_cpu_idle() is supposed to enable IRQs, however
+		 * we can't do that because of RCU and tracing.
+		 *
+		 * Trace IRQs enable here, then switch off RCU, and have
+		 * arch_cpu_idle() use raw_local_irq_enable(). Note that
+		 * rcu_idle_enter() relies on lockdep IRQ state, so switch that
+		 * last -- this is very similar to the entry code.
+		 */
+		trace_hardirqs_on_prepare();
+		lockdep_hardirqs_on_prepare(_THIS_IP_);
 		rcu_idle_enter();
+		lockdep_hardirqs_on(_THIS_IP_);
+
 		arch_cpu_idle();
+
+		/*
+		 * OK, so IRQs are enabled here, but RCU needs them disabled to
+		 * turn itself back on.. funny thing is that disabling IRQs
+		 * will cause tracing, which needs RCU. Jump through hoops to
+		 * make it 'work'.
+		 */
+		raw_local_irq_disable();
+		lockdep_hardirqs_off(_THIS_IP_);
 		rcu_idle_exit();
+		lockdep_hardirqs_on(_THIS_IP_);
+		raw_local_irq_enable();
+
 		start_critical_timings();
 		trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id());
 	}

Quoted 7 lines · Peter Zijlstra

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

Quoted 3 lines

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Has this patch been tested on s390 ? Reason for asking is that it causes all my s390 emulations to crash. Reverting it fixes the problem.

Quoted 12 lines · Guenter Roeck

Quoted 10 lines · Peter Zijlstra

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Quoted 2 lines

Has this patch been tested on s390 ? Reason for asking is that it causes all my s390 emulations to crash. Reverting it fixes the problem.

My understanding is that it changes the error on s390. Previously it would complain about the local_irq_enable() in arch_cpu_idle(), now it complains when taking an interrupt during idle.

Quoted 12 lines · Guenter Roeck

Quoted 10 lines · Peter Zijlstra

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Quoted 2 lines

Has this patch been tested on s390 ? Reason for asking is that it causes all my s390 emulations to crash. Reverting it fixes the problem.

My understanding is that it changes the error on s390. Previously it would complain about the local_irq_enable() in arch_cpu_idle(), now it complains when taking an interrupt during idle.

Hi Peter,

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Guenter Roeck

Quoted 10 lines · Peter Zijlstra

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Quoted 6 lines

Quoted 2 lines

Has this patch been tested on s390 ? Reason for asking is that it causes all my s390 emulations to crash. Reverting it fixes the problem.

My understanding is that it changes the error on s390. Previously it would complain about the local_irq_enable() in arch_cpu_idle(), now it complains when taking an interrupt during idle.

I looked into adding the required functionality for s390, but the code we would need to add to entry.S is rather large - as you noted we would have to duplicate large portions of irqentry_enter() into our code. Given that s390 was fine before that patch, can you revert it and submit it again during the next merge window?

Quoted 24 lines · Sven Schnelle

Hi Peter,

Quoted 19 lines · Peter Zijlstra

Quoted 8 lines · Guenter Roeck

Quoted 7 lines · Peter Zijlstra

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

Quoted 6 lines

Quoted 3 lines

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Has this patch been tested on s390 ? Reason for asking is that it causes all my s390 emulations to crash. Reverting it fixes the problem.

My understanding is that it changes the error on s390. Previously it would complain about the local_irq_enable() in arch_cpu_idle(), now it complains when taking an interrupt during idle.

Quoted 5 lines

I looked into adding the required functionality for s390, but the code we would need to add to entry.S is rather large - as you noted we would have to duplicate large portions of irqentry_enter() into our code. Given that s390 was fine before that patch, can you revert it and submit it again during the next merge window?

I'm not sure I understand how s390 was fine without it, let me consdier. Also, what's the status of ARM64, they do need this too.

Quoted 21 lines · Sven Schnelle

Quoted 19 lines · Peter Zijlstra

Quoted 8 lines · Guenter Roeck

Quoted 7 lines · Peter Zijlstra

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

Quoted 6 lines

Quoted 3 lines

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Has this patch been tested on s390 ? Reason for asking is that it causes all my s390 emulations to crash. Reverting it fixes the problem.

My understanding is that it changes the error on s390. Previously it would complain about the local_irq_enable() in arch_cpu_idle(), now it complains when taking an interrupt during idle.

Quoted 5 lines

I looked into adding the required functionality for s390, but the code we would need to add to entry.S is rather large - as you noted we would have to duplicate large portions of irqentry_enter() into our code. Given that s390 was fine before that patch, can you revert it and submit it again during the next merge window?

I'm not sure I understand how s390 was fine without it, let me consdier. Also, what's the status of ARM64, they do need this too.

Hi Peter,

Quoted 13 lines · Peter Zijlstra

Quoted 12 lines · Guenter Roeck

Quoted 10 lines · Peter Zijlstra

We call arch_cpu_idle() with RCU disabled, but then use local_irq_{en,dis}able(), which invokes tracing, which relies on RCU.

Switch all arch_cpu_idle() implementations to use raw_local_irq_{en,dis}able() and carefully manage the lockdep,rcu,tracing state like we do in entry.

(XXX: we really should change arch_cpu_idle() to not return with interrupts enabled)

Quoted 6 lines

Quoted 2 lines

Has this patch been tested on s390 ? Reason for asking is that it causes all my s390 emulations to crash. Reverting it fixes the problem.

My understanding is that it changes the error on s390. Previously it would complain about the local_irq_enable() in arch_cpu_idle(), now it complains when taking an interrupt during idle.

I looked into adding the required functionality for s390, but the code we would need to add to entry.S is rather large - as you noted we would have to duplicate large portions of irqentry_enter() into our code. Given that s390 was fine before that patch, can you revert it and submit it again during the next merge window?

cpuidle->enter() callbacks should not call into tracing because RCU has already been disabled. Instead of doing the broadcast thing itself, simply advertise to the cpuidle core that those states stop the timer.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/idle/intel_idle.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

drivers/idle/intel_idle.c +17 -17

@@ -126,26 +126,9 @@ static __cpuidle int intel_idle(struct c
        struct cpuidle_state *state = &drv->states[index];
        unsigned long eax = flg2MWAIT(state->flags);
        unsigned long ecx = 1; /* break on interrupt flag */
-       bool tick;
-
-       if (!static_cpu_has(X86_FEATURE_ARAT)) {
-               /*
-                * Switch over to one-shot tick broadcast if the target C-state
-                * is deeper than C1.
-                */
-               if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) {
-                       tick = true;
-                       tick_broadcast_enter();
-               } else {
-                       tick = false;
-               }
-       }
 
        mwait_idle_with_hints(eax, ecx);
 
-       if (!static_cpu_has(X86_FEATURE_ARAT) && tick)
-               tick_broadcast_exit();
-
        return index;
 }
 

@@ -1460,6 +1443,23 @@ static bool __init intel_idle_verify_cst
        return true;
 }
 
+static bool __init intel_idle_state_needs_timer_stop(struct cpuidle_state *state)
+{
+       unsigned long eax = flg2MWAIT(state->flags);
+
+       if (boot_cpu_has(X86_FEATURE_ARAT))
+               return false;
+
+       /*
+        * Switch over to one-shot tick broadcast if the target C-state
+        * is deeper than C1.
+        */
+       if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK)
+               return true;
+
+       return false;
+}
+
 static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
 {
        int cstate;

Quoted 68 lines · Peter Zijlstra

cpuidle->enter() callbacks should not call into tracing because RCU has already been disabled. Instead of doing the broadcast thing itself, simply advertise to the cpuidle core that those states stop the timer.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/idle/intel_idle.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

drivers/idle/intel_idle.c +17 -17

@@ -126,26 +126,9 @@ static __cpuidle int intel_idle(struct c
        struct cpuidle_state *state = &drv->states[index];
        unsigned long eax = flg2MWAIT(state->flags);
        unsigned long ecx = 1; /* break on interrupt flag */
-       bool tick;
-
-       if (!static_cpu_has(X86_FEATURE_ARAT)) {
-               /*
-                * Switch over to one-shot tick broadcast if the target C-state
-                * is deeper than C1.
-                */
-               if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) {
-                       tick = true;
-                       tick_broadcast_enter();
-               } else {
-                       tick = false;
-               }
-       }
 
        mwait_idle_with_hints(eax, ecx);
 
-       if (!static_cpu_has(X86_FEATURE_ARAT) && tick)
-               tick_broadcast_exit();
-
        return index;
 }
 

@@ -1460,6 +1443,23 @@ static bool __init intel_idle_verify_cst
        return true;
 }
 
+static bool __init intel_idle_state_needs_timer_stop(struct cpuidle_state *state)
+{
+       unsigned long eax = flg2MWAIT(state->flags);
+
+       if (boot_cpu_has(X86_FEATURE_ARAT))
+               return false;
+
+       /*
+        * Switch over to one-shot tick broadcast if the target C-state
+        * is deeper than C1.
+        */
+       if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK)
+               return true;
+
+       return false;
+}
+
 static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
 {
        int cstate;

@@ -1507,6 +1507,9 @@ static void __init intel_idle_init_cstat
                     !(cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_ALWAYS_ENABLE)))
                        drv->states[drv->state_count].flags |= CPUIDLE_FLAG_OFF;
 

drivers/idle/intel_idle.c static void __init intel_idle_init_cstat · line 1514

Quoted 5 lines

drivers/idle/intel_idle.c +3 -0

+               if (intel_idle_state_needs_timer_stop(&drv->states[drv->state_count]))
+                       drv->states[drv->state_count].flags |= CPUIDLE_FLAG_TIMER_STOP;
+
                drv->state_count++;
        }

This doesn't cover the ACPI case AFAICS.

Quoted 69 lines · Rafael J. Wysocki

Quoted 68 lines · Peter Zijlstra

cpuidle->enter() callbacks should not call into tracing because RCU has already been disabled. Instead of doing the broadcast thing itself, simply advertise to the cpuidle core that those states stop the timer.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/idle/intel_idle.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

drivers/idle/intel_idle.c +17 -17

@@ -126,26 +126,9 @@ static __cpuidle int intel_idle(struct c
        struct cpuidle_state *state = &drv->states[index];
        unsigned long eax = flg2MWAIT(state->flags);
        unsigned long ecx = 1; /* break on interrupt flag */
-       bool tick;
-
-       if (!static_cpu_has(X86_FEATURE_ARAT)) {
-               /*
-                * Switch over to one-shot tick broadcast if the target C-state
-                * is deeper than C1.
-                */
-               if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) {
-                       tick = true;
-                       tick_broadcast_enter();
-               } else {
-                       tick = false;
-               }
-       }
 
        mwait_idle_with_hints(eax, ecx);
 
-       if (!static_cpu_has(X86_FEATURE_ARAT) && tick)
-               tick_broadcast_exit();
-
        return index;
 }
 

@@ -1460,6 +1443,23 @@ static bool __init intel_idle_verify_cst
        return true;
 }
 
+static bool __init intel_idle_state_needs_timer_stop(struct cpuidle_state *state)
+{
+       unsigned long eax = flg2MWAIT(state->flags);
+
+       if (boot_cpu_has(X86_FEATURE_ARAT))
+               return false;
+
+       /*
+        * Switch over to one-shot tick broadcast if the target C-state
+        * is deeper than C1.
+        */
+       if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK)
+               return true;
+
+       return false;
+}
+
 static void __init intel_idle_init_cstates_icpu(struct cpuidle_driver *drv)
 {
        int cstate;

@@ -1507,6 +1507,9 @@ static void __init intel_idle_init_cstat
                     !(cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_ALWAYS_ENABLE)))
                        drv->states[drv->state_count].flags |= CPUIDLE_FLAG_OFF;
 

Quoted 7 lines

drivers/idle/intel_idle.c static void __init intel_idle_init_cstat · line 1514

Quoted 5 lines

drivers/idle/intel_idle.c +3 -0

+               if (intel_idle_state_needs_timer_stop(&drv->states[drv->state_count]))
+                       drv->states[drv->state_count].flags |= CPUIDLE_FLAG_TIMER_STOP;
+
                drv->state_count++;
        }

This doesn't cover the ACPI case AFAICS.

aa6b43d57f99 ("ACPI: processor: Use CPUIDLE_FLAG_TIMER_STOP")

did that, no?

intel_idle_init_cstates_acpi() needs to be updated too as it doesn't pick up the flags automatically.

It looks like CPUIDLE_FLAG_RCU_IDLE needs to be copied too.

Quoted 2 lines · Rafael J. Wysocki

intel_idle_init_cstates_acpi() needs to be updated too as it doesn't pick up the flags automatically.

Ooh, it has two different state init routines :-/, sorry I missed that. See below.

Quoted 1 line

It looks like CPUIDLE_FLAG_RCU_IDLE needs to be copied too.

I might need more clue again; processor_idle() needs this because it calls into ACPI (acpi_idle_enter_bm()) for that BM crud. I didn't find anything like that here.

---
Subject: intel_idle: Fix intel_idle() vs tracing
From: Peter Zijlstra <[email protected]>
Date: Fri Nov 20 11:28:35 CET 2020

cpuidle->enter() callbacks should not call into tracing because RCU has already been disabled. Instead of doing the broadcast thing itself, simply advertise to the cpuidle core that those states stop the timer.

Signed-off-by Peter Zijlstra (Intel) <[email protected]>

---
 drivers/idle/intel_idle.c |   40 +++++++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 17 deletions(-)

drivers/idle/intel_idle.c +5 -17

@@ -126,26 +126,9 @@ static __cpuidle int intel_idle(struct c
        struct cpuidle_state *state = &drv->states[index];
        unsigned long eax = flg2MWAIT(state->flags);
        unsigned long ecx = 1; /* break on interrupt flag */
-       bool tick;
-
-       if (!static_cpu_has(X86_FEATURE_ARAT)) {
-               /*
-                * Switch over to one-shot tick broadcast if the target C-state
-                * is deeper than C1.
-                */
-               if ((eax >> MWAIT_SUBSTATE_SIZE) & MWAIT_CSTATE_MASK) {
-                       tick = true;
-                       tick_broadcast_enter();
-               } else {
-                       tick = false;
-               }
-       }
 
        mwait_idle_with_hints(eax, ecx);
 
-       if (!static_cpu_has(X86_FEATURE_ARAT) && tick)
-               tick_broadcast_exit();
-
        return index;
 }
 

@@ -1227,6 +1210,23 @@ static bool __init intel_idle_acpi_cst_e
        return false;
 }
 
+static bool __init intel_idle_state_needs_timer_stop(struct cpuidle_state *state)
+{
+       unsigned long eax = flg2MWAIT(state->flags);
+
+       if (boot_cpu_has(X86_FEATURE_ARAT))

drivers/idle/intel_idle.c +8 -0

+}
+
 static void __init intel_idle_init_cstates_acpi(struct cpuidle_driver *drv)
 {
        int cstate, limit = min_t(int, CPUIDLE_STATE_MAX, acpi_state_table.count);

@@ -1269,6 +1269,9 @@ static void __init intel_idle_init_cstat
                if (disabled_states_mask & BIT(cstate))
                        state->flags |= CPUIDLE_FLAG_OFF;
 
+               if (intel_idle_state_needs_timer_stop(state))
+                       state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+
                state->enter = intel_idle;
                state->enter_s2idle = intel_idle_s2idle;
        }

@@ -1507,6 +1510,9 @@ static void __init intel_idle_init_cstat
                     !(cpuidle_state_table[cstate].flags & CPUIDLE_FLAG_ALWAYS_ENABLE)))
                        drv->states[drv->state_count].flags |= CPUIDLE_FLAG_OFF;
 
+               if (intel_idle_state_needs_timer_stop(&drv->states[drv->state_count]))
+                       drv->states[drv->state_count].flags |= CPUIDLE_FLAG_TIMER_STOP;
+
                drv->state_count++;
        }
 

Both arm64 and s390 are tripping over arch_cpu_idle() RCU,tracing,lockdep interaction. While looking at that I also found fail in inte_idle.

Please consider for this cycle.

Quoted 4 lines · Peter Zijlstra

Both arm64 and s390 are tripping over arch_cpu_idle() RCU,tracing,lockdep interaction. While looking at that I also found fail in inte_idle.

Please consider for this cycle.

Is anyone taking this patchset?

For s390, we also need to change the local_irq_safe/restore to the raw variants in enabled_wait() in arch/s390/kernel/idle.c.

Quoted 8 lines · Sven Schnelle

Quoted 4 lines · Peter Zijlstra

Both arm64 and s390 are tripping over arch_cpu_idle() RCU,tracing,lockdep interaction. While looking at that I also found fail in inte_idle.

Please consider for this cycle.

Is anyone taking this patchset?

I think I'll stuff it in x86/urgent for lack of a better place.