idle performance improvements

This simplifies the asm and fixes irq-off tracing over sleep instructions.

Also move powersave_nap check for POWER8 into C code, and move PSSCR register value calculation for POWER9 into C.

Signed-off-by Nicholas Piggin <[email protected]>

---
index 98a6d07ecb5c..35cf5bb7daed 100644

arch/powerpc/kernel/idle_book3s.S +0 -0

arch/powerpc/kernel/idle_book3s.S +2 -7

@@ -109,13 +109,9 @@ core_idle_lock_held:
 /*
  * Pass requested state in r3:
  *	r3 - PNV_THREAD_NAP/SLEEP/WINKLE in POWER8
- *	   - Requested STOP state in POWER9
+ *	   - Requested PSSCR value in POWER9
  *
- * To check IRQ_HAPPENED in r4
- * 	0 - don't check
- * 	1 - check
- *
- * Address to 'rfid' to in r5
+ * Address of idle handler to 'rfid' to in r4
  */
 pnv_powersave_common:
 	/* Use r3 to pass state nap/sleep/winkle */

@@ -131,30 +127,7 @@ pnv_powersave_common:
 	std	r0,_LINK(r1)
 	std	r0,_NIP(r1)
 
-	/* Hard disable interrupts */

arch/powerpc/kernel/idle_book3s.S +0 -7

-	beq	1f
-	addi	r1,r1,INT_FRAME_SIZE
-	ld	r0,16(r1)
-	li	r3,0			/* Return 0 (no nap) */
-	mtlr	r0
-	blr
-

arch/powerpc/platforms/powernv/subcore.c +1 -0

index 0babef11136f..d975d78188a9 100644
@@ -18,6 +18,7 @@
 #include <linux/stop_machine.h>
 
 #include <asm/cputhreads.h>
+#include <asm/cpuidle.h>
 #include <asm/kvm_ppc.h>
 #include <asm/machdep.h>
 #include <asm/opal.h>

arch/powerpc/platforms/powernv/subcore.c +0 -0

 		per_cpu(split_state, cpu).step = SYNC_STEP_UNSPLIT;
 		return;

drivers/cpuidle/cpuidle-powernv.c +5 -7

index 45eaf06462ae..79152676f62b 100644
@@ -73,9 +73,8 @@ static int nap_loop(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,
 			int index)
 {
-	ppc64_runlatch_off();
-	power7_idle();
-	ppc64_runlatch_on();
+	power7_idle_type(PNV_THREAD_NAP);
+
 	return index;
 }
 

