Hi Folks,
This is the second iteration of:
https://lore.kernel.org/linux-pm/4511619.LvFx2qVVIh@kreacher/
with an additional patch.
There are some small modifications of patch [1/3] and the new
patch causes governor statistics to play a role in deciding whether
or not to stop the scheduler tick.
Testing would be much appreciated!
Thanks!
From: Rafael J. Wysocki <[email protected]>
Currently, if the target residency of the deepest idle state is less than
the tick period length, which is quite likely for HZ=100, and the deepest
idle state is about to be selected by the TEO idle governor, the decision
on whether or not to stop the scheduler tick is based entirely on the
time till the closest timer. This is often insufficient, because timers
may not be in heavy use and there may be a plenty of other CPU wakeup
events between the deepest idle state's target residency and the closest
tick.
Allow the governor to count those events by making the deepest idle
state's bin effectively end at TICK_NSEC and introducing an additional
"bin" for collecting "hit" events (ie. the ones in which the measured
idle duration falls into the same bin as the time till the closest
timer) with idle duration values past TICK_NSEC.
This way the "intercepts" metric for the deepest idle state's bin
becomes nonzero in general, and so it can influence the decision on
whether or not to stop the tick possibly increasing the governor's
accuracy in that respect.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/cpuidle/governors/teo.c | 41 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 40 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
@@ -192,6 +192,7 @@ struct teo_bin {
* @total: Grand total of the "intercepts" and "hits" metrics for all bins.
* @next_recent_idx: Index of the next @recent_idx entry to update.
* @recent_idx: Indices of bins corresponding to recent "intercepts".
+ * @tick_hits: Number of "hits" after TICK_NSEC.
* @util_threshold: Threshold above which the CPU is considered utilized
*/
struct teo_cpu {
@@ -201,6 +202,7 @@ struct teo_cpu {
unsigned int total;
int next_recent_idx;
int recent_idx[NR_RECENT];
+ unsigned int tick_hits;
unsigned long util_threshold;
};
@@ -232,6 +234,7 @@ static void teo_update(struct cpuidle_dr
{
struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
int i, idx_timer = 0, idx_duration = 0;
+ s64 target_residency_ns;
u64 measured_ns;
if (cpu_data->time_span_ns >= cpu_data->sleep_length_ns) {
@@ -272,7 +275,6 @@ static void teo_update(struct cpuidle_dr
* fall into.
*/
for (i = 0; i < drv->state_count; i++) {
- s64 target_residency_ns = drv->states[i].target_residency_ns;
struct teo_bin *bin = &cpu_data->state_bins[i];
bin->hits -= bin->hits >> DECAY_SHIFT;
@@ -280,6 +282,8 @@ static void teo_update(struct cpuidle_dr
cpu_data->total += bin->hits + bin->intercepts;
+ target_residency_ns = drv->states[i].target_residency_ns;
+
if (target_residency_ns <= cpu_data->sleep_length_ns) {
idx_timer = i;
if (target_residency_ns <= measured_ns)
@@ -295,6 +299,26 @@ static void teo_update(struct cpuidle_dr
cpu_data->state_bins[cpu_data->recent_idx[i]].recent--;
/*
+ * If the deepest state's target residency is below the tick length,
+ * make a record of it to help teo_select() decide whether or not
+ * to stop the tick. This effectively adds an extra hits-only bin
+ * beyond the last state-related one.
+ */
+ if (target_residency_ns < TICK_NSEC) {
+ cpu_data->tick_hits -= cpu_data->tick_hits >> DECAY_SHIFT;
+
+ cpu_data->total += cpu_data->tick_hits;
+
+ if (TICK_NSEC <= cpu_data->sleep_length_ns) {
+ idx_timer = drv->state_count;
+ if (TICK_NSEC <= measured_ns) {
+ cpu_data->tick_hits += PULSE;
+ goto end;
+ }
+ }
+ }
+
+ /*
* If the measured idle duration falls into the same bin as the sleep
* length, this is a "hit", so update the "hits" metric for that bin.
* Otherwise, update the "intercepts" metric for the bin fallen into by
@@ -309,6 +333,7 @@ static void teo_update(struct cpuidle_dr
cpu_data->recent_idx[i] = idx_duration;
}
+end:
cpu_data->total += PULSE;
}
@@ -356,6 +381,7 @@ static int teo_select(struct cpuidle_dri
struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
s64 latency_req = cpuidle_governor_latency_req(dev->cpu);
ktime_t delta_tick = TICK_NSEC / 2;
+ unsigned int tick_intercept_sum = 0;
unsigned int idx_intercept_sum = 0;
unsigned int intercept_sum = 0;
unsigned int idx_recent_sum = 0;
@@ -429,6 +455,8 @@ static int teo_select(struct cpuidle_dri
hit_sum += prev_bin->hits;
recent_sum += prev_bin->recent;
+ tick_intercept_sum = intercept_sum;
+
if (dev->states_usage[i].disable)
continue;
@@ -461,6 +489,8 @@ static int teo_select(struct cpuidle_dri
goto end;
}
+ tick_intercept_sum += cpu_data->state_bins[drv->state_count-1].intercepts;
+
/*
* If the sum of the intercepts metric for all of the idle states
* shallower than the current candidate one (idx) is greater than the
@@ -577,6 +607,15 @@ static int teo_select(struct cpuidle_dri
idx = i;
}
+ /*
+ * If the selected state's target residency is below the tick length
+ * and intercepts occurring before the tick length are the majority of
+ * total wakeup events, do not stop the tick.
+ */
+ if (drv->states[idx].target_residency_ns < TICK_NSEC &&
+ tick_intercept_sum > cpu_data->total / 2 + cpu_data->total / 8)
+ duration_ns = TICK_NSEC / 2;
+
end:
/*
* Allow the tick to be stopped unless the selected state is a polling
Hi Rafael,
On Thu, Aug 03, 2023 at 10:57:04PM +0200, Rafael J. Wysocki wrote:
> Hi Folks,
>
> This is the second iteration of:
>
> https://lore.kernel.org/linux-pm/4511619.LvFx2qVVIh@kreacher/
>
> with an additional patch.
>
> There are some small modifications of patch [1/3] and the new
> patch causes governor statistics to play a role in deciding whether
> or not to stop the scheduler tick.
>
> Testing would be much appreciated!
>
> Thanks!
>
My test results including the v2 are below.
1. Geekbench 6
+---------------------------+---------------+-----------------+-------------------+----------------------+
| metric | teo | teo_tick | teo_tick_rfc | teo_tick_rfc_v2 |
+---------------------------+---------------+-----------------+-------------------+----------------------+
| multicore_score | 3320.9 (0.0%) | 3303.3 (-0.53%) | 3293.6 (-0.82%) | 3302.3 (-0.56%) |
| score | 1415.7 (0.0%) | 1417.7 (0.14%) | 1423.4 (0.54%) | 1425.8 (0.71%) |
| CPU_total_power | 2421.3 (0.0%) | 2429.3 (0.33%) | 2442.2 (0.86%) | 2461.9 (1.67%) |
| latency (AsyncTask #1) | 49.41μ (0.0%) | 51.07μ (3.36%) | 50.1μ (1.4%) | 50.76μ (2.73%) |
| latency (labs.geekbench6) | 65.63μ (0.0%) | 77.47μ (18.03%) | 55.82μ (-14.95%) | 66.12μ (0.75%) |
| latency (surfaceflinger) | 39.46μ (0.0%) | 36.94μ (-6.39%) | 35.79μ (-9.28%) | 40.36μ (2.3%) |
+---------------------------+---------------+-----------------+-------------------+----------------------+
+----------------------+-------------+------------+
| tag | type | count_perc |
+----------------------+-------------+------------+
| teo | too deep | 2.034 |
| teo_tick | too deep | 2.16 |
| teo_tick_rfc | too deep | 2.071 |
| teo_tick_rfc_v2 | too deep | 2.548 |
| teo | too shallow | 15.791 |
| teo_tick | too shallow | 20.881 |
| teo_tick_rfc | too shallow | 20.337 |
| teo_tick_rfc_v2 | too shallow | 19.886 |
+----------------------+-------------+------------+
2. JetNews
+-----------------+---------------+----------------+-----------------+-----------------+
| metric | teo | teo_tick | teo_tick_rfc | teo_tick_rfc_v2 |
+-----------------+---------------+----------------+-----------------+-----------------+
| fps | 86.2 (0.0%) | 86.4 (0.16%) | 86.0 (-0.28%) | 86.6 (0.41%) |
| janks_pc | 0.8 (0.0%) | 0.8 (-0.66%) | 0.8 (-1.37%) | 0.7 (-11.37%) |
| CPU_total_power | 185.2 (0.0%) | 178.2 (-3.76%) | 182.2 (-1.6%) | 169.4 (-8.53%) | <- very interesting
+-----------------+---------------+----------------+-----------------+-----------------+
+----------------------+-------------+--------------------+
| tag | type | count_perc |
+----------------------+-------------+--------------------+
| teo | too deep | 0.992 |
| teo_tick | too deep | 0.945 |
| teo_tick_rfc | too deep | 1.035 |
| teo_tick_rfc_v2 | too deep | 1.127 |
| teo | too shallow | 17.085 |
| teo_tick | too shallow | 15.236 |
| teo_tick_rfc | too shallow | 15.379 |
| teo_tick_rfc_v2 | too shallow | 15.34 |
+----------------------+-------------+--------------------+
All in all looks pretty good. Unfortunately there's a slightly larger
percentage of too deep sleeps with the v2 (which is probably where the
increase in GB6 power usage comes from) but the lower jank percentage +
substantially lower power usage for the UI workload are very promising.
Since we don't care about GB6 power usage as much as UI power usage, I'd
say that the patchset looks good :)
Tested-by: Kajetan Puchalski <[email protected]>
On Mon, 7 Aug 2023, Anna-Maria Behnsen wrote:
> On Thu, 3 Aug 2023, Rafael J. Wysocki wrote:
>
> > On Thu, Aug 3, 2023 at 11:12 PM Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > Hi Folks,
> > >
> > > This is the second iteration of:
> > >
> > > https://lore.kernel.org/linux-pm/4511619.LvFx2qVVIh@kreacher/
> > >
> > > with an additional patch.
> > >
> > > There are some small modifications of patch [1/3] and the new
> > > patch causes governor statistics to play a role in deciding whether
> > > or not to stop the scheduler tick.
> > >
> > > Testing would be much appreciated!
> >
> > For convenience, this series is now available in the following git branch:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > pm-cpuidle-teo
> >
>
> Gauthams tests and the distribution of idle time durations looks pretty
> good. Also the prevention of calling tick_nohz_get_sleep_length() is very
> nice (21477 calls of tick_nohz_next_event() and the tick was stopped 2670
> times).
>
> Here is the deviation of idle time durations (based on your branch):
Sorry, s/deviation/distribution
> Idle Total 2670 100.00%
> x >= 4ms 2537 95.02%
> 4ms> x >= 2ms 19 0.71%
> 2ms > x >= 1ms 10 0.37%
> 1ms > x >= 500us 7 0.26%
> 500us > x >= 250us 6 0.22%
> 250us > x >=100us 13 0.49%
> 100us > x >= 50us 17 0.64%
> 50us > x >= 25us 25 0.94%
> 25us > x >= 10us 22 0.82%
> 10us > x > 5us 9 0.34%
> 5us > x 5 0.19%
>
>
> Thanks,
>
> Anna-Maria
Hi Kajetan,
On Mon, Aug 7, 2023 at 4:04 PM Kajetan Puchalski
<[email protected]> wrote:
>
> Hi Rafael,
>
> On Thu, Aug 03, 2023 at 10:57:04PM +0200, Rafael J. Wysocki wrote:
> > Hi Folks,
> >
> > This is the second iteration of:
> >
> > https://lore.kernel.org/linux-pm/4511619.LvFx2qVVIh@kreacher/
> >
> > with an additional patch.
> >
> > There are some small modifications of patch [1/3] and the new
> > patch causes governor statistics to play a role in deciding whether
> > or not to stop the scheduler tick.
> >
> > Testing would be much appreciated!
> >
> > Thanks!
> >
>
> My test results including the v2 are below.
>
> 1. Geekbench 6
>
> +---------------------------+---------------+-----------------+-------------------+----------------------+
> | metric | teo | teo_tick | teo_tick_rfc | teo_tick_rfc_v2 |
> +---------------------------+---------------+-----------------+-------------------+----------------------+
> | multicore_score | 3320.9 (0.0%) | 3303.3 (-0.53%) | 3293.6 (-0.82%) | 3302.3 (-0.56%) |
> | score | 1415.7 (0.0%) | 1417.7 (0.14%) | 1423.4 (0.54%) | 1425.8 (0.71%) |
> | CPU_total_power | 2421.3 (0.0%) | 2429.3 (0.33%) | 2442.2 (0.86%) | 2461.9 (1.67%) |
> | latency (AsyncTask #1) | 49.41μ (0.0%) | 51.07μ (3.36%) | 50.1μ (1.4%) | 50.76μ (2.73%) |
> | latency (labs.geekbench6) | 65.63μ (0.0%) | 77.47μ (18.03%) | 55.82μ (-14.95%) | 66.12μ (0.75%) |
> | latency (surfaceflinger) | 39.46μ (0.0%) | 36.94μ (-6.39%) | 35.79μ (-9.28%) | 40.36μ (2.3%) |
> +---------------------------+---------------+-----------------+-------------------+----------------------+
>
> +----------------------+-------------+------------+
> | tag | type | count_perc |
> +----------------------+-------------+------------+
> | teo | too deep | 2.034 |
> | teo_tick | too deep | 2.16 |
> | teo_tick_rfc | too deep | 2.071 |
> | teo_tick_rfc_v2 | too deep | 2.548 |
> | teo | too shallow | 15.791 |
> | teo_tick | too shallow | 20.881 |
> | teo_tick_rfc | too shallow | 20.337 |
> | teo_tick_rfc_v2 | too shallow | 19.886 |
> +----------------------+-------------+------------+
>
>
> 2. JetNews
>
> +-----------------+---------------+----------------+-----------------+-----------------+
> | metric | teo | teo_tick | teo_tick_rfc | teo_tick_rfc_v2 |
> +-----------------+---------------+----------------+-----------------+-----------------+
> | fps | 86.2 (0.0%) | 86.4 (0.16%) | 86.0 (-0.28%) | 86.6 (0.41%) |
> | janks_pc | 0.8 (0.0%) | 0.8 (-0.66%) | 0.8 (-1.37%) | 0.7 (-11.37%) |
> | CPU_total_power | 185.2 (0.0%) | 178.2 (-3.76%) | 182.2 (-1.6%) | 169.4 (-8.53%) | <- very interesting
> +-----------------+---------------+----------------+-----------------+-----------------+
>
> +----------------------+-------------+--------------------+
> | tag | type | count_perc |
> +----------------------+-------------+--------------------+
> | teo | too deep | 0.992 |
> | teo_tick | too deep | 0.945 |
> | teo_tick_rfc | too deep | 1.035 |
> | teo_tick_rfc_v2 | too deep | 1.127 |
> | teo | too shallow | 17.085 |
> | teo_tick | too shallow | 15.236 |
> | teo_tick_rfc | too shallow | 15.379 |
> | teo_tick_rfc_v2 | too shallow | 15.34 |
> +----------------------+-------------+--------------------+
>
> All in all looks pretty good. Unfortunately there's a slightly larger
> percentage of too deep sleeps with the v2 (which is probably where the
> increase in GB6 power usage comes from) but the lower jank percentage +
> substantially lower power usage for the UI workload are very promising.
>
> Since we don't care about GB6 power usage as much as UI power usage, I'd
> say that the patchset looks good :)
>
> Tested-by: Kajetan Puchalski <[email protected]>
Thanks a lot, much appreciated!
On Mon, Aug 7, 2023 at 5:38 PM Anna-Maria Behnsen
<[email protected]> wrote:
>
> On Thu, 3 Aug 2023, Rafael J. Wysocki wrote:
>
> > On Thu, Aug 3, 2023 at 11:12 PM Rafael J. Wysocki <[email protected]> wrote:
> > >
> > > Hi Folks,
> > >
> > > This is the second iteration of:
> > >
> > > https://lore.kernel.org/linux-pm/4511619.LvFx2qVVIh@kreacher/
> > >
> > > with an additional patch.
> > >
> > > There are some small modifications of patch [1/3] and the new
> > > patch causes governor statistics to play a role in deciding whether
> > > or not to stop the scheduler tick.
> > >
> > > Testing would be much appreciated!
> >
> > For convenience, this series is now available in the following git branch:
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > pm-cpuidle-teo
> >
>
> Gauthams tests and the distribution of idle time durations looks pretty
> good. Also the prevention of calling tick_nohz_get_sleep_length() is very
> nice (21477 calls of tick_nohz_next_event() and the tick was stopped 2670
> times).
>
> Here is the deviation of idle time durations (based on your branch):
>
> Idle Total 2670 100.00%
> x >= 4ms 2537 95.02%
> 4ms> x >= 2ms 19 0.71%
> 2ms > x >= 1ms 10 0.37%
> 1ms > x >= 500us 7 0.26%
> 500us > x >= 250us 6 0.22%
> 250us > x >=100us 13 0.49%
> 100us > x >= 50us 17 0.64%
> 50us > x >= 25us 25 0.94%
> 25us > x >= 10us 22 0.82%
> 10us > x > 5us 9 0.34%
> 5us > x 5 0.19%
Thanks a lot for the data!
Can I add a Tested-by: tag from you to this series?
On Mon, 7 Aug 2023, Rafael J. Wysocki wrote:
> On Mon, Aug 7, 2023 at 5:38 PM Anna-Maria Behnsen
> <[email protected]> wrote:
> >
> > On Thu, 3 Aug 2023, Rafael J. Wysocki wrote:
> >
> > > On Thu, Aug 3, 2023 at 11:12 PM Rafael J. Wysocki <[email protected]> wrote:
> > > >
> > > > Hi Folks,
> > > >
> > > > This is the second iteration of:
> > > >
> > > > https://lore.kernel.org/linux-pm/4511619.LvFx2qVVIh@kreacher/
> > > >
> > > > with an additional patch.
> > > >
> > > > There are some small modifications of patch [1/3] and the new
> > > > patch causes governor statistics to play a role in deciding whether
> > > > or not to stop the scheduler tick.
> > > >
> > > > Testing would be much appreciated!
> > >
> > > For convenience, this series is now available in the following git branch:
> > >
> > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > pm-cpuidle-teo
> > >
> >
> > Gauthams tests and the distribution of idle time durations looks pretty
> > good. Also the prevention of calling tick_nohz_get_sleep_length() is very
> > nice (21477 calls of tick_nohz_next_event() and the tick was stopped 2670
> > times).
> >
> > Here is the deviation of idle time durations (based on your branch):
> >
> > Idle Total 2670 100.00%
> > x >= 4ms 2537 95.02%
> > 4ms> x >= 2ms 19 0.71%
> > 2ms > x >= 1ms 10 0.37%
> > 1ms > x >= 500us 7 0.26%
> > 500us > x >= 250us 6 0.22%
> > 250us > x >=100us 13 0.49%
> > 100us > x >= 50us 17 0.64%
> > 50us > x >= 25us 25 0.94%
> > 25us > x >= 10us 22 0.82%
> > 10us > x > 5us 9 0.34%
> > 5us > x 5 0.19%
>
> Thanks a lot for the data!
>
> Can I add a Tested-by: tag from you to this series?
>
Sure - sorry for the delay!
On Wed, Aug 9, 2023 at 5:10 PM Anna-Maria Behnsen
<[email protected]> wrote:
>
> On Mon, 7 Aug 2023, Rafael J. Wysocki wrote:
>
> > On Mon, Aug 7, 2023 at 5:38 PM Anna-Maria Behnsen
> > <[email protected]> wrote:
> > >
> > > On Thu, 3 Aug 2023, Rafael J. Wysocki wrote:
> > >
> > > > On Thu, Aug 3, 2023 at 11:12 PM Rafael J. Wysocki <[email protected]> wrote:
> > > > >
> > > > > Hi Folks,
> > > > >
> > > > > This is the second iteration of:
> > > > >
> > > > > https://lore.kernel.org/linux-pm/4511619.LvFx2qVVIh@kreacher/
> > > > >
> > > > > with an additional patch.
> > > > >
> > > > > There are some small modifications of patch [1/3] and the new
> > > > > patch causes governor statistics to play a role in deciding whether
> > > > > or not to stop the scheduler tick.
> > > > >
> > > > > Testing would be much appreciated!
> > > >
> > > > For convenience, this series is now available in the following git branch:
> > > >
> > > > git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git \
> > > > pm-cpuidle-teo
> > > >
> > >
> > > Gauthams tests and the distribution of idle time durations looks pretty
> > > good. Also the prevention of calling tick_nohz_get_sleep_length() is very
> > > nice (21477 calls of tick_nohz_next_event() and the tick was stopped 2670
> > > times).
> > >
> > > Here is the deviation of idle time durations (based on your branch):
> > >
> > > Idle Total 2670 100.00%
> > > x >= 4ms 2537 95.02%
> > > 4ms> x >= 2ms 19 0.71%
> > > 2ms > x >= 1ms 10 0.37%
> > > 1ms > x >= 500us 7 0.26%
> > > 500us > x >= 250us 6 0.22%
> > > 250us > x >=100us 13 0.49%
> > > 100us > x >= 50us 17 0.64%
> > > 50us > x >= 25us 25 0.94%
> > > 25us > x >= 10us 22 0.82%
> > > 10us > x > 5us 9 0.34%
> > > 5us > x 5 0.19%
> >
> > Thanks a lot for the data!
> >
> > Can I add a Tested-by: tag from you to this series?
> >
>
> Sure - sorry for the delay!
No worries, thanks!