Skip to contentSkip to conversation

powerpc/64s: exception optimise MSR handling

PATCH Applied Nicholas Piggin 3 participants 4 messages Sep 15, 2016 – Sep 20, 2016 PowerPC

lore
Share & tools
lore

Nicholas Piggin

author reviewer
#0 lore raw

PATCH 1/1 powerpc/64s: exception optimise MSR handling

mtmsrd with L=1 only affects MSR_EE and MSR_RI bits, and we always know what state those bits are, so the kernel MSR does not need to be loaded when modifying them.

mtmsrd is often in the critical execution path, so avoiding dependency on even L1 load is noticable. On a POWER8 this saves about 3 cycles from the syscall path, and possibly a few from other exception returns (not measured).

---
 arch/powerpc/kernel/entry_64.S | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)
arch/powerpc/kernel/entry_64.S +9 -12
index 6b8bc0d..585b9ca 100644
@@ -139,7 +139,7 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	1
 #else
-	ld	r11,PACAKMSR(r13)
+	li	r11,MSR_RI
 	ori	r11,r11,MSR_EE
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */
@@ -195,7 +195,6 @@ system_call:			/* label this so stack traces look sane */
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	0
 #else
-	ld	r10,PACAKMSR(r13)
 	/*
 	 * For performance reasons we clear RI the same time that we
 	 * clear EE. We only need to clear RI just before we restore r13
@@ -203,8 +202,7 @@ system_call:			/* label this so stack traces look sane */
 	 * We have to be careful to restore RI if we branch anywhere from
 	 * here (eg syscall_exit_work).
 	 */
-	li	r9,MSR_RI
-	andc	r11,r10,r9
+	li	r11,0
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */
 
@@ -221,13 +219,12 @@ system_call:			/* label this so stack traces look sane */
 #endif
 2:	addi    r3,r1,STACK_FRAME_OVERHEAD
 #ifdef CONFIG_PPC_BOOK3S
+	li	r10,MSR_RI
 	mtmsrd	r10,1		/* Restore RI */
 #endif
 	bl	restore_math
 #ifdef CONFIG_PPC_BOOK3S
-	ld	r10,PACAKMSR(r13)
-	li	r9,MSR_RI
-	andc	r11,r10,r9 /* Re-clear RI */
+	li	r11,0
 	mtmsrd	r11,1
 #endif
 	ld	r8,_MSR(r1)
@@ -308,6 +305,7 @@ syscall_enosys:
 	
 syscall_exit_work:
 #ifdef CONFIG_PPC_BOOK3S
+	li	r10,MSR_RI
 	mtmsrd	r10,1		/* Restore RI */
 #endif
 	/* If TIF_RESTOREALL is set, don't scribble on either r3 or ccr.
@@ -354,7 +352,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	1
 #else
-	ld	r10,PACAKMSR(r13)
+	li	r10,MSR_RI
 	ori	r10,r10,MSR_EE
 	mtmsrd	r10,1
 #endif /* CONFIG_PPC_BOOK3E */
@@ -619,7 +617,7 @@ _GLOBAL(ret_from_except_lite)
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	0
 #else
-	ld	r10,PACAKMSR(r13) /* Get kernel MSR without EE */
+	li	r10,MSR_RI
 	mtmsrd	r10,1		  /* Update machine state */
 #endif /* CONFIG_PPC_BOOK3E */
 
@@ -751,7 +749,7 @@ resume_kernel:
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	0
 #else
-	ld	r10,PACAKMSR(r13) /* Get kernel MSR without EE */
+	li	r10,MSR_RI
 	mtmsrd	r10,1		  /* Update machine state */
 #endif /* CONFIG_PPC_BOOK3E */
 #endif /* CONFIG_PREEMPT */
@@ -841,8 +839,7 @@ END_FTR_SECTION_IFSET(CPU_FTR_HAS_PPR)
 	 * userspace and we take an exception after restoring r13,
 	 * we end up corrupting the userspace r13 value.
 	 */