@@ -98,7 +97,8 @@ static int fastsleep_loop(struct cpuidle_device *dev,
 	new_lpcr &= ~LPCR_PECE1;
 
 	mtspr(SPRN_LPCR, new_lpcr);
-	power7_sleep();
+
+	power7_idle_type(PNV_THREAD_SLEEP);
 
 	mtspr(SPRN_LPCR, old_lpcr);
 

@@ -110,10 +110,8 @@ static int stop_loop(struct cpuidle_device *dev,
 		     struct cpuidle_driver *drv,
 		     int index)
 {
-	ppc64_runlatch_off();
-	power9_idle_stop(stop_psscr_table[index].val,
+	power9_idle_type(stop_psscr_table[index].val,
 			 stop_psscr_table[index].mask);
-	ppc64_runlatch_on();
 	return index;
 }
 

Signature · 1 line

2.11.0

Hi Nick,

(Added Paul Mackerass to the Cc)

Quoted 6 lines · Nicholas Piggin

This simplifies the asm and fixes irq-off tracing over sleep instructions.

Also move powersave_nap check for POWER8 into C code, and move PSSCR register value calculation for POWER9 into C.

Thanks for doing this. Only one minor comment.

Quoted 5 lines

Signed-off-by Nicholas Piggin <[email protected]>

---
index 98a6d07ecb5c..35cf5bb7daed 100644

arch/powerpc/kernel/idle_book3s.S +0 -0

[..snip..]

Quoted 20 lines

arch/powerpc/kernel/idle_book3s.S +2 -6

@@ -109,13 +109,9 @@ core_idle_lock_held:
 /*
  * Pass requested state in r3:
  *	r3 - PNV_THREAD_NAP/SLEEP/WINKLE in POWER8
- *	   - Requested STOP state in POWER9
+ *	   - Requested PSSCR value in POWER9
  *
- * To check IRQ_HAPPENED in r4
- * 	0 - don't check
- * 	1 - check
- *
- * Address to 'rfid' to in r5
+ * Address of idle handler to 'rfid' to in r4
  */
 pnv_powersave_common:
 	/* Use r3 to pass state nap/sleep/winkle */

@@ -131,30 +127,7 @@ pnv_powersave_common:
 	std	r0,_LINK(r1)
 	std	r0,_NIP(r1)
 

arch/powerpc/kernel/idle_book3s.S pnv_powersave_common: · line 144

Quoted 11 lines

arch/powerpc/kernel/idle_book3s.S +0 -11

-	/* Hard disable interrupts */
-	mfmsr	r9
-	rldicl	r9,r9,48,1
-	rotldi	r9,r9,16
-	mtmsrd	r9,1			/* hard-disable interrupts */
-
-	/* Check if something happened while soft-disabled */
-	lbz	r0,PACAIRQHAPPENED(r13)
-	andi.	r0,r0,~PACA_IRQ_HARD_DIS@l
-	beq	1f
-	cmpwi	cr0,r4,0  

There were callers to power7_nap() in the dynamic-core-split/unsplit which wanted nap to be forced irrespective of whether an irq was pending or not. With this new patch, there won't be a way to enforce that on POWER8.

Hi Gautham,

Thanks for the reviews.

Quoted 41 lines · On Mon, 12 Jun 2017 14:07:27 +0530 Gautham R Shenoy <[email protected]>…

Hi Nick,

(Added Paul Mackerass to the Cc)

Quoted 6 lines · Nicholas Piggin

This simplifies the asm and fixes irq-off tracing over sleep instructions.

Also move powersave_nap check for POWER8 into C code, and move PSSCR register value calculation for POWER9 into C.

Thanks for doing this. Only one minor comment.

Quoted 5 lines

Signed-off-by Nicholas Piggin <[email protected]>

---
index 98a6d07ecb5c..35cf5bb7daed 100644

arch/powerpc/kernel/idle_book3s.S +0 -0

[..snip..]

Quoted 20 lines

arch/powerpc/kernel/idle_book3s.S +2 -6

@@ -109,13 +109,9 @@ core_idle_lock_held:
 /*
  * Pass requested state in r3:
  *	r3 - PNV_THREAD_NAP/SLEEP/WINKLE in POWER8
- *	   - Requested STOP state in POWER9
+ *	   - Requested PSSCR value in POWER9
  *
- * To check IRQ_HAPPENED in r4
- * 	0 - don't check
- * 	1 - check
- *
- * Address to 'rfid' to in r5
+ * Address of idle handler to 'rfid' to in r4
  */
 pnv_powersave_common:
 	/* Use r3 to pass state nap/sleep/winkle */

@@ -131,30 +127,7 @@ pnv_powersave_common:
 	std	r0,_LINK(r1)
 	std	r0,_NIP(r1)
 

Quoted 16 lines

arch/powerpc/kernel/idle_book3s.S pnv_powersave_common: · line 144

Quoted 11 lines

arch/powerpc/kernel/idle_book3s.S +0 -11

-	/* Hard disable interrupts */
-	mfmsr	r9
-	rldicl	r9,r9,48,1
-	rotldi	r9,r9,16
-	mtmsrd	r9,1			/* hard-disable interrupts */
-
-	/* Check if something happened while soft-disabled */
-	lbz	r0,PACAIRQHAPPENED(r13)
-	andi.	r0,r0,~PACA_IRQ_HARD_DIS@l
-	beq	1f
-	cmpwi	cr0,r4,0  

There were callers to power7_nap() in the dynamic-core-split/unsplit which wanted nap to be forced irrespective of whether an irq was pending or not. With this new patch, there won't be a way to enforce that on POWER8.

Actually there are two APIs to sleep now, the low level one which does not check lazy irq or nap disable is power7_idle_insn(). The one which sets up the lazy irq state is power7_idle_type(). The dynamic core split calls the former, so AFAIKS it should be equivalent.

Not the best names perhaps. Open to better suggestions.

Thanks,
Nick

Before the cpuidle driver is enabled, provide a default idle function similarly to POWER7/8.

This should not have much effect, because the cpuidle driver for powernv is mandatory, but if that changes we should have a fallback.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/platforms/powernv/idle.c | 2 ++
 1 file changed, 2 insertions(+)

arch/powerpc/platforms/powernv/idle.c +0 -0

index f188d84d9c59..e327e1585ddc 100644

arch/powerpc/platforms/powernv/idle.c +0 -0

 out:
 	return 0;

Signature · 1 line

2.11.0

Hi Nick,

Quoted 18 lines · Nicholas Piggin

Before the cpuidle driver is enabled, provide a default idle function similarly to POWER7/8.

This should not have much effect, because the cpuidle driver for powernv is mandatory, but if that changes we should have a fallback.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/platforms/powernv/idle.c | 2 ++
 1 file changed, 2 insertions(+)

arch/powerpc/platforms/powernv/idle.c +0 -0

index f188d84d9c59..e327e1585ddc 100644
@@ -677,6 +677,8 @@ static int __init pnv_init_idle_states(void)
 

arch/powerpc/platforms/powernv/idle.c static int __init pnv_init_idle_states(void) · line 681

Quoted 4 lines

arch/powerpc/platforms/powernv/idle.c +2 -0

 	if (supported_cpuidle_states & OPAL_PM_NAP_ENABLED)
 		ppc_md.power_save = power7_idle;
+	else if (supported_cpuidle_states & OPAL_PM_STOP_INST_FAST)
+		ppc_md.power_save = power9_idle;  

We are already initializing this in pnv_power9_idle_init() depending on whether the device tree has exposed at least one INST_FAST idle state. Else this should be NULL, because the firmware doesn't want us to use a platform idle state!

When the CPU wakes from low power state, it begins at the system reset interrupt with the exception that caused the wakeup encoded in SRR1.

Today, powernv idle wakeup ignores the wakeup reason (except a special case for HMI), and the regular interrupt corresponding to the exception will fire after the idle wakeup exits.

Change this to replay the interrupt from the idle wakeup before interrupts are hard-enabled.

Test on POWER8 of context_switch selftests benchmark with polling idle disabled (e.g., always nap, giving cross-CPU IPIs) gives the following results:

                                original         wakeup direct
Different threads, same core:   315k/s           264k/s
Different cores:                235k/s           242k/s

There is a slowdown for doorbell IPI (same core) case because system reset wakeup does not clear the message and the doorbell interrupt fires again needlessly.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/include/asm/hw_irq.h     |  2 ++
 arch/powerpc/kernel/irq.c             | 25 +++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/idle.c | 10 ++++++++--
 3 files changed, 35 insertions(+), 2 deletions(-)

arch/powerpc/include/asm/hw_irq.h +0 -0

index f06112cf8734..8366bdc69988 100644

arch/powerpc/kernel/irq.c +9 -0

@@ -348,6 +348,7 @@ bool prep_irq_for_idle(void)
 	return true;
 }
 
+#ifdef CONFIG_PPC_BOOK3S
 /*
  * This is for idle sequences that return with IRQs off, but the
  * idle state itself wakes on interrupt. Tell the irq tracer that

@@ -379,6 +380,30 @@ bool prep_irq_for_idle_irqsoff(void)
 }
 
 /*
+ * Take the SRR1 wakeup reason, index into this table to find the
+ * appropriate irq_happened bit.
+ */
+static const u8 srr1_to_irq[0x10] = {
+	0, 0, 0,
+	PACA_IRQ_DBELL,
+	0,
+	PACA_IRQ_DBELL,

Hi Nick,

Quoted 18 lines · Nicholas Piggin

When the CPU wakes from low power state, it begins at the system reset interrupt with the exception that caused the wakeup encoded in SRR1.

Today, powernv idle wakeup ignores the wakeup reason (except a special case for HMI), and the regular interrupt corresponding to the exception will fire after the idle wakeup exits.

Change this to replay the interrupt from the idle wakeup before interrupts are hard-enabled.

Test on POWER8 of context_switch selftests benchmark with polling idle disabled (e.g., always nap, giving cross-CPU IPIs) gives the following results:

                                original         wakeup direct
Different threads, same core:   315k/s           264k/s
Different cores:                235k/s           242k/s

Quoted 3 lines

There is a slowdown for doorbell IPI (same core) case because system reset wakeup does not clear the message and the doorbell interrupt fires again needlessly.

Should we clear the doorbell message if we are recording the fact that the Doorbell irq has happened in paca ?

Quoted 11 lines

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/include/asm/hw_irq.h     |  2 ++
 arch/powerpc/kernel/irq.c             | 25 +++++++++++++++++++++++++
 arch/powerpc/platforms/powernv/idle.c | 10 ++++++++--
 3 files changed, 35 insertions(+), 2 deletions(-)

arch/powerpc/include/asm/hw_irq.h +0 -0

index f06112cf8734..8366bdc69988 100644  

[..snip..]

Quoted 22 lines

arch/powerpc/kernel/irq.c +9 -0

@@ -348,6 +348,7 @@ bool prep_irq_for_idle(void)
 	return true;
 }
 
+#ifdef CONFIG_PPC_BOOK3S
 /*
  * This is for idle sequences that return with IRQs off, but the
  * idle state itself wakes on interrupt. Tell the irq tracer that

@@ -379,6 +380,30 @@ bool prep_irq_for_idle_irqsoff(void)
 }
 
 /*
+ * Take the SRR1 wakeup reason, index into this table to find the
+ * appropriate irq_happened bit.
+ */
+static const u8 srr1_to_irq[0x10] = {
+	0, 0, 0,
+	PACA_IRQ_DBELL,
+	0,
+	PACA_IRQ_DBELL,

arch/powerpc/kernel/irq.c bool prep_irq_for_idle_irqsoff(void) · line 400

Quoted 10 lines

arch/powerpc/kernel/irq.c +10 -0

+	PACA_IRQ_DEC,
+	0,
+	PACA_IRQ_EE,
+	PACA_IRQ_EE,
+	PACA_IRQ_HMI,
+	0, 0, 0, 0, 0 };
+
+void irq_set_pending_from_srr1(unsigned long srr1)
+{
+	unsigned int idx = (srr1 >> 18) & SRR1_WAKEMASK_P8;  

Shouldn't this be

	unsigned int idx = (srr1 & SRR1_WAKEMASK_P8) >> 18;
	
	?

msgsnd doorbell exceptions are cleared when the doorbell interrupt is taken. However if a doorbell exception causes a system reset interrupt wake from power saving state, the message is not cleared. Processing the doorbell from the system reset interrupt requires msgclr to avoid taking the exception again.

Testing this plus the previous wakup direct patch gives:

                                original         wakeup direct     msgclr
Different threads, same core:   315k/s           264k/s            345k/s
Different cores:                235k/s           242k/s            242k/s

Net speedup is +10% for same core, and +3% for different core.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/include/asm/dbell.h      | 13 +++++++++++++
 arch/powerpc/include/asm/ppc-opcode.h |  3 +++
 arch/powerpc/kernel/asm-offsets.c     |  1 +
 arch/powerpc/kernel/exceptions-64s.S  | 23 +++++++++++++++++++++--
 4 files changed, 38 insertions(+), 2 deletions(-)

arch/powerpc/include/asm/dbell.h +13 -0

index f70cbfe0ec04..9f2ae0d25e15 100644
@@ -56,6 +56,19 @@ static inline void ppc_msgsync(void)
 				: : "i" (CPU_FTR_HVMODE|CPU_FTR_ARCH_300));
 }
 
