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

Message ID 3144686.I8R4d9A2JO@kreacher
State In Next
Commit e4c9f53aab305f67bb7714164e833dfbe8bffb84
Headers show
Series
  • cpuidle: teo: Bug fix, formal correction and cleanup
Related show

Commit Message

Rafael J. Wysocki Nov. 13, 2019, 12:07 a.m. UTC
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(-)

Comments

Rafael J. Wysocki Nov. 14, 2019, 11:50 p.m. UTC | #1
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!
Doug Smythies Nov. 15, 2019, 1:24 a.m. UTC | #2
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
Rafael J. Wysocki Nov. 15, 2019, 9:07 a.m. UTC | #3
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!

Patch
diff mbox series

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;
 		}
 	}