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

[Len Brown]

On Thu, Oct 23, 2014 at 5:01 AM, Daniel Lezcano
<daniel.lezcano@xxxxxxxxxx> 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@xxxxxxxxxx>
> ---
>  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;

thanks,
Len Brown, Intel Open Source Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/