+static inline void _ppc_msgclr(u32 msg)
+{
+	__asm__ __volatile__ (ASM_FTR_IFSET(PPC_MSGCLR(%1), PPC_MSGCLRP(%1), %0)
+				: : "i" (CPU_FTR_HVMODE), "r" (msg));
+}
+
+static inline void ppc_msgclr(enum ppc_dbell type)
+{
+	u32 msg = PPC_DBELL_TYPE(type);
+
+	_ppc_msgclr(msg);
+}
+
 #else /* CONFIG_PPC_BOOK3S */
 
 #define PPC_DBELL_MSGTYPE		PPC_DBELL

arch/powerpc/include/asm/ppc-opcode.h +3 -0

index 3a8d278e7421..3b29c54e51fa 100644
@@ -221,6 +221,7 @@
 #define PPC_INST_MSGCLR			0x7c0001dc
 #define PPC_INST_MSGSYNC		0x7c0006ec
 #define PPC_INST_MSGSNDP		0x7c00011c
+#define PPC_INST_MSGCLRP		0x7c00015c
 #define PPC_INST_MTTMR			0x7c0003dc
 #define PPC_INST_NOP			0x60000000
 #define PPC_INST_PASTE			0x7c00070c

@@ -409,6 +410,8 @@
 					___PPC_RB(b))
 #define PPC_MSGSNDP(b)		stringify_in_c(.long PPC_INST_MSGSNDP | \
 					___PPC_RB(b))
