diff mbox

[06/14] powerpc/64s: interrupt replay balance the return branch predictor

Message ID 20170611235835.7400-7-npiggin@gmail.com (mailing list archive)
State Superseded
Headers show

Commit Message

Nicholas Piggin June 11, 2017, 11:58 p.m. UTC
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 <npiggin@gmail.com>
---
 arch/powerpc/kernel/exceptions-64s.S | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Gautham R Shenoy June 12, 2017, 2:41 p.m. UTC | #1
On Mon, Jun 12, 2017 at 09:58:27AM +1000, Nicholas Piggin wrote:
> 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 <npiggin@gmail.com>

Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index a04ee0d7f88e..d55201625ea3 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -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
> -- 
> 2.11.0
>
Michael Ellerman June 13, 2017, 9:51 a.m. UTC | #2
Nicholas Piggin <npiggin@gmail.com> writes:

> 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 <npiggin@gmail.com>
> ---
>  arch/powerpc/kernel/exceptions-64s.S | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> index a04ee0d7f88e..d55201625ea3 100644
> --- a/arch/powerpc/kernel/exceptions-64s.S
> +++ b/arch/powerpc/kernel/exceptions-64s.S
> @@ -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?

cheers
Nicholas Piggin June 13, 2017, 11:09 a.m. UTC | #3
On Tue, 13 Jun 2017 19:51:19 +1000
Michael Ellerman <mpe@ellerman.id.au> wrote:

> Nicholas Piggin <npiggin@gmail.com> writes:
> 
> > 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 <npiggin@gmail.com>
> > ---
> >  arch/powerpc/kernel/exceptions-64s.S | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
> > index a04ee0d7f88e..d55201625ea3 100644
> > --- a/arch/powerpc/kernel/exceptions-64s.S
> > +++ b/arch/powerpc/kernel/exceptions-64s.S
> > @@ -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?

I can do that. Interrupt returns will now get significantly
attributed to this guy in profiles (and you can see it on some
sleep/wake workloads). __replay_interrupt is probably better
than arch_local_irq_restore, I guess.

Thanks,
Nick
diff mbox

Patch

diff --git a/arch/powerpc/kernel/exceptions-64s.S b/arch/powerpc/kernel/exceptions-64s.S
index a04ee0d7f88e..d55201625ea3 100644
--- a/arch/powerpc/kernel/exceptions-64s.S
+++ b/arch/powerpc/kernel/exceptions-64s.S
@@ -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