Re: [PATCH 08/14] powerpc/64s: idle avoid SRR usage in idle sleep/wake paths

On Tue, 13 Jun 2017 15:55:53 +0530
Gautham R Shenoy [off-list ref] wrote:

Hi Nick,

On Mon, Jun 12, 2017 at 09:58:29AM +1000, Nicholas Piggin wrote:

quoted

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(-)

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index fec7c933d095..c3d0aef089a7 100644
--- a/arch/powerpc/kernel/idle_book3s.S
+++ b/arch/powerpc/kernel/idle_book3s.S

@@ -148,12 +147,8 @@ pnv_powersave_common:
 	 * the MMU context to the guest.
 	 */
 	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.

I believe it's not a problem. Actually the ISA has a note about using
it in this way, p.1181 of ISA v3.0B, the programming note suggests
using mtmsrd rather than rfid to enable IR.

I've tested on POWER8 and 9 and not had any problems with it. Interesting
question though.

One thing I wonder about is that ERAT installs real mode entries, so
using mtmsrd to transition from real to virtual mode will require 2
I-ERAT entries for these nearby instruction addresses. Then if you did
a branch to a distant address, that would require another I-ERAT. On
the other hand if you do an rfid to switch MSR and branch to distant
address at the same time, it should only require 2 I-ERAT entries. So
you may see better microbenchmark performance of the first case, but
the latter may end up being slower on real work.

I've decided that probably the kernel is compact enough that we aren't
likely to cause more ERAT footprint by doing this. It would be
interesting to do a proper analysis of this, but I haven't got around
to it yet.

Thanks,
Nick