+#define PPC_MSGCLRP(b)		stringify_in_c(.long PPC_INST_MSGCLRP | \
+					___PPC_RB(b))
 #define PPC_POPCNTB(a, s)	stringify_in_c(.long PPC_INST_POPCNTB | \
 					__PPC_RA(a) | __PPC_RS(s))
 #define PPC_POPCNTD(a, s)	stringify_in_c(.long PPC_INST_POPCNTD | \

arch/powerpc/kernel/asm-offsets.c +1 -0

index e15c178ba079..9624851ca276 100644
@@ -746,6 +746,7 @@ int main(void)
 #endif
 
 	DEFINE(PPC_DBELL_SERVER, PPC_DBELL_SERVER);
+	DEFINE(PPC_DBELL_MSGTYPE, PPC_DBELL_MSGTYPE);
 
 #ifdef CONFIG_PPC_8xx
 	DEFINE(VIRT_IMMR_BASE, (u64)__fix_to_virt(FIX_IMMR_BASE));

arch/powerpc/kernel/exceptions-64s.S +21 -2

index ae418b85c17c..a04ee0d7f88e 100644
@@ -1552,6 +1552,25 @@ END_FTR_SECTION_IFSET(CPU_FTR_CFAR)
 	b	1b
 
 /*
+ * When doorbell is triggered from system reset wakeup, the message is
+ * not cleared, so it would fire again when EE is enabled.
+ *
+ * When coming from local_irq_enable, there may be the same problem if
+ * we were hard disabled.
+ *
+ * Execute msgclr to clear pending exceptions before handling it.
+ */
+h_doorbell_common_msgclr:
+	LOAD_REG_IMMEDIATE(r3, PPC_DBELL_MSGTYPE << (63-36))
+	PPC_MSGCLR(3)
+	b 	h_doorbell_common
+
+doorbell_super_common_msgclr:
+	LOAD_REG_IMMEDIATE(r3, PPC_DBELL_MSGTYPE << (63-36))
+	PPC_MSGCLRP(3)
+	b 	doorbell_super_common
+
+/*
  * Called from arch_local_irq_enable when an interrupt needs
  * to be resent. r3 contains 0x500, 0x900, 0xa00 or 0xe80 to indicate
  * which kind of interrupt. MSR:EE is already off. We generate a

@@ -1576,13 +1595,13 @@ _GLOBAL(__replay_interrupt)
 	beq	hardware_interrupt_common
 BEGIN_FTR_SECTION
 	cmpwi	r3,0xe80
-	beq	h_doorbell_common
+	beq	h_doorbell_common_msgclr
 	cmpwi	r3,0xea0
 	beq	h_virt_irq_common
 	cmpwi	r3,0xe60
 	beq	hmi_exception_common
 FTR_SECTION_ELSE
 	cmpwi	r3,0xa00
-	beq	doorbell_super_common
+	beq	doorbell_super_common_msgclr
 ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
 	blr

Signature · 1 line

2.11.0

The __replay_interrupt code is branched to with bl, but the caller is returned to directly with rfid from the interrupt.

Instead return to a return stub that returns to the caller with blr, which should do better with the return predictor.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/kernel/exceptions-64s.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/powerpc/kernel/exceptions-64s.S +2 -1

index a04ee0d7f88e..d55201625ea3 100644
@@ -1586,7 +1586,7 @@ _GLOBAL(__replay_interrupt)
 	 * we don't give a damn about, so we don't bother storing them.
 	 */
 	mfmsr	r12
