Re: [PATCH v3] cpuidle: poll_state: Add time limit to poll_idle()

[Rik van Riel]

On Thu, 2018-03-22 at 18:09 +0100, Rafael J. Wysocki wrote:
> On Thursday, March 22, 2018 5:32:23 PM CET Rik van Riel wrote:
> > On Wed, 2018-03-14 at 15:08 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > > 
> > > If poll_idle() is allowed to spin until need_resched() returns
> > > 'true',
> > > it may actually spin for a much longer time than expected by the
> > > idle
> > > governor, since set_tsk_need_resched() is not always called by
> > > the
> > > timer interrupt handler.  If that happens, the CPU may spend much
> > > more time than anticipated in the "polling" state.
> > > 
> > > To prevent that from happening, limit the time of the spinning
> > > loop
> > > in poll_idle().
> > > 
> > > Suggested-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
> > 
> > So ... about bisecting that other patch series...
> > 
> > It turned out I had this patch, which looks so
> > obviously correct, as patch #1 in my series.
> > 
> > It also turned out that this patch is responsible
> > for the entire 5-10% increase in CPU use for the
> > memcache style workload.
> > 
> > I wonder if keeping an idle HT thread much busier
> > than before slows down its sibling, or something
> > like that.
> 
> Uhm, sorry about this.

No worries, this is why we do patch reviews and
tests in the first place.

> Does it improve if you do something like the below on top of it?

That was my next thing to try, after testing just
the idle nohz series by itself :)

I'll push both into the test systems, and will
get back to you when I have answers.

> > Let me go test the nohz idle series by itself,
> > without this patch.
> 
> OK
> 
> ---
>  drivers/cpuidle/poll_state.c |    6 ++++++
>  1 file changed, 6 insertions(+)
> 
> Index: linux-pm/drivers/cpuidle/poll_state.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/poll_state.c
> +++ linux-pm/drivers/cpuidle/poll_state.c
> @@ -10,6 +10,7 @@
>  #include <linux/sched/idle.h>
>  
>  #define POLL_IDLE_TIME_LIMIT	(TICK_NSEC / 16)
> +#define POLL_IDLE_COUNT		1000
>  
>  static int __cpuidle poll_idle(struct cpuidle_device *dev,
>  			       struct cpuidle_driver *drv, int
> index)
> @@ -18,9 +19,14 @@ static int __cpuidle poll_idle(struct cp
>  
>  	local_irq_enable();
>  	if (!current_set_polling_and_test()) {
> +		unsigned int loop_count = 0;
> +
>  		while (!need_resched()) {
>  			cpu_relax();
> +			if (loop_count++ < POLL_IDLE_COUNT)
> +				continue;
>  
> +			loop_count = 0;
>  			if (local_clock() - time_start >
> POLL_IDLE_TIME_LIMIT)
>  				break;
>  		}
> 
> 
-- 
All Rights Reversed.

Attachment: signature.asc
Description: This is a digitally signed message part