Re: [PATCH V2 4/5] cpuidle: menu: Fix the get_typical_interval

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

On 10/28/2014 03:48 AM, Len Brown wrote:
> On Thu, Oct 23, 2014 at 5:01 AM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> The first time the 'get_typical_function' is called, it computes an average
>> of zero as no data is filled yet. That leads the 'data->predicted_us' variable
>> to be set to zero too.
>>
>> The caller, 'menu_select' will then do:
>>
>>          interactivity_req = data->predicted_us /
>>                          performance_multiplier(nr_iowaiters, cpu_load);
>>
>> That sets the interactivity_req to zero (0/performance...).
>>
>> and then
>>
>>          if (latency_req > interactivity_req)
>>                  latency_req = interactivity_req;
>>
>> ... setting 'latency_req' to zero too.
>>
>> No idle state will fulfill this constraint and we will go the C1 state as
>> default and leading to an update. So the next calls will compute an average
>> different from zero.
>>
>> Even if that works with the current code but with a broken semantic, it will
>> just break with the next patches where we are stricter with the latencies
>> check: the first check will fail (latency_req is zero), then no update will
>> occur leading to always falling to choose an idle state.
>>
>> As there are no previous values and it is pointless to compute a standard
>> deviation for these unexisting values. Just return without setting the
>> 'data->predicted_us' to zero.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   drivers/cpuidle/governors/menu.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
>> index 3907301..6ae8390 100644
>> --- a/drivers/cpuidle/governors/menu.c
>> +++ b/drivers/cpuidle/governors/menu.c
>> @@ -226,6 +226,15 @@ again:
>>          else
>>                  do_div(avg, divisor);
>>
>> +       /*
>> +        * We are at the very beginning and no data have been filled
>> +        * yet. Let's skip the standard deviation computation
>> +        * otherwise the data->predicted_us will be zero and that will
>> +        * lead to a zero latency req in the select function
>> +        */
>> +       if (!avg)
>> +               return;
>> +
>
> Unfortunately, you've touched ugly code,
> and your (correct) patch makes it ever-so slightly more ugly,
> instead of more clear.
>
> I think the code would read more clearly, and your patch would
> less obscure, if the code read something like this sow that it is
> clear at the menu_select level when and where we monkey
> with predicted_us:
>
> menu_select()...
> ...
> data->predicted_us = div_round64(bla bla bla
>
> interactivity_overrride_us = get_typical_interval(data);
>
> if (interactivity_override_us)
>    if (interactivity_predicted_us < data->predicted_us)
>          data->predicted_us = interactivity_override_us;
>
> And, of course, down inside get_typical_interval()
> ...
> if (!avg)
>          return 0;
> ...
> if (likely(stddev <= ULONG_MAX)) {
> ...
>          return avg;


Ok, thanks for the suggestion. I will look at reworking this patch.

   -- Daniel


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