-	mflr	r11
+	LOAD_REG_ADDR(r11, __replay_interrupt_return)
 	mfcr	r9
 	ori	r12,r12,MSR_EE
 	cmpwi	r3,0x900

@@ -1604,4 +1604,5 @@ FTR_SECTION_ELSE
 	cmpwi	r3,0xa00
 	beq	doorbell_super_common_msgclr
 ALT_FTR_SECTION_END_IFSET(CPU_FTR_HVMODE)
+__replay_interrupt_return:
 	blr

Signature · 1 line

2.11.0

The __replay_interrupt code is branched to with bl, but the caller is returned to directly with rfid from the interrupt.

Instead return to a return stub that returns to the caller with blr, which should do better with the return predictor.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/kernel/exceptions-64s.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/powerpc/kernel/exceptions-64s.S +0 -0

index a04ee0d7f88e..d55201625ea3 100644

Quoted 11 lines · Nicholas Piggin

The __replay_interrupt code is branched to with bl, but the caller is returned to directly with rfid from the interrupt.

Instead return to a return stub that returns to the caller with blr, which should do better with the return predictor.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/kernel/exceptions-64s.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

arch/powerpc/kernel/exceptions-64s.S _GLOBAL(__replay_interrupt) · line 1589

Quoted 10 lines

arch/powerpc/kernel/exceptions-64s.S +1 -1

index a04ee0d7f88e..d55201625ea3 100644
@@ -1586,7 +1586,7 @@ _GLOBAL(__replay_interrupt)
 	 * we don't give a damn about, so we don't bother storing them.
 	 */
 	mfmsr	r12
-	mflr	r11
+	LOAD_REG_ADDR(r11, __replay_interrupt_return)  

Can you make it a local label, to make it clear nothing outside the file returns to there, and to not clutter the symbol map?

Idle code now always runs at the 0xc... effective address whether in real or virtual mode. This means rfid can be ditched, along with a lot of SRR manipulations.

In the wakeup path, carry SRR1 around in r12. Use mtmsrd to change MSR states as required.

This also balances the return prediction for the idle call, by doing blr rather than rfid to return to the idle caller.

On POWER9, 2-process context switch on different cores, with snooze disabled, increases performance by 2%.

---
 arch/powerpc/kernel/exceptions-64s.S    |  1 +
 arch/powerpc/kernel/idle_book3s.S       | 57 +++++++++++++++------------------
 arch/powerpc/kvm/book3s_hv_rmhandlers.S |  8 ++++-
 3 files changed, 33 insertions(+), 33 deletions(-)

arch/powerpc/kernel/idle_book3s.S +0 -0

index fec7c933d095..c3d0aef089a7 100644
@@ -148,12 +147,8 @@ pnv_powersave_common:
 	 * the MMU context to the guest.

Hi Nick,

Quoted 24 lines · Nicholas Piggin

Idle code now always runs at the 0xc... effective address whether in real or virtual mode. This means rfid can be ditched, along with a lot of SRR manipulations.

In the wakeup path, carry SRR1 around in r12. Use mtmsrd to change MSR states as required.

This also balances the return prediction for the idle call, by doing blr rather than rfid to return to the idle caller.

On POWER9, 2-process context switch on different cores, with snooze disabled, increases performance by 2%.

---
 arch/powerpc/kernel/exceptions-64s.S    |  1 +
 arch/powerpc/kernel/idle_book3s.S       | 57 +++++++++++++++------------------
 arch/powerpc/kvm/book3s_hv_rmhandlers.S |  8 ++++-
 3 files changed, 33 insertions(+), 33 deletions(-)

arch/powerpc/kernel/idle_book3s.S +0 -0

index fec7c933d095..c3d0aef089a7 100644
@@ -148,12 +147,8 @@ pnv_powersave_common:
 	 * the MMU context to the guest.

arch/powerpc/kernel/idle_book3s.S pnv_powersave_common: · line 151

Quoted 10 lines

arch/powerpc/kernel/idle_book3s.S +2 -6

 	 */
 	LOAD_REG_IMMEDIATE(r7, MSR_IDLE)
-	li	r6, MSR_RI
-	andc	r6, r9, r6
-	mtmsrd	r6, 1		/* clear RI before setting SRR0/1 */
-	mtspr	SPRN_SRR0, r4
-	mtspr	SPRN_SRR1, r7
-	rfid
+	mtmsrd	r7,0
+	bctr  

So at this point we need to transition from virtual to real mode as the comment in pnv_enter_arch207_idle_mode expects us to. Which is being performed by mtmsrd here. Then we jump to the function via bctr. So, in this patch we are using two instructions to modify the MSR and the PC, while earlier the rfid would update these atomically.

Does forgoing atomicity have any risk? I am asking this because historically we have modified IR/DR bits in the MSR via rfid mechanism.

In a busy system, idle wakeups can be expected from IPIs and device interrupts.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/kernel/idle_book3s.S | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

arch/powerpc/kernel/idle_book3s.S +1 -1

index 6305d4d7a268..32b76fb28352 100644
@@ -306,7 +306,7 @@ FTR_SECTION_ELSE_NESTED(66);						\
 	rlwinm	r0,r12,45-31,0xe;  /* P7 wake reason field is 3 bits */	\
 ALT_FTR_SECTION_END_NESTED_IFSET(CPU_FTR_ARCH_207S, 66);		\
 	cmpwi	r0,0xa;			/* Hypervisor maintenance ? */	\
