Re: [PATCH 04/14] powerpc/64s: idle process interrupts from system reset wakeup

[Nicholas Piggin <npiggin@gmail.com>]

On Mon, 12 Jun 2017 15:11:06 +0530
Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:

> Hi Nick,
> 
> On Mon, Jun 12, 2017 at 09:58:25AM +1000, Nicholas Piggin wrote:
> > 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.  
> 
> Should we clear the doorbell message if we are recording the fact that
> the Doorbell irq has happened in paca ?

As you saw, I clear it in the next patch.

It would be quite tidy to msgclear when moving srr1 wakeup into pending
mask. But on the other hand, I thought that deferring the clear as late
as possible and moving it closer to where IRQs will be hard enabled
again may help to avoid taking other IPIs.

> 
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
> > ---
> >  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(-)
> > 
> > diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> > index f06112cf8734..8366bdc69988 100644  
> 
> [..snip..]
> 
> > --- a/arch/powerpc/kernel/irq.c
> > +++ b/arch/powerpc/kernel/irq.c
> > @@ -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,
> > +	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;
> 	
> 	?

Yes, good catch. That will teach me not to verify after making a change.
Will fix.

Thanks,
Nick