[2/3] cpuidle: teo: Avoid expecting unrealistic idle times

1152763 diff mbox series

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

If an idle state shallower than the one "matching" the time till the
next timer event is considered for selection, expect the idle duration
to fall in the middle of the "bin" corresponding to that state rather
than at the beginning of it which is unrealistic.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/teo.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

On Wed, Nov 13, 2019 at 1:11 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If an idle state shallower than the one "matching" the time till the
> next timer event is considered for selection, expect the idle duration
> to fall in the middle of the "bin" corresponding to that state rather
> than at the beginning of it which is unrealistic.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/governors/teo.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/cpuidle/governors/teo.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/teo.c
> +++ linux-pm/drivers/cpuidle/governors/teo.c
> @@ -360,7 +360,14 @@ static int teo_select(struct cpuidle_dri
>
>                 if (max_early_idx >= 0) {
>                         idx = max_early_idx;
> -                       duration_ns = drv->states[idx].target_residency_ns;
> +                       /*
> +                        * Expect the idle duration to fall in the middle of the
> +                        * "bin" corresponding to idx (note that the maximum
> +                        * state index is guaranteed to be greater than idx at
> +                        * this point).
> +                        */
> +                       duration_ns = (drv->states[idx].target_residency_ns +
> +                               drv->states[idx+1].target_residency_ns) / 2;
>                 }
>         }

This change turns out to cause the governor to choose idle states that
are too deep or too shallow too often, so I'm withdrawing it.

Thanks!

On 2019.11.14 15:51 Rafael J. Wysocki wrote:
> On Wed, Nov 13, 2019 at 1:11 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>
>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> If an idle state shallower than the one "matching" the time till the
>> next timer event is considered for selection, expect the idle duration
>> to fall in the middle of the "bin" corresponding to that state rather
>> than at the beginning of it which is unrealistic.
>>
>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  drivers/cpuidle/governors/teo.c |    9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> Index: linux-pm/drivers/cpuidle/governors/teo.c
>> ===================================================================
>> --- linux-pm.orig/drivers/cpuidle/governors/teo.c
>> +++ linux-pm/drivers/cpuidle/governors/teo.c
>> @@ -360,7 +360,14 @@ static int teo_select(struct cpuidle_dri
>>
>>                 if (max_early_idx >= 0) {
>>                         idx = max_early_idx;
>> -                       duration_ns = drv->states[idx].target_residency_ns;
>> +                       /*
>> +                        * Expect the idle duration to fall in the middle of the
>> +                        * "bin" corresponding to idx (note that the maximum
>> +                        * state index is guaranteed to be greater than idx at
>> +                        * this point).
>> +                        */
>> +                       duration_ns = (drv->states[idx].target_residency_ns +
>> +                               drv->states[idx+1].target_residency_ns) / 2;
>>                 }
>>         }
>
> This change turns out to cause the governor to choose idle states that
> are too deep or too shallow too often, so I'm withdrawing it.

O.K. thanks for letting us know.
I did see some differences in the testing I did so far, but hadn't drilled down
into it yet.
I am somewhat wondering about the above and below stats in general.

By the way, I had a daft mistake in my post processing program, such that the
"below" graph for idle state 0 was always plotting 0.

Reference for that sweep test that I do (which is as far I got so far):
http://www.smythies.com/~doug/linux/idle/teo-2019-11/sweep/index.html

Legend:

teo-v2: re-run of previous teo-v2 so that I could get non-zero idle state "below" data
linux-next 2019.11.07 + cpuidle: Consolidate disabled state checks +
[PATCH v2] cpuidle: Use nanoseconds as the unit of time

teo-v3: teo-v2 + cpuidle: teo: Exclude cpuidle overhead from computations

teo-v4: linux-pm + linux-next 2019.11.12 +
cpuidle: teo: Avoid code duplication in conditionals
cpuidle: teo: Avoid expecting unrealistic idle times
cpuidle: teo: Avoid using "early hits" incorrectly

teo-v5: teo-v4 + cpuidle: teo: Exclude cpuidle overhead from computations

... Doug

On Fri, Nov 15, 2019 at 2:24 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2019.11.14 15:51 Rafael J. Wysocki wrote:
> > On Wed, Nov 13, 2019 at 1:11 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> If an idle state shallower than the one "matching" the time till the
> >> next timer event is considered for selection, expect the idle duration
> >> to fall in the middle of the "bin" corresponding to that state rather
> >> than at the beginning of it which is unrealistic.
> >>
> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>  drivers/cpuidle/governors/teo.c |    9 ++++++++-
> >>  1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> Index: linux-pm/drivers/cpuidle/governors/teo.c
> >> ===================================================================
> >> --- linux-pm.orig/drivers/cpuidle/governors/teo.c
> >> +++ linux-pm/drivers/cpuidle/governors/teo.c
> >> @@ -360,7 +360,14 @@ static int teo_select(struct cpuidle_dri
> >>
> >>                 if (max_early_idx >= 0) {
> >>                         idx = max_early_idx;
> >> -                       duration_ns = drv->states[idx].target_residency_ns;
> >> +                       /*
> >> +                        * Expect the idle duration to fall in the middle of the
> >> +                        * "bin" corresponding to idx (note that the maximum
> >> +                        * state index is guaranteed to be greater than idx at
> >> +                        * this point).
> >> +                        */
> >> +                       duration_ns = (drv->states[idx].target_residency_ns +
> >> +                               drv->states[idx+1].target_residency_ns) / 2;
> >>                 }
> >>         }
> >
> > This change turns out to cause the governor to choose idle states that
> > are too deep or too shallow too often, so I'm withdrawing it.
>
> O.K. thanks for letting us know.
> I did see some differences in the testing I did so far, but hadn't drilled down
> into it yet.
> I am somewhat wondering about the above and below stats in general.
>
> By the way, I had a daft mistake in my post processing program, such that the
> "below" graph for idle state 0 was always plotting 0.
>
> Reference for that sweep test that I do (which is as far I got so far):
> http://www.smythies.com/~doug/linux/idle/teo-2019-11/sweep/index.html
>
> Legend:
>
> teo-v2: re-run of previous teo-v2 so that I could get non-zero idle state "below" data
> linux-next 2019.11.07 + cpuidle: Consolidate disabled state checks +
> [PATCH v2] cpuidle: Use nanoseconds as the unit of time
>
> teo-v3: teo-v2 + cpuidle: teo: Exclude cpuidle overhead from computations
>
> teo-v4: linux-pm + linux-next 2019.11.12 +
> cpuidle: teo: Avoid code duplication in conditionals
> cpuidle: teo: Avoid expecting unrealistic idle times
> cpuidle: teo: Avoid using "early hits" incorrectly
>
> teo-v5: teo-v4 + cpuidle: teo: Exclude cpuidle overhead from computations

Thanks for running the tests, appreciated!