Re: [PATCH 00/14 v2] idle performance improvements

[Nicholas Piggin <npiggin@gmail.com>]

On Mon, 12 Jun 2017 14:25:24 +1000
Benjamin Herrenschmidt <benh@au1.ibm.com> wrote:

> On Sun, 2017-06-11 at 19:30 +1000, Nicholas Piggin wrote:
> > I rebased this on the powerpc next tree.
> > 
> > A couple of things are changed since last post:
> > 
> > - Patch 1 now properly accounts for the fact the powernv idle
> >   wakeups do not re-enable interrupts until the cpuidle driver
> >   enables them. This was not quite right in the previous patch
> >   (and prep_irq_for_idle() is not quite right for that case so
> >   a new primitive has to be introduced).  
> 
> What do you mean ? We shouldn't be going to sleep with the CPU thinking
> it's interrupts are off, otherwise we end up effectively "taking an
> interrupt while off" which is not right and it will cause accounting to
> think we are off for too long.
> 
> Is this a generic cpuidle problem or a powerpc issue ?

cpuidle drivers enter their idle state with local_irq_disable().
powernv cpuidle driver currently does not call trace_hardirqs_on()
before going to sleep (e.g., it does not use prep_irq_for_idle()).

I did a previous patch that uses prep_irq_for_idle directly, but
that assumes when we return from idle that local irqs should be
on. The generic cpuidle does not want that, I haven't dug into
exactly why not. But it seems to work better just to put the SRR1
wakeup reason into the irq_pending bit and let the lazy irq logic
take care of the rest.
 
> I'd rather we don't have to of those "prep_for_idle...". If necessary
> sync the other one.

Okay one can call the other rather than implementing twice.

Thanks,
Nick