Re: [PATCH 7/7] powerpc/64s: idle POWER8 avoid full state loss recovery where possible

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

Hi Nick,

On Mon, Mar 20, 2017 at 08:26:05PM +1000, Nicholas Piggin wrote:
> On Mon, 20 Mar 2017 15:41:39 +0530
> Gautham R Shenoy <ego@linux.vnet.ibm.com> wrote:
> 
> > Hi Nick,
> > 
> > On Mon, Mar 20, 2017 at 04:01:52PM +1000, Nicholas Piggin wrote:
> > > If not all threads were in winkle, full state loss recovery is not
> > > necessary and can be avoided. A previous patch removed this optimisation
> > > due to some complexity with the implementation. Re-implement it by
> > > counting the number of threads in winkle with the per-core idle state.
> > > Only restore full state loss if all threads were in winkle.
> > > 
> > > This has a small window of false positives right before threads execute
> > > winkle and just after they wake up, when the winkle count does not
> > > reflect the true number of threads in winkle. This is not a significant
> > > problem in comparison with even the minimum winkle duration. For
> > > correctness, a false positive is not a problem (only false negatives
> > > would be).  
> > 
> > The patch looks good. Just a minor comment.
> > 
> > 
> > >  BEGIN_FTR_SECTION
> > > +	/*
> > > +	 * Were we in winkle?
> > > +	 * If yes, check if all threads were in winkle, decrement our
> > > +	 * winkle count, set all thread winkle bits if all were in winkle.
> > > +	 * Check if our thread has a winkle bit set, and set cr4 accordingly
> > > +	 * (to match ISA300, above). Pseudo-code for core idle state
> > > +	 * transitions for ISA207 is as follows (everything happens atomically
> > > +	 * due to store conditional and/or lock bit):
> > > +	 *
> > > +	 * nap_idle() { }
> > > +	 * nap_wake() { }
> > > +	 *
> > > +	 * sleep_idle()
> > > +	 * {
> > > +	 *	core_idle_state &= ~thread_in_core
> > > +	 * }
> > > +	 *
> > > +	 * sleep_wake()
> > > +	 * {
> > > +	 *     bool first_in_core, first_in_subcore;
> > > +	 *
> > > +	 *     first_in_core = (core_idle_state & IDLE_THREAD_BITS) == 0;
> > > +	 *     first_in_subcore = (core_idle_state & SUBCORE_SIBLING_MASK) == 0;
> > > +	 *
> > > +	 *     core_idle_state |= thread_in_core;
> > > +	 * }
> > > +	 *
> > > +	 * winkle_idle()
> > > +	 * {
> > > +	 *	core_idle_state &= ~thread_in_core;
> > > +	 *	core_idle_state += 1 << WINKLE_COUNT_SHIFT;
> > > +	 * }
> > > +	 *
> > > +	 * winkle_wake()
> > > +	 * {
> > > +	 *     bool first_in_core, first_in_subcore, winkle_state_lost;
> > > +	 *
> > > +	 *     first_in_core = (core_idle_state & IDLE_THREAD_BITS) == 0;
> > > +	 *     first_in_subcore = (core_idle_state & SUBCORE_SIBLING_MASK) == 0;
> > > +	 *
> > > +	 *     core_idle_state |= thread_in_core;
> > > +	 *
> > > +	 *     if ((core_idle_state & WINKLE_MASK) == (8 << WINKLE_COUNT_SIHFT))
> > > +	 *         core_idle_state |= THREAD_WINKLE_BITS;  
> > 
> > We also do the following decrement. I forgot this in the pseudo-code in my
> > earlier reply.
> > 
> >   	 	core_idle_state -= 1 << WINKLE_COUNT_SHIFT;
> 
> Lucky somebody is paying attention. Yes, this is needed. I won't resend
> a patch if mpe can make the change.
> 
> 
> > Looks good other wise.
> > 
> > Reviewed-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>
> 
> Thank you.
> 
> What do you want to do about your DD1 fix? I think there are some minor
> clashes between them. I'm happy to rebase on top of yours if you prefer
> it to go in first.

I have sent an updated version of the DD1 fix today rebasing on
v4.11-rc3.

I applied your series on top of that and noticed some minor conflict
with patches 1,2,3 and 7.

If you are ok with it, I would like the DD1 Hotplug fixes to go in
first.

> 
> Thanks,
> Nick
>