-	bne	20f;							\
+	bne+	20f;							\
 	/* Invoke opal call to handle hmi */				\
 	ld	r2,PACATOC(r13);					\
 	ld	r1,PACAR1(r13);						\

Signature · 1 line

2.11.0

local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.

Expand the TIF_POLLING_NRFLAG coverage to as large as possible.

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 4 +++-
 drivers/cpuidle/cpuidle-pseries.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +3 -1

index 79152676f62b..50b3c2e0306f 100644
@@ -51,9 +51,10 @@ static int snooze_loop(struct cpuidle_device *dev,
 {
 	u64 snooze_exit_time;
 
-	local_irq_enable();
 	set_thread_flag(TIF_POLLING_NRFLAG);
 
+	local_irq_enable();
+
 	snooze_exit_time = get_tb() + snooze_timeout;
 	ppc64_runlatch_off();
 	HMT_very_low();

@@ -66,6 +67,7 @@ static int snooze_loop(struct cpuidle_device *dev,
 	ppc64_runlatch_on();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
 	smp_mb();
+
 	return index;
 }
 

drivers/cpuidle/cpuidle-pseries.c +2 -1

index 166ccd711ec9..7b12bb2ea70f 100644
@@ -62,9 +62,10 @@ static int snooze_loop(struct cpuidle_device *dev,
 	unsigned long in_purr;
 	u64 snooze_exit_time;
 
+	set_thread_flag(TIF_POLLING_NRFLAG);
+
 	idle_loop_prolog(&in_purr);
 	local_irq_enable();
-	set_thread_flag(TIF_POLLING_NRFLAG);
 	snooze_exit_time = get_tb() + snooze_timeout;
 
 	while (!need_resched()) {

Signature · 1 line

2.11.0

Quoted 5 lines · Nicholas Piggin

local_irq_enable can cause interrupts to be taken which could take significant amount of processing time. The idle process should set its polling flag before this, so another process that wakes it during this time will not have to send an IPI.

Quoted 3 lines

Expand the TIF_POLLING_NRFLAG coverage to as large as possible.

Signed-off-by Nicholas Piggin <[email protected]>

Looks good. Were you able to see this make a difference in any of the tests ?

Ensure these don't get put into bouncing cachelines.

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 10 +++++-----
 drivers/cpuidle/cpuidle-pseries.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +5 -5

index 50b3c2e0306f..9d03326ac05e 100644
@@ -32,18 +32,18 @@ static struct cpuidle_driver powernv_idle_driver = {
 	.owner            = THIS_MODULE,
 };
 
-static int max_idle_state;
-static struct cpuidle_state *cpuidle_state_table;
+static int max_idle_state __read_mostly;
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
 
 struct stop_psscr_table {
 	u64 val;
 	u64 mask;
 };
 
-static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX];
+static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly;
 
-static u64 snooze_timeout;
-static bool snooze_timeout_en;
+static u64 snooze_timeout __read_mostly;
+static bool snooze_timeout_en __read_mostly;
 
 static int snooze_loop(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,

drivers/cpuidle/cpuidle-pseries.c +4 -4

index 7b12bb2ea70f..a404f352d284 100644
@@ -25,10 +25,10 @@ struct cpuidle_driver pseries_idle_driver = {
 	.owner            = THIS_MODULE,
 };
 
-static int max_idle_state;
-static struct cpuidle_state *cpuidle_state_table;
-static u64 snooze_timeout;
-static bool snooze_timeout_en;
+static int max_idle_state __read_mostly;
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
+static u64 snooze_timeout __read_mostly;
+static bool snooze_timeout_en __read_mostly;
 
 static inline void idle_loop_prolog(unsigned long *in_purr)
 {

Signature · 1 line

2.11.0

Quoted 3 lines · Nicholas Piggin

Ensure these don't get put into bouncing cachelines.

Signed-off-by Nicholas Piggin <[email protected]>

Reviewed-by Gautham R. Shenoy <[email protected]>

Quoted 41 lines

---
 drivers/cpuidle/cpuidle-powernv.c | 10 +++++-----
 drivers/cpuidle/cpuidle-pseries.c |  8 ++++----
 2 files changed, 9 insertions(+), 9 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +5 -5

index 50b3c2e0306f..9d03326ac05e 100644
@@ -32,18 +32,18 @@ static struct cpuidle_driver powernv_idle_driver = {
 	.owner            = THIS_MODULE,
 };
 
-static int max_idle_state;
-static struct cpuidle_state *cpuidle_state_table;
+static int max_idle_state __read_mostly;
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
 
 struct stop_psscr_table {
 	u64 val;
 	u64 mask;
 };
 
-static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX];
+static struct stop_psscr_table stop_psscr_table[CPUIDLE_STATE_MAX] __read_mostly;
 
-static u64 snooze_timeout;
-static bool snooze_timeout_en;
+static u64 snooze_timeout __read_mostly;
+static bool snooze_timeout_en __read_mostly;
 
 static int snooze_loop(struct cpuidle_device *dev,
 			struct cpuidle_driver *drv,

drivers/cpuidle/cpuidle-pseries.c +0 -0

index 7b12bb2ea70f..a404f352d284 100644
@@ -25,10 +25,10 @@ struct cpuidle_driver pseries_idle_driver = {
 	.owner            = THIS_MODULE,
 };
 

drivers/cpuidle/cpuidle-pseries.c struct cpuidle_driver pseries_idle_driver = { · line 32

Quoted 9 lines

drivers/cpuidle/cpuidle-pseries.c +4 -4

-static int max_idle_state;
-static struct cpuidle_state *cpuidle_state_table;
-static u64 snooze_timeout;
-static bool snooze_timeout_en;
+static int max_idle_state __read_mostly;
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
+static u64 snooze_timeout __read_mostly;
+static bool snooze_timeout_en __read_mostly;
 

A memory barrier is not required after the task wakes up, only if we clear the polling flag before waking. The case where we have work to do is the important one, so optimise for it.

Signed-off-by Nicholas Piggin <[email protected]>

---
 drivers/cpuidle/cpuidle-powernv.c | 11 +++++++++--
 drivers/cpuidle/cpuidle-pseries.c | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +6 -1

index 9d03326ac05e..37b0698b7193 100644
@@ -59,14 +59,21 @@ static int snooze_loop(struct cpuidle_device *dev,
 	ppc64_runlatch_off();
 	HMT_very_low();
 	while (!need_resched()) {
-		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time)
+		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time) {
+			/*
+			 * Task has not woken up but we are exiting the polling
+			 * loop anyway. Require a barrier after polling is
+			 * cleared to order subsequent test of need_resched().
+			 */

Quoted 6 lines · Nicholas Piggin

A memory barrier is not required after the task wakes up, only if we clear the polling flag before waking. The case where we have work to do is the important one, so optimise for it.

Signed-off-by Nicholas Piggin <[email protected]>

Reviewed-by Vaidyanathan Srinivasan <[email protected]>

Quoted 20 lines

---
 drivers/cpuidle/cpuidle-powernv.c | 11 +++++++++--
 drivers/cpuidle/cpuidle-pseries.c | 11 +++++++++--
 2 files changed, 18 insertions(+), 4 deletions(-)

drivers/cpuidle/cpuidle-powernv.c +6 -1

index 9d03326ac05e..37b0698b7193 100644
@@ -59,14 +59,21 @@ static int snooze_loop(struct cpuidle_device *dev,
 	ppc64_runlatch_off();
 	HMT_very_low();
 	while (!need_resched()) {
-		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time)
+		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time) {
+			/*
+			 * Task has not woken up but we are exiting the polling
+			 * loop anyway. Require a barrier after polling is
+			 * cleared to order subsequent test of need_resched().
+			 */

drivers/cpuidle/cpuidle-powernv.c static int snooze_loop(struct cpuidle_device *dev, · line 69

Quoted 10 lines

drivers/cpuidle/cpuidle-powernv.c +3 -1

+			clear_thread_flag(TIF_POLLING_NRFLAG);
+			smp_mb();
 			break;
+		}
 	}
 
 	HMT_medium();
 	ppc64_runlatch_on();
 	clear_thread_flag(TIF_POLLING_NRFLAG);
-	smp_mb();  

If we reach here without executing if(snooze_timeout) with the barrier+break, that means we have seen the need_resched flag and hence we can avoid the barrier. Clearing of the polling flag can be seen (take affect) later as we will exit the idle loop and re-enter anyway at this point. The caller also will check for need_resched and exit the idle loop.

Actually do_idle() has a __current_set_polling() and __current_clr_polling() in the default idle loop. Do we really need to set/clear the TIF_POLLING_NRFLAG again in cpuidle driver?

The CTRL register is read-only except bit 63 which is the run latch control. This means it can be updated with a mtspr rather than mfspr/mtspr.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/kernel/process.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

arch/powerpc/kernel/process.c +1 -5

index baae104b16c7..a44ea034c226 100644
@@ -1960,12 +1960,8 @@ void show_stack(struct task_struct *tsk, unsigned long *stack)
 void notrace __ppc64_runlatch_on(void)
 {
 	struct thread_info *ti = current_thread_info();
-	unsigned long ctrl;
-
-	ctrl = mfspr(SPRN_CTRLF);
-	ctrl |= CTRL_RUNLATCH;
-	mtspr(SPRN_CTRLT, ctrl);
 
+	mtspr(SPRN_CTRLT, CTRL_RUNLATCH);
 	ti->local_flags |= _TLF_RUNLATCH;
 }
 

@@ -1973,13 +1969,9 @@ void notrace __ppc64_runlatch_on(void)
 void notrace __ppc64_runlatch_off(void)

Quoted 16 lines · Nicholas Piggin

The CTRL register is read-only except bit 63 which is the run latch control. This means it can be updated with a mtspr rather than mfspr/mtspr.

Signed-off-by Nicholas Piggin <[email protected]>

arch/powerpc/kernel/process.c +0 -1

index baae104b16c7..a44ea034c226 100644
@@ -1973,13 +1969,9 @@ void notrace __ppc64_runlatch_on(void)
 void notrace __ppc64_runlatch_off(void)
 {
 	struct thread_info *ti = current_thread_info();
-	unsigned long ctrl;
 

arch/powerpc/kernel/process.c void notrace __ppc64_runlatch_on(void) · line 1974

Quoted 6 lines

arch/powerpc/kernel/process.c +1 -4

 	ti->local_flags &= ~_TLF_RUNLATCH;
-
-	ctrl = mfspr(SPRN_CTRLF);
-	ctrl &= ~CTRL_RUNLATCH;
-	mtspr(SPRN_CTRLT, ctrl);
+	mtspr(SPRN_CTRLT, 0);

Good idea. Writing to CTRL register can change only the RUN field. Was this any different in older generations?

Anton and Ben kept the mfspr/mtspr part in earlier updates to this routine.

Quoted 18 lines · Vaidyanathan Srinivasan

Quoted 16 lines · Nicholas Piggin

The CTRL register is read-only except bit 63 which is the run latch control. This means it can be updated with a mtspr rather than mfspr/mtspr.

Signed-off-by Nicholas Piggin <[email protected]>

arch/powerpc/kernel/process.c +0 -1

index baae104b16c7..a44ea034c226 100644
@@ -1973,13 +1969,9 @@ void notrace __ppc64_runlatch_on(void)
 void notrace __ppc64_runlatch_off(void)
 {
 	struct thread_info *ti = current_thread_info();
-	unsigned long ctrl;
 

Quoted 9 lines

arch/powerpc/kernel/process.c void notrace __ppc64_runlatch_on(void) · line 1974

Quoted 6 lines

arch/powerpc/kernel/process.c +1 -4

 	ti->local_flags &= ~_TLF_RUNLATCH;
-
-	ctrl = mfspr(SPRN_CTRLF);
-	ctrl &= ~CTRL_RUNLATCH;
-	mtspr(SPRN_CTRLT, ctrl);
+	mtspr(SPRN_CTRLT, 0);  

Good idea. Writing to CTRL register can change only the RUN field. Was this any different in older generations?

No AFAICS back to 2.02.

Quoted 2 lines

Anton and Ben kept the mfspr/mtspr part in earlier updates to this routine.

Doing the read/modify write is forward compatible vs a new writable field, whereas writing the whole register with a known value is not.

Quoted 2 lines

Good idea.  Writing to CTRL register can change only the RUN field. Was this any different in older generations?

No AFAICS back to 2.02.

Quoted 2 lines

Anton and Ben kept the mfspr/mtspr part in earlier updates to this routine.

Doing the read/modify write is forward compatible vs a new writable field, whereas writing the whole register with a known value is not.

Quoted 1 line · Michael Ellerman

Quoted 1 line

Good idea.=C2=A0 Writing to CTRL register can change only the RUN fiel=

Quoted 9 lines

Quoted 1 line

Was this any different in older generations?

=20
No AFAICS back to 2.02.
=20

Quoted 2 lines

Anton and Ben kept the mfspr/mtspr part in earlier updates to this routine.

=20
Doing the read/modify write is forward compatible vs a new writable field, whereas writing the whole register with a known value is not.

At this stage I wouldn't worry too much about it. What we can do is write a pre-cooked value (from reading it earlier once at boot) if we are paranoid or just do what Nick does and put the onus on future designs that might want to re-use it for other things to add a mode bits to configure the new feature in.

2*mfmsr and 2*mtmsr can be avoided in the idle sleep/wake code because we know the MSR[EE] is clear.

Signed-off-by Nicholas Piggin <[email protected]>

---
 arch/powerpc/platforms/powernv/idle.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

arch/powerpc/platforms/powernv/idle.c +6 -6

index ee416e016387..84c55a5ef7ea 100644
@@ -291,9 +291,9 @@ static unsigned long __power7_idle_type(unsigned long type)
 	if (!prep_irq_for_idle_irqsoff())
 		return 0;
 
-	ppc64_runlatch_off();
+	__ppc64_runlatch_off();
 	srr1 = power7_idle_insn(type);
-	ppc64_runlatch_on();
+	__ppc64_runlatch_on();
 
 	fini_irq_for_idle_irqsoff();
 

@@ -328,9 +328,9 @@ static unsigned long __power9_idle_type(unsigned long stop_psscr_val,
 	psscr = mfspr(SPRN_PSSCR);
 	psscr = (psscr & ~stop_psscr_mask) | stop_psscr_val;
 
-	ppc64_runlatch_off();
+	__ppc64_runlatch_off();
 	srr1 = power9_idle_stop(psscr);
-	ppc64_runlatch_on();
+	__ppc64_runlatch_on();
 
 	fini_irq_for_idle_irqsoff();
 

@@ -365,7 +365,7 @@ unsigned long pnv_cpu_offline(unsigned int cpu)
 	unsigned long srr1;
 	u32 idle_states = pnv_get_supported_cpuidle_states();
 
-	ppc64_runlatch_off();
+	__ppc64_runlatch_off();
 
 	if (cpu_has_feature(CPU_FTR_ARCH_300) && deepest_stop_found) {
 		unsigned long psscr;

@@ -392,7 +392,7 @@ unsigned long pnv_cpu_offline(unsigned int cpu)
 		HMT_medium();
 	}
 
-	ppc64_runlatch_on();
+	__ppc64_runlatch_on();
 
 	return srr1;
 }

Signature · 1 line

2.11.0