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

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

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