Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 10/28/2015 11:46 PM, riel@redhat.com wrote:
> From: Rik van Riel <riel@redhat.com>
>
> The cpuidle menu governor has a forced cut-off for polling at 5us,
> in order to deal with firmware that gives the OS bad information
> on cpuidle states, leading to the system spending way too much time
> in polling.

May be I am misunderstanding your explanation but it is not how I read 
the code.

The default idle state is C1 (hlt) if no other states suits the 
constraint. If a timer is happening really soon, then set the default 
idle state to POLL if no other idle state suits the constraint.

That applies only on x86.

This is not related to break-even but exit latency.

IMO, we should just drop this 5us and the POLL state selection in the 
menu governor as we have since a while hyper fast C1 exit. Except a few 
embedded processors where polling is not adequate.

Furthermore, the number of times the poll state is selected vs the other 
states is negligible.

I already raised this point but Len is opposed to the removal.

Len ? Can you elaborate why you are opposed to this removal ?

> However, at least one x86 CPU family (Atom) has chips that have
> a 20us break-even point for C1. Forcing the polling cut-off to
> less than that wastes performance and power.
>
> Increase the polling cut-off to 20us.
>
> Systems with a lower C1 latency will be found in the states table by
> the menu governor, which will pick those states as appropriate.

With this change, I believe the poll state will be selected more often 
(not too much certainly), hence implying a bigger energy consumption.

And finally it may improve the situation for specific processor but 
deteriorate for other processors.

Does anyone have the rational behind this '5' number ? why not '3' or '7' ?

> Signed-off-by: Rik van Riel <riel@redhat.com>
> ---
>   drivers/cpuidle/governors/menu.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
> index 22e4463d1787..ecc242a586c9 100644
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -330,7 +330,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>   	 * We want to default to C1 (hlt), not to busy polling
>   	 * unless the timer is happening really really soon.
>   	 */
> -	if (data->next_timer_us > 5 &&
> +	if (data->next_timer_us > 20 &&
>   	    !drv->states[CPUIDLE_DRIVER_STATE_START].disabled &&
>   		dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0)
>   		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
>


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog