Re: [PATCH 12/14] powerpc/64s: cpuidle no memory barrier after break from idle

[Nicholas Piggin <npiggin@gmail.com>]

On Mon, 12 Jun 2017 23:18:44 +0530
Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com> wrote:

> * Nicholas Piggin <npiggin@gmail.com> [2017-06-12 09:58:33]:
> 
> > A memory barrier is not required after the task wakes up,
> > only if we clear the polling flag before waking. The case
> > where we have work to do is the important one, so optimise
> > for it.
> > 
> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com>  
> 
> Reviewed-by: Vaidyanathan Srinivasan <svaidy@linux.vnet.ibm.com>
> 
> 
> > ---
> >  drivers/cpuidle/cpuidle-powernv.c | 11 +++++++++--
> >  drivers/cpuidle/cpuidle-pseries.c | 11 +++++++++--
> >  2 files changed, 18 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/cpuidle/cpuidle-powernv.c b/drivers/cpuidle/cpuidle-powernv.c
> > index 9d03326ac05e..37b0698b7193 100644
> > --- a/drivers/cpuidle/cpuidle-powernv.c
> > +++ b/drivers/cpuidle/cpuidle-powernv.c
> > @@ -59,14 +59,21 @@ static int snooze_loop(struct cpuidle_device *dev,
> >  	ppc64_runlatch_off();
> >  	HMT_very_low();
> >  	while (!need_resched()) {
> > -		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time)
> > +		if (likely(snooze_timeout_en) && get_tb() > snooze_exit_time) {
> > +			/*
> > +			 * Task has not woken up but we are exiting the polling
> > +			 * loop anyway. Require a barrier after polling is
> > +			 * cleared to order subsequent test of need_resched().
> > +			 */
> > +			clear_thread_flag(TIF_POLLING_NRFLAG);
> > +			smp_mb();
> >  			break;
> > +		}
> >  	}
> > 
> >  	HMT_medium();
> >  	ppc64_runlatch_on();
> >  	clear_thread_flag(TIF_POLLING_NRFLAG);
> > -	smp_mb();  
> 
> If we reach here without executing if(snooze_timeout) with the
> barrier+break, that means we have seen the need_resched flag and hence
> we can avoid the barrier.  Clearing of the polling flag can be seen
> (take affect) later as we will exit the idle loop and re-enter anyway
> at this point.  The caller also will check for need_resched and exit
> the idle loop.
> 
> Actually do_idle() has a __current_set_polling() and
> __current_clr_polling() in the default idle loop.  Do we really need
> to set/clear the TIF_POLLING_NRFLAG again in cpuidle driver?

We do because cpuidle drivers are entered with POLLING clear, don't
they?

It would be nice to have the entire scheduler idle including cpuidle
entry run with POLLING set. It would then be up to interrupt-based
cpuidle drivers to clear POLLING before sleeping. I think that should
remove... maybe 4 atomic ops from the most performance-critical idle
states (the polling ones).

That's a bigger change though and will touch many archs and core code.

Thanks,
Nick