linux-kernel - Re: [PATCH V2 4/5] cpuidle: menu: Fix the get_typical

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 29 Oct 2014 19:15:34 +0100
From:	Daniel Lezcano <daniel.lezcano@...aro.org>
To:	Len Brown <lenb@...nel.org>
CC:	"Rafael J. Wysocki" <rjw@...ysocki.net>,
	Nicolas Pitre <nicolas.pitre@...aro.org>,
	Linux PM list <linux-pm@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Peter Zijlstra <peterz@...radead.org>,
	linaro-kernel@...ts.linaro.org, Patch Tracking <patches@...aro.org>
Subject: Re: [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@...aro.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@...aro.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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/