Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1258472 > unrolled thread

[PATCH 0/3] cpuidle: small improvements & fixes for menu governor

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] cpuidle: small improvements & fixes for menu governor riel@redhat.com - 2015-10-29 00:10 +0100
    [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us riel@redhat.com - 2015-10-29 00:10 +0100
      Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to  20us Daniel Lezcano <daniel.lezcano@linaro.org> - 2015-10-29 11:20 +0100
        Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling  to 20us Rik van Riel <riel@redhat.com> - 2015-10-29 13:00 +0100
          Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to  20us Daniel Lezcano <daniel.lezcano@linaro.org> - 2015-10-29 14:10 +0100

#1258472 — [PATCH 0/3] cpuidle: small improvements & fixes for menu governor

While working on a paravirt cpuidle driver for KVM guests, I
noticed a number of small logic errors in the menu governor
code.

These patches should get rid of some artifacts that can break
the logic in the menu governor under certain corner cases, and
make idle state selection work better on CPUs with long C1 exit
latencies.

I have not seen any adverse effects with them in my (quick)
tests. As expected, they do not seem to do much on systems with
many power states and very low C1 exit latencies and target residencies.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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.

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.

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;
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On 10/29/2015 06:17 AM, Daniel Lezcano wrote:
> 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.

With the current code, the default idle state is C1 (hlt) even if
C1 does not suit the constraint.

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

Why would we not care about break-even for C1?

On systems where going into C1 for too-short periods wastes
power, why would we waste the power when we expect a very
short sleep?

> 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.

We have hyper fast C1 exit on Nehalem and newer high performance
chips. On those chips, we will pick C1 (or deeper) when we have
an expected sleep time of just a few microseconds.

However, on Atom, and for the paravirt cpuidle driver I am
working on, C1 exit latency and target residence are higher
than the cut-off hardcoded in the menu governor.

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

And it will continue to be with this patch, on CPUs with
hyper fast C1 exit.

Which makes me confused about what your are objecting to,
since the system should continue to be have the way you want,
with the patch applied.

-- 
All rights reversed
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On 10/29/2015 12:54 PM, Rik van Riel wrote:
> On 10/29/2015 06:17 AM, Daniel Lezcano wrote:
>> 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.
>
> With the current code, the default idle state is C1 (hlt) even if
> C1 does not suit the constraint.
>
>> This is not related to break-even but exit latency.
>
> Why would we not care about break-even for C1?
>
> On systems where going into C1 for too-short periods wastes
> power, why would we waste the power when we expect a very
> short sleep?
>
>> 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.
>
> We have hyper fast C1 exit on Nehalem and newer high performance
> chips. On those chips, we will pick C1 (or deeper) when we have
> an expected sleep time of just a few microseconds.
>
> However, on Atom, and for the paravirt cpuidle driver I am
> working on, C1 exit latency and target residence are higher
> than the cut-off hardcoded in the menu governor.
>
>> Furthermore, the number of times the poll state is selected vs the other
>> states is negligible.
>
> And it will continue to be with this patch, on CPUs with
> hyper fast C1 exit.
>
> Which makes me confused about what your are objecting to,
> since the system should continue to be have the way you want,
> with the patch applied.

Ok, I don't object the correctness of your patch but the reasoning 
behind this small optimization which bring us a lot of mess in the 
cpuidle code.

As you are touching this part of the code, I take the opportunity to 
raise a discussion about it.

 From my POV, the poll state is *not* an idle state. It is like a 
vehicle burnout [1].

But it is inserted into the idle state tables using a trick with a macro 
CPUIDLE_DRIVER_STATE_START which already led us to some bugs.

So instead of falling back into the poll state under certain 
circumstances, I propose we extract this state from the idle state table 
and we let the menu governor to fail choosing a state (or not).

 From the caller, we decide what to do (poll or C1) if the idle state 
selection fails or we choose to poll *before* like what we already have 
in kernel/sched/idle.c:

in the idle loop:

if (cpu_idle_force_poll || tick_check_broadcast_expired())
	cpu_idle_poll();
else
	cpuidle_idle_call();

By this way, we:

1) factor out the idle state selection with the find_deepest_idle_state
2) remove the CPUIDLE_DRIVER_STATE_START macro
3) concentrate the optimization logic outside of a governor which will 
benefit to all architectures

Does it make sense ?

   -- Daniel

[1] https://en.wikipedia.org/wiki/Burnout_%28vehicle%29


-- 
  <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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web