-	ld	r4,PACAKMSR(r13) /* Get kernel MSR without EE */
-	andc	r4,r4,r0	 /* r0 contains MSR_RI here */
+	li	r4,0
 	mtmsrd	r4,1
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
Signature · 1 line
2.9.3
#1 lore raw
Quoted 8 lines · Nicholas Piggin

mtmsrd with L=1 only affects MSR_EE and MSR_RI bits, and we always know what state those bits are, so the kernel MSR does not need to be loaded when modifying them.

mtmsrd is often in the critical execution path, so avoiding dependency on even L1 load is noticable. On a POWER8 this saves about 3 cycles from the syscall path, and possibly a few from other exception returns (not measured).

This looks good in principle.

My worry is that these code paths have lots of assumptions about what's left in registers, so we may have a path somewhere which expects rX to contain PACAKMSR. Hence the review below ...

arch/powerpc/kernel/entry_64.S END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR) · line 145

Quoted 13 lines
arch/powerpc/kernel/entry_64.S +1 -1
index 6b8bc0d..585b9ca 100644
@@ -139,7 +139,7 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	1
 #else
-	ld	r11,PACAKMSR(r13)
+	li	r11,MSR_RI
 	ori	r11,r11,MSR_EE
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */
	/* We do need to set SOFTE in the stack frame or the return
	 * from interrupt will be painful
	 */
	li	r10,1
	std	r10,SOFTE(r1)

	CURRENT_THREAD_INFO(r11, r1)

So that's one OK. r11 isn't used again until its clobbered here.

arch/powerpc/kernel/entry_64.S system_call: /* label this so stack traces look sane */

Quoted 1 line
arch/powerpc/kernel/entry_64.S +0 -0
@@ -195,7 +195,6 @@ system_call:			/* label this so stack traces look sane */
#ifdef CONFIG_PPC_BOOK3S
	/* No MSR:RI on BookE */
	andi.	r10,r8,MSR_RI
	beq-	unrecov_restore

#endif

So at this point r10 == MSR_RI, otherwise we would have taken the branch.

	/*
	 * Disable interrupts so current_thread_info()->flags can't change,
	 * and so that we don't get interrupted after loading SRR0/1.
	 */
Quoted 8 lines
arch/powerpc/kernel/entry_64.S +0 -1
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	0
 #else
-	ld	r10,PACAKMSR(r13)
 	/*
 	 * For performance reasons we clear RI the same time that we
 	 * clear EE. We only need to clear RI just before we restore r13

* below, but batching it with EE saves us one expensive mtmsrd call.

arch/powerpc/kernel/entry_64.S system_call: /* label this so stack traces look sane */ · line 207

Quoted 8 lines
arch/powerpc/kernel/entry_64.S +1 -2
 	 * We have to be careful to restore RI if we branch anywhere from
 	 * here (eg syscall_exit_work).
 	 */
-	li	r9,MSR_RI
-	andc	r11,r10,r9
+	li	r11,0
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */
	ld	r9,TI_FLAGS(r12)
	li	r11,-MAX_ERRNO
	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
	bne-	syscall_exit_work

Which is:

syscall_exit_work:

#ifdef CONFIG_PPC_BOOK3S
	mtmsrd	r10,1		/* Restore RI */

#endif

So that appears to still work. But it's super fragile.

What I'd like to do is drop that optimisation of clearing RI early with EE. That would avoid us needing to restore RI in syscall_exit_work and before restore_math (and reclearing it after).

But I guess that will hurt performance too much :/

cheers

author reviewer
#2 lore raw
Quoted 78 lines · On Tue, 20 Sep 2016 14:25:48 +1000 Michael Ellerman <[email protected]> wrote:
Quoted 8 lines · Nicholas Piggin

mtmsrd with L=1 only affects MSR_EE and MSR_RI bits, and we always know what state those bits are, so the kernel MSR does not need to be loaded when modifying them.

mtmsrd is often in the critical execution path, so avoiding dependency on even L1 load is noticable. On a POWER8 this saves about 3 cycles from the syscall path, and possibly a few from other exception returns (not measured).

This looks good in principle.

My worry is that these code paths have lots of assumptions about what's left in registers, so we may have a path somewhere which expects rX to contain PACAKMSR. Hence the review below ...

arch/powerpc/kernel/entry_64.S END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR) · line 145

Quoted 13 lines
arch/powerpc/kernel/entry_64.S +1 -1
index 6b8bc0d..585b9ca 100644
@@ -139,7 +139,7 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	1
 #else
-	ld	r11,PACAKMSR(r13)
+	li	r11,MSR_RI
 	ori	r11,r11,MSR_EE
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */  
	/* We do need to set SOFTE in the stack frame or the return
	 * from interrupt will be painful
	 */
	li	r10,1
	std	r10,SOFTE(r1)

	CURRENT_THREAD_INFO(r11, r1)

So that's one OK. r11 isn't used again until its clobbered here.

arch/powerpc/kernel/entry_64.S system_call: /* label this so stack traces look sane */

Quoted 1 line
arch/powerpc/kernel/entry_64.S +0 -0
@@ -195,7 +195,6 @@ system_call:			/* label this so stack traces look sane */  
#ifdef CONFIG_PPC_BOOK3S
	/* No MSR:RI on BookE */
	andi.	r10,r8,MSR_RI
	beq-	unrecov_restore

#endif

So at this point r10 == MSR_RI, otherwise we would have taken the branch.

	/*
	 * Disable interrupts so current_thread_info()->flags can't change,
	 * and so that we don't get interrupted after loading SRR0/1.
	 */
Quoted 8 lines
arch/powerpc/kernel/entry_64.S +0 -1
 #ifdef CONFIG_PPC_BOOK3E
 	wrteei	0
 #else
-	ld	r10,PACAKMSR(r13)
 	/*
 	 * For performance reasons we clear RI the same time that we
 	 * clear EE. We only need to clear RI just before we restore r13

* below, but batching it with EE saves us one expensive mtmsrd call.

arch/powerpc/kernel/entry_64.S system_call: /* label this so stack traces look sane */ · line 207

Quoted 8 lines
arch/powerpc/kernel/entry_64.S +1 -2
 	 * We have to be careful to restore RI if we branch anywhere from
 	 * here (eg syscall_exit_work).
 	 */
-	li	r9,MSR_RI
-	andc	r11,r10,r9
+	li	r11,0
 	mtmsrd	r11,1
 #endif /* CONFIG_PPC_BOOK3E */  
	ld	r9,TI_FLAGS(r12)
	li	r11,-MAX_ERRNO
	andi.	r0,r9,(_TIF_SYSCALL_DOTRACE|_TIF_SINGLESTEP|_TIF_USER_WORK_MASK|_TIF_PERSYSCALL_MASK)
	bne-	syscall_exit_work
Quoted 8 lines

Which is:

syscall_exit_work:

#ifdef CONFIG_PPC_BOOK3S
	mtmsrd	r10,1		/* Restore RI */

#endif

So that appears to still work. But it's super fragile.

Agreed. We'll go with the idea you mentioned offline to just load r10 again here to avoid the long dependency -- it's not going to be a measurable cost.

Quoted 5 lines

What I'd like to do is drop that optimisation of clearing RI early with EE. That would avoid us needing to restore RI in syscall_exit_work and before restore_math (and reclearing it after).

But I guess that will hurt performance too much :/

Yes that's something to look into. Newer cores, more kernel code using fp registers. I'll look into it.

Thanks,
Nick

#3 lore raw
In reply to Nicholas Piggin
Quoted 10 lines · Nicholas Piggin

mtmsrd with L=1 only affects MSR_EE and MSR_RI bits, and we always know what state those bits are, so the kernel MSR does not need to be loaded when modifying them.

mtmsrd is often in the critical execution path, so avoiding dependency on even L1 load is noticable. On a POWER8 this saves about 3 cycles from the syscall path, and possibly a few from other exception returns (not measured).

Applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/49d09bf2a66f4b5a6eabefb0d4

cheers