Re: [RFC] cpuidle: menu: nearby timer use lightest state; allow state 0 to be disabled

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

On Tue, Jun 06, 2017 at 11:15:55AM +1000, Nicholas Piggin wrote:
> > 
> > Are you able to observe any functional difference with this patch ?
> 
> Yes for some tests, but it did have a bug where state0 was still
> being used despite other states being available. This patch really
> disables state0 (unless all states are disabled, then it falls back
> to 0 again).

Nice. I agree with this approach. We need to document that if the
lower states are disabled, then the menu governor selection logic
would promote to a higher cpuidle state even when the predicted
residency is small. So,folks who are worried about latency shouldn't
be disabling the lower states in the first place.

Acked-by: Gautham R. Shenoy <ego@linux.vnet.ibm.com>

> 
> ---
>  drivers/cpuidle/governors/menu.c | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index b2330fd69e34..61b64c2b2cb8 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -286,6 +286,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	struct device *device = get_cpu_device(dev->cpu);
>  	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
>  	int i;
> +	int first_idx;
> +	int idx;
>  	unsigned int interactivity_req;
>  	unsigned int expected_interval;
>  	unsigned long nr_iowaiters, cpu_load;
> @@ -335,11 +337,11 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  		if (data->next_timer_us > polling_threshold &&
>  		    latency_req > s->exit_latency && !s->disabled &&
>  		    !dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable)
> -			data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
> +			first_idx = CPUIDLE_DRIVER_STATE_START;
>  		else
> -			data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
> +			first_idx = CPUIDLE_DRIVER_STATE_START - 1;
>  	} else {
> -		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
> +		first_idx = 0;
>  	}
> 
>  	/*
> @@ -359,20 +361,28 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	 * Find the idle state with the lowest power while satisfying
>  	 * our constraints.
>  	 */
> -	for (i = data->last_state_idx + 1; i < drv->state_count; i++) {
> +	idx = -1;
> +	for (i = first_idx; i < drv->state_count; i++) {
>  		struct cpuidle_state *s = &drv->states[i];
>  		struct cpuidle_state_usage *su = &dev->states_usage[i];
> 
>  		if (s->disabled || su->disable)
>  			continue;
> +		if (idx == -1)
> +			idx = i; /* first enabled state */
>  		if (s->target_residency > data->predicted_us)
>  			break;
>  		if (s->exit_latency > latency_req)
>  			break;
> 
> -		data->last_state_idx = i;
> +		idx = i;
>  	}
> 
> +	if (idx == -1)
> +		idx = 0; /* No states enabled. Must use 0. */
> +
> +	data->last_state_idx = idx;
> +
>  	return data->last_state_idx;
>  }
> 
> -- 
>