| Message ID | 1414054881-17713-4-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Thu, 23 Oct 2014, Daniel Lezcano 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. s/falling/failing/ > > 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> Acked-by: Nicolas Pitre <nico@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; > + > /* Then try to determine standard deviation */ > stddev = 0; > for (i = 0; i < INTERVALS; i++) { > -- > 1.9.1 > >
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; thanks, Len Brown, Intel Open Source Technology Center
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
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; + /* Then try to determine standard deviation */ stddev = 0; for (i = 0; i < INTERVALS; i++) {
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(+)