Messages in this thread |  | | Date | Wed, 29 Oct 2014 19:15:34 +0100 | From | Daniel Lezcano <> | 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@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/
|  |