lkml.org 
[lkml]   [2014]   [Oct]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH V2 4/5] cpuidle: menu: Fix the get_typical_interval
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

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

\
 
 \ /
  Last update: 2014-10-29 19:41    [W:0.084 / U:1.156 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site