* RE: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
@ 2019-09-26 16:31 Doug Smythies
2019-09-29 16:04 ` Doug Smythies
0 siblings, 1 reply; 15+ messages in thread
From: Doug Smythies @ 2019-09-26 16:31 UTC (permalink / raw)
To: 'Rafael J. Wysocki'
Cc: 'Srinivas Pandruvada', 'Peter Zijlstra',
'LKML', 'Frederic Weisbecker',
'Mel Gorman', 'Daniel Lezcano',
'Chen, Hu', 'Quentin Perret', 'Linux PM',
'Giovanni Gherdovich'
All,
Recall the extensive tests and work done between 2018.10.11 and 2019.01.16
on Rafael's teo governor, versions 1 through 11, noting that versions
9, 10, and 11 did not contain functional changes.
Hi Rafael,
If the deepest idle state is disabled, the system
can become somewhat unstable, with anywhere between no problem
at all, to the occasional temporary jump using a lot more
power for a few seconds, to a permanent jump using a lot more
power continuously. I have been unable to isolate the exact
test load conditions under which this will occur. However,
temporarily disabling and then enabling other idle states
seems to make for a somewhat repeatable test. It is important
to note that the issue occurs with only ever disabling the deepest
idle state, just not reliably.
I want to know how you want to proceed before I do a bunch of
regression testing.
On 2018.12.11 03:50 Rafael J. Wysocki wrote:
> v7 -> v8:
> * Apply the selection rules to the idle deepest state as well as to
> the shallower ones (the deepest idle state was treated differently
> before by mistake).
> * Subtract 1/2 of the exit latency from the measured idle duration
> in teo_update() (instead of subtracting the entire exit latency).
> This makes the idle state selection be slightly more performance-
> oriented.
I have isolated the issue to a subset of the v7 to v8 changes, however
it was not the exit latency changes.
The partial revert to V7 changes I made were (on top of 5.3):
diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index 7d05efd..d2892bb 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -283,9 +283,28 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
if (idx < 0)
idx = i; /* first enabled state */
- if (s->target_residency > duration_us)
- break;
+ if (s->target_residency > duration_us) {
+ /*
+ * If the "hits" metric of the state matching the sleep
+ * length is greater than its "misses" metric, that is
+ * the one to use.
+ */
+ if (cpu_data->states[idx].hits > cpu_data->states[idx].misses)
+ break;
+ /*
+ * It is more likely that one of the shallower states
+ * will match the idle duration measured after wakeup,
+ * so take the one with the maximum "early hits" metric,
+ * but if that cannot be determined, just use the state
+ * selected so far.
+ */
+ if (max_early_idx >= 0) {
+ idx = max_early_idx;
+ duration_us = drv->states[idx].target_residency;
+ }
+ break;
+ }
if (s->exit_latency > latency_req) {
/*
* If we break out of the loop for latency reasons, use
@@ -294,7 +313,7 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
* as long as that target residency is low enough.
*/
duration_us = drv->states[idx].target_residency;
- goto refine;
+ break;
}
idx = i;
@@ -307,21 +326,6 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
}
}
- /*
- * If the "hits" metric of the idle state matching the sleep length is
- * greater than its "misses" metric, that is the one to use. Otherwise,
- * it is more likely that one of the shallower states will match the
- * idle duration observed after wakeup, so take the one with the maximum
- * "early hits" metric, but if that cannot be determined, just use the
- * state selected so far.
- */
- if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
- max_early_idx >= 0) {
- idx = max_early_idx;
- duration_us = drv->states[idx].target_residency;
- }
-
-refine:
if (idx < 0) {
idx = 0; /* No states enabled. Must use 0. */
} else if (idx > 0) {
Test results (note: I use my own monitoring utility, because it feeds my
graphing/html utility, but have used turbostat here in case others
want to repeat the test):
Processor: i7-2600K
Deepest idle state: 4 (C6)
The system is mostly idle for these tests.
turbostat command used:
sudo turbostat --Summary --quiet --hide
IRQ,Avg_MHz,SMI,GFXMHz,TSC_MHz,GFXWatt,CorWatt,CPU%c1,CPU%c7,CoreTmp,GFX%rc6,Pkg%pc2,Pkg%pc3,Pkg%pc6,C1,C1E,,C1%,C1E%,C3%,C6%
--interval 3
Only because it works so reliably, the test involves disabling all idle states
deeper than 0, then enabling all idle states shallower than the deepest.
I have attempted to minimize the number of display columns,
hopefully without hiding useful information.
Kernel 5.3:
Busy% Bzy_MHz POLL C3 C6 POLL% CPU%c3 CPU%c6 PkgTmp PkgWatt
0.04 1600 0 0 232 0.00 0.00 99.85 28 3.73
0.05 1600 0 3 254 0.00 0.00 99.82 28 3.73
0.04 1600 0 1 264 0.00 0.00 99.82 30 3.73
61.08 3493 14735 16 258 60.84 0.04 38.57 45 31.00
100.00 3500 24057 0 0 99.88 0.00 0.00 46 48.73
100.00 3500 24064 0 0 99.86 0.00 0.00 48 48.87
100.00 3500 24057 0 0 99.88 0.00 0.00 49 49.07
100.00 3500 24070 0 0 99.87 0.00 0.00 50 49.21
100.00 3500 24068 0 0 99.87 0.00 0.00 50 49.29
100.00 3500 24063 0 0 99.88 0.00 0.00 52 49.42
100.00 3500 24060 0 0 99.87 0.00 0.00 53 49.58
100.00 3500 24076 0 0 99.87 0.00 0.00 53 49.70
100.00 3500 24067 0 0 99.87 0.00 0.00 55 49.77
28.89 3575 15138 261 0 28.71 59.97 0.00 48 27.93
10.52 3666 11884 379 0 10.43 75.24 0.00 47 20.72
10.80 3680 12141 558 0 10.65 75.13 0.00 46 21.11
11.51 3742 13016 217 0 11.43 74.93 0.00 47 22.48
10.84 3696 12254 294 0 10.76 74.89 0.00 48 21.26
10.34 3652 11683 243 0 10.25 74.93 0.00 47 20.36
10.67 3681 12065 240 0 10.59 74.94 0.00 48 20.96
10.68 3671 12067 278 0 10.59 74.92 0.00 47 20.85
10.84 3690 12259 236 0 10.76 74.94 0.00 47 21.24
11.51 3735 13023 265 0 11.43 74.93 0.00 48 22.43
Notice how once idle state 3 is enabled again (as were states 1 and 2) and we
should be at about 5 watts, there is still a ton of time spent in idle state
0 (POLL).
Kernel 5.3 + above patch:
Busy% Bzy_MHz POLL C3 C6 POLL% CPU%c3 CPU%c6 PkgTmp PkgWatt
0.04 1600 0 3 235 0.00 0.00 99.82 26 3.71
0.04 1600 0 1 252 0.00 0.00 99.79 26 3.71
0.05 1600 0 3 273 0.00 0.00 99.79 26 3.72
0.04 1600 0 3 224 0.00 0.00 99.83 26 3.71
0.05 1600 0 1 252 0.00 0.00 99.81 25 3.71
0.04 1600 0 1 231 0.00 0.00 99.84 26 3.71
0.05 1600 0 3 294 0.00 0.00 99.77 28 3.71
0.04 1600 0 2 259 0.00 0.00 99.81 25 3.70
69.61 3490 16773 15 326 69.33 0.14 29.98 42 34.41
100.00 3500 24079 0 0 99.88 0.00 0.00 45 48.27
100.00 3500 24062 0 0 99.89 0.00 0.00 45 48.41
100.00 3500 24056 0 0 99.88 0.00 0.00 46 48.52
100.00 3500 24064 0 0 99.89 0.00 0.00 47 48.63
27.31 3498 6588 340 0 27.16 72.40 0.00 32 17.13
0.03 1600 1 230 0 0.00 99.91 0.00 30 5.08
0.03 1600 2 234 0 0.00 99.90 0.00 31 5.07
0.03 1600 5 238 0 0.00 99.91 0.00 30 5.07
0.03 1600 3 230 0 0.00 99.91 0.00 30 5.05
0.03 1600 3 246 0 0.00 99.91 0.00 29 5.05
0.03 1600 3 235 0 0.00 99.90 0.00 30 5.04
Notice how once idle state 3 is enabled again (as were states 1 and 2) almost
all time is spent there, as expected, and the power is the expected 5 watts.
... Doug
^ permalink raw reply related [flat|nested] 15+ messages in thread
* RE: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-09-26 16:31 [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems Doug Smythies
@ 2019-09-29 16:04 ` Doug Smythies
2019-10-01 9:31 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Doug Smythies @ 2019-09-29 16:04 UTC (permalink / raw)
To: 'Rafael J. Wysocki'
Cc: 'Srinivas Pandruvada', 'Peter Zijlstra',
'LKML', 'Frederic Weisbecker',
'Mel Gorman', 'Daniel Lezcano',
'Chen, Hu', 'Quentin Perret', 'Linux PM',
'Giovanni Gherdovich'
On 2019.09.26 09:32 Doug Smythies wrote:
> If the deepest idle state is disabled, the system
> can become somewhat unstable, with anywhere between no problem
> at all, to the occasional temporary jump using a lot more
> power for a few seconds, to a permanent jump using a lot more
> power continuously. I have been unable to isolate the exact
> test load conditions under which this will occur. However,
> temporarily disabling and then enabling other idle states
> seems to make for a somewhat repeatable test. It is important
> to note that the issue occurs with only ever disabling the deepest
> idle state, just not reliably.
>
> I want to know how you want to proceed before I do a bunch of
> regression testing.
I did some regression testing anyhow, more to create and debug
a methodology than anything else.
> On 2018.12.11 03:50 Rafael J. Wysocki wrote:
>
>> v7 -> v8:
>> * Apply the selection rules to the idle deepest state as well as to
>> the shallower ones (the deepest idle state was treated differently
>> before by mistake).
>> * Subtract 1/2 of the exit latency from the measured idle duration
>> in teo_update() (instead of subtracting the entire exit latency).
>> This makes the idle state selection be slightly more performance-
>> oriented.
>
> I have isolated the issue to a subset of the v7 to v8 changes, however
> it was not the exit latency changes.
>
> The partial revert to V7 changes I made were (on top of 5.3):
The further testing showed a problem or two with my partial teo-v7 reversion
(I call it teo-v12) under slightly different testing conditions.
I also have a 5.3 based kernel with the current teo reverted and the entire
teo-v7 put in its place. I have yet to find a idle state disabled related issue
with this kernel.
I'll come back to this thread at a later date with better details and test results.
... Doug
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-09-29 16:04 ` Doug Smythies
@ 2019-10-01 9:31 ` Rafael J. Wysocki
2019-10-06 14:46 ` Doug Smythies
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2019-10-01 9:31 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Srinivas Pandruvada, Peter Zijlstra, LKML,
Frederic Weisbecker, Mel Gorman, Daniel Lezcano, Chen, Hu,
Quentin Perret, Linux PM, Giovanni Gherdovich
On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2019.09.26 09:32 Doug Smythies wrote:
>
> > If the deepest idle state is disabled, the system
> > can become somewhat unstable, with anywhere between no problem
> > at all, to the occasional temporary jump using a lot more
> > power for a few seconds, to a permanent jump using a lot more
> > power continuously. I have been unable to isolate the exact
> > test load conditions under which this will occur. However,
> > temporarily disabling and then enabling other idle states
> > seems to make for a somewhat repeatable test. It is important
> > to note that the issue occurs with only ever disabling the deepest
> > idle state, just not reliably.
> >
> > I want to know how you want to proceed before I do a bunch of
> > regression testing.
>
> I did some regression testing anyhow, more to create and debug
> a methodology than anything else.
>
> > On 2018.12.11 03:50 Rafael J. Wysocki wrote:
> >
> >> v7 -> v8:
> >> * Apply the selection rules to the idle deepest state as well as to
> >> the shallower ones (the deepest idle state was treated differently
> >> before by mistake).
> >> * Subtract 1/2 of the exit latency from the measured idle duration
> >> in teo_update() (instead of subtracting the entire exit latency).
> >> This makes the idle state selection be slightly more performance-
> >> oriented.
> >
> > I have isolated the issue to a subset of the v7 to v8 changes, however
> > it was not the exit latency changes.
> >
> > The partial revert to V7 changes I made were (on top of 5.3):
>
> The further testing showed a problem or two with my partial teo-v7 reversion
> (I call it teo-v12) under slightly different testing conditions.
>
> I also have a 5.3 based kernel with the current teo reverted and the entire
> teo-v7 put in its place. I have yet to find a idle state disabled related issue
> with this kernel.
>
> I'll come back to this thread at a later date with better details and test results.
Thanks for this work!
Please also note that there is a teo patch in 5.4-rc1 that may make a
difference in principle.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-01 9:31 ` Rafael J. Wysocki
@ 2019-10-06 14:46 ` Doug Smythies
2019-10-06 15:34 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Doug Smythies @ 2019-10-06 14:46 UTC (permalink / raw)
To: 'Rafael J. Wysocki'
Cc: 'Rafael J. Wysocki', 'Srinivas Pandruvada',
'Peter Zijlstra', 'LKML',
'Frederic Weisbecker', 'Mel Gorman',
'Daniel Lezcano', 'Chen, Hu',
'Quentin Perret', 'Linux PM',
'Giovanni Gherdovich'
On 2019.10.01 02:32 Rafael J. Wysocki wrote:
> On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
>> On 2019.09.26 09:32 Doug Smythies wrote:
>>
>>> If the deepest idle state is disabled, the system
>>> can become somewhat unstable, with anywhere between no problem
>>> at all, to the occasional temporary jump using a lot more
>>> power for a few seconds, to a permanent jump using a lot more
>>> power continuously. I have been unable to isolate the exact
>>> test load conditions under which this will occur. However,
>>> temporarily disabling and then enabling other idle states
>>> seems to make for a somewhat repeatable test. It is important
>>> to note that the issue occurs with only ever disabling the deepest
>>> idle state, just not reliably.
>>>
>>> I want to know how you want to proceed before I do a bunch of
>>> regression testing.
>>
>> I did some regression testing anyhow, more to create and debug
>> a methodology than anything else.
>>
>>> On 2018.12.11 03:50 Rafael J. Wysocki wrote:
>>>
>>>> v7 -> v8:
>>>> * Apply the selection rules to the idle deepest state as well as to
>>>> the shallower ones (the deepest idle state was treated differently
>>>> before by mistake).
>>>> * Subtract 1/2 of the exit latency from the measured idle duration
>>>> in teo_update() (instead of subtracting the entire exit latency).
>>>> This makes the idle state selection be slightly more performance-
>>>> oriented.
>>>
>>> I have isolated the issue to a subset of the v7 to v8 changes, however
>>> it was not the exit latency changes.
>>>
>>> The partial revert to V7 changes I made were (on top of 5.3):
>>
>> The further testing showed a problem or two with my partial teo-v7 reversion
>> (I call it teo-v12) under slightly different testing conditions.
Correction:
There was no problem with my partial reversion kernel (a.k.a. teo-v12). The problem
was confusion over which kernel I was actually running for whatever test.
>>
>> I also have a 5.3 based kernel with the current teo reverted and the entire
>> teo-v7 put in its place. I have yet to find a idle state disabled related issue
>> with this kernel.
>>
>> I'll come back to this thread at a later date with better details and test results.
>
> Thanks for this work!
>
> Please also note that there is a teo patch in 5.4-rc1 that may make a
> difference in principle.
Yes, actually this saga started from somewhere between kernel 5.3 and 5.4-rc1,
and did include those teo patches, which actually significantly increases the
probability of the issue occurring.
When the deepest idle state is disabled, and the all states search loop exits
normally, it might incorrectly re-evaluate a previous idle state previously
deemed not worthy of the check. This was introduced between teo development
versions 7 and 8. The fix is to move the code back inside the loop.
(I'll submit a patch in a day or two).
I do not think I stated it clearly before: The problem here is that some CPUs
seem to get stuck in idle state 0, and when they do power consumption spikes,
often by several hundred % and often indefinitely.
I made a hack job automated test:
Kernel tests fail rate
5.4-rc1 6616 13.45%
5.3 2376 4.50%
5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
5.4-rc1-ds 11168 0.00% <<< proposed patch (> 7 hours test time)
Proposed patch (on top of kernel 5.4-rc1):
doug@s15:~/temp-k-git/linux$ git diff
diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index b5a0e49..0502aa9 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -276,8 +276,22 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
if (idx < 0)
idx = i; /* first enabled state */
- if (s->target_residency > duration_us)
+ if (s->target_residency > duration_us){
+ /*
+ * If the "hits" metric of the idle state matching the sleep length is
+ * greater than its "misses" metric, that is the one to use. Otherwise,
+ * it is more likely that one of the shallower states will match the
+ * idle duration observed after wakeup, so take the one with the maximum
+ * "early hits" metric, but if that cannot be determined, just use the
+ * state selected so far.
+ */
+ if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
+ max_early_idx >= 0) {
+ idx = max_early_idx;
+ duration_us = drv->states[idx].target_residency;
+ }
break;
+ }
if (s->exit_latency > latency_req && constraint_idx > i)
constraint_idx = i;
@@ -293,20 +307,6 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
}
/*
- * If the "hits" metric of the idle state matching the sleep length is
- * greater than its "misses" metric, that is the one to use. Otherwise,
- * it is more likely that one of the shallower states will match the
- * idle duration observed after wakeup, so take the one with the maximum
- * "early hits" metric, but if that cannot be determined, just use the
- * state selected so far.
- */
- if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
- max_early_idx >= 0) {
- idx = max_early_idx;
- duration_us = drv->states[idx].target_residency;
- }
-
- /*
* If there is a latency constraint, it may be necessary to use a
* shallower idle state than the one selected so far.
*/
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-06 14:46 ` Doug Smythies
@ 2019-10-06 15:34 ` Rafael J. Wysocki
2019-10-08 6:20 ` Doug Smythies
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2019-10-06 15:34 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Srinivas Pandruvada,
Peter Zijlstra, LKML, Frederic Weisbecker, Mel Gorman,
Daniel Lezcano, Chen, Hu, Quentin Perret, Linux PM,
Giovanni Gherdovich
On Sun, Oct 6, 2019 at 4:46 PM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2019.10.01 02:32 Rafael J. Wysocki wrote:
> > On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
> >> On 2019.09.26 09:32 Doug Smythies wrote:
> >>
> >>> If the deepest idle state is disabled, the system
> >>> can become somewhat unstable, with anywhere between no problem
> >>> at all, to the occasional temporary jump using a lot more
> >>> power for a few seconds, to a permanent jump using a lot more
> >>> power continuously. I have been unable to isolate the exact
> >>> test load conditions under which this will occur. However,
> >>> temporarily disabling and then enabling other idle states
> >>> seems to make for a somewhat repeatable test. It is important
> >>> to note that the issue occurs with only ever disabling the deepest
> >>> idle state, just not reliably.
> >>>
> >>> I want to know how you want to proceed before I do a bunch of
> >>> regression testing.
> >>
> >> I did some regression testing anyhow, more to create and debug
> >> a methodology than anything else.
> >>
> >>> On 2018.12.11 03:50 Rafael J. Wysocki wrote:
> >>>
> >>>> v7 -> v8:
> >>>> * Apply the selection rules to the idle deepest state as well as to
> >>>> the shallower ones (the deepest idle state was treated differently
> >>>> before by mistake).
> >>>> * Subtract 1/2 of the exit latency from the measured idle duration
> >>>> in teo_update() (instead of subtracting the entire exit latency).
> >>>> This makes the idle state selection be slightly more performance-
> >>>> oriented.
> >>>
> >>> I have isolated the issue to a subset of the v7 to v8 changes, however
> >>> it was not the exit latency changes.
> >>>
> >>> The partial revert to V7 changes I made were (on top of 5.3):
> >>
> >> The further testing showed a problem or two with my partial teo-v7 reversion
> >> (I call it teo-v12) under slightly different testing conditions.
>
> Correction:
> There was no problem with my partial reversion kernel (a.k.a. teo-v12). The problem
> was confusion over which kernel I was actually running for whatever test.
>
> >>
> >> I also have a 5.3 based kernel with the current teo reverted and the entire
> >> teo-v7 put in its place. I have yet to find a idle state disabled related issue
> >> with this kernel.
> >>
> >> I'll come back to this thread at a later date with better details and test results.
> >
> > Thanks for this work!
> >
> > Please also note that there is a teo patch in 5.4-rc1 that may make a
> > difference in principle.
>
> Yes, actually this saga started from somewhere between kernel 5.3 and 5.4-rc1,
> and did include those teo patches, which actually significantly increases the
> probability of the issue occurring.
>
> When the deepest idle state is disabled, and the all states search loop exits
> normally, it might incorrectly re-evaluate a previous idle state previously
> deemed not worthy of the check. This was introduced between teo development
> versions 7 and 8. The fix is to move the code back inside the loop.
> (I'll submit a patch in a day or two).
OK
> I do not think I stated it clearly before: The problem here is that some CPUs
> seem to get stuck in idle state 0, and when they do power consumption spikes,
> often by several hundred % and often indefinitely.
That indeed has not been clear to me, thanks for the clarification!
> I made a hack job automated test:
> Kernel tests fail rate
> 5.4-rc1 6616 13.45%
> 5.3 2376 4.50%
> 5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
> 5.4-rc1-ds 11168 0.00% <<< proposed patch (> 7 hours test time)
>
> Proposed patch (on top of kernel 5.4-rc1):
>
> doug@s15:~/temp-k-git/linux$ git diff
> diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> index b5a0e49..0502aa9 100644
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -276,8 +276,22 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> if (idx < 0)
> idx = i; /* first enabled state */
>
> - if (s->target_residency > duration_us)
> + if (s->target_residency > duration_us){
> + /*
> + * If the "hits" metric of the idle state matching the sleep length is
> + * greater than its "misses" metric, that is the one to use. Otherwise,
> + * it is more likely that one of the shallower states will match the
> + * idle duration observed after wakeup, so take the one with the maximum
> + * "early hits" metric, but if that cannot be determined, just use the
> + * state selected so far.
> + */
> + if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
> + max_early_idx >= 0) {
> + idx = max_early_idx;
> + duration_us = drv->states[idx].target_residency;
> + }
> break;
> + }
>
> if (s->exit_latency > latency_req && constraint_idx > i)
> constraint_idx = i;
> @@ -293,20 +307,6 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> }
>
> /*
> - * If the "hits" metric of the idle state matching the sleep length is
> - * greater than its "misses" metric, that is the one to use. Otherwise,
> - * it is more likely that one of the shallower states will match the
> - * idle duration observed after wakeup, so take the one with the maximum
> - * "early hits" metric, but if that cannot be determined, just use the
> - * state selected so far.
> - */
> - if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
> - max_early_idx >= 0) {
> - idx = max_early_idx;
> - duration_us = drv->states[idx].target_residency;
> - }
> -
> - /*
> * If there is a latency constraint, it may be necessary to use a
> * shallower idle state than the one selected so far.
> */
This change may cause the deepest state to be selected even if its
"hits" metric is less than the "misses" one AFAICS, in which case the
max_early_index state should be selected instead.
It looks like the max_early_index computation is broken when the
deepest state is disabled.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-06 15:34 ` Rafael J. Wysocki
@ 2019-10-08 6:20 ` Doug Smythies
2019-10-08 9:51 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Doug Smythies @ 2019-10-08 6:20 UTC (permalink / raw)
To: 'Rafael J. Wysocki'
Cc: 'Rafael J. Wysocki', 'Srinivas Pandruvada',
'Peter Zijlstra', 'LKML',
'Frederic Weisbecker', 'Mel Gorman',
'Daniel Lezcano', 'Chen, Hu',
'Quentin Perret', 'Linux PM',
'Giovanni Gherdovich'
On 2019.10.06 08:34 Rafael J. Wysocki wrote:
> On Sun, Oct 6, 2019 at 4:46 PM Doug Smythies <dsmythies@telus.net> wrote:
>> On 2019.10.01 02:32 Rafael J. Wysocki wrote:
>>> On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
>>>> On 2019.09.26 09:32 Doug Smythies wrote:
>>>>
>>>>> If the deepest idle state is disabled, the system
>>>>> can become somewhat unstable, with anywhere between no problem
>>>>> at all, to the occasional temporary jump using a lot more
>>>>> power for a few seconds, to a permanent jump using a lot more
>>>>> power continuously. I have been unable to isolate the exact
>>>>> test load conditions under which this will occur. However,
>>>>> temporarily disabling and then enabling other idle states
>>>>> seems to make for a somewhat repeatable test. It is important
>>>>> to note that the issue occurs with only ever disabling the deepest
>>>>> idle state, just not reliably.
>>>>>
>>>>> I want to know how you want to proceed before I do a bunch of
>>>>> regression testing.
>>>>
>> I do not think I stated it clearly before: The problem here is that some CPUs
>> seem to get stuck in idle state 0, and when they do power consumption spikes,
>> often by several hundred % and often indefinitely.
>
> That indeed has not been clear to me, thanks for the clarification!
>
>> I made a hack job automated test:
>> Kernel tests fail rate
>> 5.4-rc1 6616 13.45%
>> 5.3 2376 4.50%
>> 5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
>> 5.4-rc1-ds 11168 0.00% <<< [old] proposed patch (> 7 hours test time)
5.4-rc1-ds12 4224 0.005 <<< new proposed patch
>>
>> [old] Proposed patch (on top of kernel 5.4-rc1): [deleted]
> This change may cause the deepest state to be selected even if its
> "hits" metric is less than the "misses" one AFAICS, in which case the
> max_early_index state should be selected instead.
>
> It looks like the max_early_index computation is broken when the
> deepest state is disabled.
O.K. Thanks for your quick reply, and insight.
I think long durations always need to be counted, but currently if
the deepest idle state is disabled, they are not.
How about this?:
(test results added above, more tests pending if this might be a path forward.)
diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index b5a0e49..a970d2c 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -155,10 +155,12 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
cpu_data->states[i].early_hits -= early_hits >> DECAY_SHIFT;
- if (drv->states[i].target_residency <= sleep_length_us) {
- idx_timer = i;
- if (drv->states[i].target_residency <= measured_us)
- idx_hit = i;
+ if (!(drv->states[i].disabled || dev->states_usage[i].disable)){
+ if (drv->states[i].target_residency <= sleep_length_us) {
+ idx_timer = i;
+ if (drv->states[i].target_residency <= measured_us)
+ idx_hit = i;
+ }
}
}
@@ -256,39 +258,25 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
struct cpuidle_state *s = &drv->states[i];
struct cpuidle_state_usage *su = &dev->states_usage[i];
- if (s->disabled || su->disable) {
- /*
- * If the "early hits" metric of a disabled state is
- * greater than the current maximum, it should be taken
- * into account, because it would be a mistake to select
- * a deeper state with lower "early hits" metric. The
- * index cannot be changed to point to it, however, so
- * just increase the max count alone and let the index
- * still point to a shallower idle state.
- */
- if (max_early_idx >= 0 &&
- count < cpu_data->states[i].early_hits)
- count = cpu_data->states[i].early_hits;
-
- continue;
- }
- if (idx < 0)
- idx = i; /* first enabled state */
+ if (!(s->disabled || su->disable)) {
+ if (idx < 0)
+ idx = i; /* first enabled state */
- if (s->target_residency > duration_us)
- break;
+ if (s->target_residency > duration_us)
+ break;
- if (s->exit_latency > latency_req && constraint_idx > i)
- constraint_idx = i;
+ if (s->exit_latency > latency_req && constraint_idx > i)
+ constraint_idx = i;
- idx = i;
+ idx = i;
- if (count < cpu_data->states[i].early_hits &&
- !(tick_nohz_tick_stopped() &&
- drv->states[i].target_residency < TICK_USEC)) {
- count = cpu_data->states[i].early_hits;
- max_early_idx = i;
+ if (count < cpu_data->states[i].early_hits &&
+ !(tick_nohz_tick_stopped() &&
+ drv->states[i].target_residency < TICK_USEC)) {
+ count = cpu_data->states[i].early_hits;
+ max_early_idx = i;
+ }
}
}
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-08 6:20 ` Doug Smythies
@ 2019-10-08 9:51 ` Rafael J. Wysocki
2019-10-08 10:49 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2019-10-08 9:51 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Srinivas Pandruvada,
Peter Zijlstra, LKML, Frederic Weisbecker, Mel Gorman,
Daniel Lezcano, Chen, Hu, Quentin Perret, Linux PM,
Giovanni Gherdovich
On Tue, Oct 8, 2019 at 8:20 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2019.10.06 08:34 Rafael J. Wysocki wrote:
> > On Sun, Oct 6, 2019 at 4:46 PM Doug Smythies <dsmythies@telus.net> wrote:
> >> On 2019.10.01 02:32 Rafael J. Wysocki wrote:
> >>> On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
> >>>> On 2019.09.26 09:32 Doug Smythies wrote:
> >>>>
> >>>>> If the deepest idle state is disabled, the system
> >>>>> can become somewhat unstable, with anywhere between no problem
> >>>>> at all, to the occasional temporary jump using a lot more
> >>>>> power for a few seconds, to a permanent jump using a lot more
> >>>>> power continuously. I have been unable to isolate the exact
> >>>>> test load conditions under which this will occur. However,
> >>>>> temporarily disabling and then enabling other idle states
> >>>>> seems to make for a somewhat repeatable test. It is important
> >>>>> to note that the issue occurs with only ever disabling the deepest
> >>>>> idle state, just not reliably.
> >>>>>
> >>>>> I want to know how you want to proceed before I do a bunch of
> >>>>> regression testing.
> >>>>
> >> I do not think I stated it clearly before: The problem here is that some CPUs
> >> seem to get stuck in idle state 0, and when they do power consumption spikes,
> >> often by several hundred % and often indefinitely.
> >
> > That indeed has not been clear to me, thanks for the clarification!
>
> >
> >> I made a hack job automated test:
> >> Kernel tests fail rate
> >> 5.4-rc1 6616 13.45%
> >> 5.3 2376 4.50%
> >> 5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
> >> 5.4-rc1-ds 11168 0.00% <<< [old] proposed patch (> 7 hours test time)
>
>
> 5.4-rc1-ds12 4224 0.005 <<< new proposed patch
>
> >>
> >> [old] Proposed patch (on top of kernel 5.4-rc1): [deleted]
>
> > This change may cause the deepest state to be selected even if its
> > "hits" metric is less than the "misses" one AFAICS, in which case the
> > max_early_index state should be selected instead.
> >
> > It looks like the max_early_index computation is broken when the
> > deepest state is disabled.
>
> O.K. Thanks for your quick reply, and insight.
>
> I think long durations always need to be counted, but currently if
> the deepest idle state is disabled, they are not.
> How about this?:
> (test results added above, more tests pending if this might be a path forward.)
>
> diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> index b5a0e49..a970d2c 100644
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -155,10 +155,12 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>
> cpu_data->states[i].early_hits -= early_hits >> DECAY_SHIFT;
>
> - if (drv->states[i].target_residency <= sleep_length_us) {
> - idx_timer = i;
> - if (drv->states[i].target_residency <= measured_us)
> - idx_hit = i;
> + if (!(drv->states[i].disabled || dev->states_usage[i].disable)){
> + if (drv->states[i].target_residency <= sleep_length_us) {
> + idx_timer = i;
> + if (drv->states[i].target_residency <= measured_us)
> + idx_hit = i;
> + }
What if the state is enabled again after some time?
> }
> }
>
> @@ -256,39 +258,25 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> struct cpuidle_state *s = &drv->states[i];
> struct cpuidle_state_usage *su = &dev->states_usage[i];
>
> - if (s->disabled || su->disable) {
> - /*
> - * If the "early hits" metric of a disabled state is
> - * greater than the current maximum, it should be taken
> - * into account, because it would be a mistake to select
> - * a deeper state with lower "early hits" metric. The
> - * index cannot be changed to point to it, however, so
> - * just increase the max count alone and let the index
> - * still point to a shallower idle state.
> - */
> - if (max_early_idx >= 0 &&
> - count < cpu_data->states[i].early_hits)
> - count = cpu_data->states[i].early_hits;
> -
> - continue;
AFAICS, adding early_hits to count is not a mistake if there are still
enabled states deeper than the current one.
Besides, can you just leave the "continue" here instead of changing
the indentation level for everything below?
> - }
>
> - if (idx < 0)
> - idx = i; /* first enabled state */
> + if (!(s->disabled || su->disable)) {
> + if (idx < 0)
> + idx = i; /* first enabled state */
>
> - if (s->target_residency > duration_us)
> - break;
> + if (s->target_residency > duration_us)
> + break;
>
> - if (s->exit_latency > latency_req && constraint_idx > i)
> - constraint_idx = i;
> + if (s->exit_latency > latency_req && constraint_idx > i)
> + constraint_idx = i;
>
> - idx = i;
> + idx = i;
>
> - if (count < cpu_data->states[i].early_hits &&
> - !(tick_nohz_tick_stopped() &&
> - drv->states[i].target_residency < TICK_USEC)) {
> - count = cpu_data->states[i].early_hits;
> - max_early_idx = i;
> + if (count < cpu_data->states[i].early_hits &&
> + !(tick_nohz_tick_stopped() &&
> + drv->states[i].target_residency < TICK_USEC)) {
> + count = cpu_data->states[i].early_hits;
> + max_early_idx = i;
> + }
> }
> }
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-08 9:51 ` Rafael J. Wysocki
@ 2019-10-08 10:49 ` Rafael J. Wysocki
2019-10-08 23:19 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2019-10-08 10:49 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Srinivas Pandruvada, Peter Zijlstra, LKML,
Frederic Weisbecker, Mel Gorman, Daniel Lezcano, Chen, Hu,
Quentin Perret, Linux PM, Giovanni Gherdovich
On Tue, Oct 8, 2019 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Tue, Oct 8, 2019 at 8:20 AM Doug Smythies <dsmythies@telus.net> wrote:
> >
> > On 2019.10.06 08:34 Rafael J. Wysocki wrote:
> > > On Sun, Oct 6, 2019 at 4:46 PM Doug Smythies <dsmythies@telus.net> wrote:
> > >> On 2019.10.01 02:32 Rafael J. Wysocki wrote:
> > >>> On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
> > >>>> On 2019.09.26 09:32 Doug Smythies wrote:
> > >>>>
> > >>>>> If the deepest idle state is disabled, the system
> > >>>>> can become somewhat unstable, with anywhere between no problem
> > >>>>> at all, to the occasional temporary jump using a lot more
> > >>>>> power for a few seconds, to a permanent jump using a lot more
> > >>>>> power continuously. I have been unable to isolate the exact
> > >>>>> test load conditions under which this will occur. However,
> > >>>>> temporarily disabling and then enabling other idle states
> > >>>>> seems to make for a somewhat repeatable test. It is important
> > >>>>> to note that the issue occurs with only ever disabling the deepest
> > >>>>> idle state, just not reliably.
> > >>>>>
> > >>>>> I want to know how you want to proceed before I do a bunch of
> > >>>>> regression testing.
> > >>>>
> > >> I do not think I stated it clearly before: The problem here is that some CPUs
> > >> seem to get stuck in idle state 0, and when they do power consumption spikes,
> > >> often by several hundred % and often indefinitely.
> > >
> > > That indeed has not been clear to me, thanks for the clarification!
> >
> > >
> > >> I made a hack job automated test:
> > >> Kernel tests fail rate
> > >> 5.4-rc1 6616 13.45%
> > >> 5.3 2376 4.50%
> > >> 5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
> > >> 5.4-rc1-ds 11168 0.00% <<< [old] proposed patch (> 7 hours test time)
> >
> >
> > 5.4-rc1-ds12 4224 0.005 <<< new proposed patch
> >
> > >>
> > >> [old] Proposed patch (on top of kernel 5.4-rc1): [deleted]
> >
> > > This change may cause the deepest state to be selected even if its
> > > "hits" metric is less than the "misses" one AFAICS, in which case the
> > > max_early_index state should be selected instead.
> > >
> > > It looks like the max_early_index computation is broken when the
> > > deepest state is disabled.
> >
> > O.K. Thanks for your quick reply, and insight.
> >
> > I think long durations always need to be counted, but currently if
> > the deepest idle state is disabled, they are not.
> > How about this?:
> > (test results added above, more tests pending if this might be a path forward.)
> >
> > diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> > index b5a0e49..a970d2c 100644
> > --- a/drivers/cpuidle/governors/teo.c
> > +++ b/drivers/cpuidle/governors/teo.c
> > @@ -155,10 +155,12 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> >
> > cpu_data->states[i].early_hits -= early_hits >> DECAY_SHIFT;
> >
> > - if (drv->states[i].target_residency <= sleep_length_us) {
> > - idx_timer = i;
> > - if (drv->states[i].target_residency <= measured_us)
> > - idx_hit = i;
> > + if (!(drv->states[i].disabled || dev->states_usage[i].disable)){
> > + if (drv->states[i].target_residency <= sleep_length_us) {
> > + idx_timer = i;
> > + if (drv->states[i].target_residency <= measured_us)
> > + idx_hit = i;
> > + }
>
> What if the state is enabled again after some time?
Actually, the states are treated as "bins" here, so for the metrics it
doesn't matter whether or not they are enabled at the moment.
> > }
> > }
> >
> > @@ -256,39 +258,25 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > struct cpuidle_state *s = &drv->states[i];
> > struct cpuidle_state_usage *su = &dev->states_usage[i];
> >
> > - if (s->disabled || su->disable) {
> > - /*
> > - * If the "early hits" metric of a disabled state is
> > - * greater than the current maximum, it should be taken
> > - * into account, because it would be a mistake to select
> > - * a deeper state with lower "early hits" metric. The
> > - * index cannot be changed to point to it, however, so
> > - * just increase the max count alone and let the index
> > - * still point to a shallower idle state.
> > - */
> > - if (max_early_idx >= 0 &&
> > - count < cpu_data->states[i].early_hits)
> > - count = cpu_data->states[i].early_hits;
> > -
> > - continue;
>
> AFAICS, adding early_hits to count is not a mistake if there are still
> enabled states deeper than the current one.
And the mistake appears to be that the "hits" and "misses" metrics
aren't handled in analogy with the "early_hits" one when the current
state is disabled.
Let me try to cut a patch to address that.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-08 10:49 ` Rafael J. Wysocki
@ 2019-10-08 23:19 ` Rafael J. Wysocki
2019-10-09 13:36 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2019-10-08 23:19 UTC (permalink / raw)
To: Doug Smythies
Cc: Srinivas Pandruvada, Peter Zijlstra, LKML, Frederic Weisbecker,
Mel Gorman, Daniel Lezcano, Chen, Hu, Quentin Perret, Linux PM,
Giovanni Gherdovich
On Tuesday, October 8, 2019 12:49:01 PM CEST Rafael J. Wysocki wrote:
> On Tue, Oct 8, 2019 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Tue, Oct 8, 2019 at 8:20 AM Doug Smythies <dsmythies@telus.net> wrote:
> > >
> > > On 2019.10.06 08:34 Rafael J. Wysocki wrote:
> > > > On Sun, Oct 6, 2019 at 4:46 PM Doug Smythies <dsmythies@telus.net> wrote:
> > > >> On 2019.10.01 02:32 Rafael J. Wysocki wrote:
> > > >>> On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
> > > >>>> On 2019.09.26 09:32 Doug Smythies wrote:
> > > >>>>
> > > >>>>> If the deepest idle state is disabled, the system
> > > >>>>> can become somewhat unstable, with anywhere between no problem
> > > >>>>> at all, to the occasional temporary jump using a lot more
> > > >>>>> power for a few seconds, to a permanent jump using a lot more
> > > >>>>> power continuously. I have been unable to isolate the exact
> > > >>>>> test load conditions under which this will occur. However,
> > > >>>>> temporarily disabling and then enabling other idle states
> > > >>>>> seems to make for a somewhat repeatable test. It is important
> > > >>>>> to note that the issue occurs with only ever disabling the deepest
> > > >>>>> idle state, just not reliably.
> > > >>>>>
> > > >>>>> I want to know how you want to proceed before I do a bunch of
> > > >>>>> regression testing.
> > > >>>>
> > > >> I do not think I stated it clearly before: The problem here is that some CPUs
> > > >> seem to get stuck in idle state 0, and when they do power consumption spikes,
> > > >> often by several hundred % and often indefinitely.
> > > >
> > > > That indeed has not been clear to me, thanks for the clarification!
> > >
> > > >
> > > >> I made a hack job automated test:
> > > >> Kernel tests fail rate
> > > >> 5.4-rc1 6616 13.45%
> > > >> 5.3 2376 4.50%
> > > >> 5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
> > > >> 5.4-rc1-ds 11168 0.00% <<< [old] proposed patch (> 7 hours test time)
> > >
> > >
> > > 5.4-rc1-ds12 4224 0.005 <<< new proposed patch
> > >
> > > >>
> > > >> [old] Proposed patch (on top of kernel 5.4-rc1): [deleted]
> > >
> > > > This change may cause the deepest state to be selected even if its
> > > > "hits" metric is less than the "misses" one AFAICS, in which case the
> > > > max_early_index state should be selected instead.
> > > >
> > > > It looks like the max_early_index computation is broken when the
> > > > deepest state is disabled.
> > >
> > > O.K. Thanks for your quick reply, and insight.
> > >
> > > I think long durations always need to be counted, but currently if
> > > the deepest idle state is disabled, they are not.
> > > How about this?:
> > > (test results added above, more tests pending if this might be a path forward.)
> > >
> > > diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> > > index b5a0e49..a970d2c 100644
> > > --- a/drivers/cpuidle/governors/teo.c
> > > +++ b/drivers/cpuidle/governors/teo.c
> > > @@ -155,10 +155,12 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> > >
> > > cpu_data->states[i].early_hits -= early_hits >> DECAY_SHIFT;
> > >
> > > - if (drv->states[i].target_residency <= sleep_length_us) {
> > > - idx_timer = i;
> > > - if (drv->states[i].target_residency <= measured_us)
> > > - idx_hit = i;
> > > + if (!(drv->states[i].disabled || dev->states_usage[i].disable)){
> > > + if (drv->states[i].target_residency <= sleep_length_us) {
> > > + idx_timer = i;
> > > + if (drv->states[i].target_residency <= measured_us)
> > > + idx_hit = i;
> > > + }
> >
> > What if the state is enabled again after some time?
>
> Actually, the states are treated as "bins" here, so for the metrics it
> doesn't matter whether or not they are enabled at the moment.
>
> > > }
> > > }
> > >
> > > @@ -256,39 +258,25 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > > struct cpuidle_state *s = &drv->states[i];
> > > struct cpuidle_state_usage *su = &dev->states_usage[i];
> > >
> > > - if (s->disabled || su->disable) {
> > > - /*
> > > - * If the "early hits" metric of a disabled state is
> > > - * greater than the current maximum, it should be taken
> > > - * into account, because it would be a mistake to select
> > > - * a deeper state with lower "early hits" metric. The
> > > - * index cannot be changed to point to it, however, so
> > > - * just increase the max count alone and let the index
> > > - * still point to a shallower idle state.
> > > - */
> > > - if (max_early_idx >= 0 &&
> > > - count < cpu_data->states[i].early_hits)
> > > - count = cpu_data->states[i].early_hits;
> > > -
> > > - continue;
> >
> > AFAICS, adding early_hits to count is not a mistake if there are still
> > enabled states deeper than the current one.
>
> And the mistake appears to be that the "hits" and "misses" metrics
> aren't handled in analogy with the "early_hits" one when the current
> state is disabled.
>
> Let me try to cut a patch to address that.
Appended below, not tested.
It is meant to address two problems, one of which is that the "hits" and
"misses" metrics of disabled states need to be taken into account too in
some cases, and the other is an issue with the handling of "early hits"
which may lead to suboptimal state selection if some states are disabled.
---
drivers/cpuidle/governors/teo.c | 71 +++++++++++++++++++++++++++++-----------
1 file changed, 52 insertions(+), 19 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/teo.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/teo.c
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -233,7 +233,7 @@ static int teo_select(struct cpuidle_dri
{
struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
int latency_req = cpuidle_governor_latency_req(dev->cpu);
- unsigned int duration_us, count;
+ unsigned int duration_us, hits, misses, early_hits;
int max_early_idx, constraint_idx, idx, i;
ktime_t delta_tick;
@@ -247,7 +247,9 @@ static int teo_select(struct cpuidle_dri
cpu_data->sleep_length_ns = tick_nohz_get_sleep_length(&delta_tick);
duration_us = ktime_to_us(cpu_data->sleep_length_ns);
- count = 0;
+ hits = 0;
+ misses = 0;
+ early_hits = 0;
max_early_idx = -1;
constraint_idx = drv->state_count;
idx = -1;
@@ -258,23 +260,54 @@ static int teo_select(struct cpuidle_dri
if (s->disabled || su->disable) {
/*
- * If the "early hits" metric of a disabled state is
- * greater than the current maximum, it should be taken
- * into account, because it would be a mistake to select
- * a deeper state with lower "early hits" metric. The
- * index cannot be changed to point to it, however, so
- * just increase the max count alone and let the index
- * still point to a shallower idle state.
+ * If the "hits" metric of a disabled state is greater
+ * than its "misses" metric, it needs to be taken into
+ * account, because the closest shallower enabled state
+ * "represents" it in that case.
*/
- if (max_early_idx >= 0 &&
- count < cpu_data->states[i].early_hits)
- count = cpu_data->states[i].early_hits;
+ if (cpu_data->states[i].hits > cpu_data->states[i].misses) {
+ hits = cpu_data->states[i].hits;
+ misses = cpu_data->states[i].misses;
+ }
+
+ if (early_hits >= cpu_data->states[i].early_hits ||
+ idx < 0)
+ continue;
+
+ /*
+ * If the current candidate state has been the one with
+ * the maximum "early hits" metric so far, the "early
+ * hits" metric of the disabled state replaces the
+ * current "early hits" count to avoid selecting a
+ * deeper state with lower "early hits" metric.
+ */
+ if (max_early_idx == idx) {
+ early_hits = cpu_data->states[i].early_hits;
+ continue;
+ }
+
+ /*
+ * The current candidate state is closer to the disabled
+ * one than the current maximum "early hits" state, so
+ * replace the latter with it, but in case the maximum
+ * "early hits" state index has not been set so far,
+ * check if the current candidate state is not too
+ * shallow for that role.
+ */
+ if (!(tick_nohz_tick_stopped() &&
+ drv->states[idx].target_residency < TICK_USEC)) {
+ early_hits = cpu_data->states[i].early_hits;
+ max_early_idx = idx;
+ }
continue;
}
- if (idx < 0)
+ if (idx < 0) {
idx = i; /* first enabled state */
+ hits = cpu_data->states[i].hits;
+ misses = cpu_data->states[i].misses;
+ }
if (s->target_residency > duration_us)
break;
@@ -283,11 +316,13 @@ static int teo_select(struct cpuidle_dri
constraint_idx = i;
idx = i;
+ hits = cpu_data->states[i].hits;
+ misses = cpu_data->states[i].misses;
- if (count < cpu_data->states[i].early_hits &&
+ if (early_hits < cpu_data->states[i].early_hits &&
!(tick_nohz_tick_stopped() &&
drv->states[i].target_residency < TICK_USEC)) {
- count = cpu_data->states[i].early_hits;
+ early_hits = cpu_data->states[i].early_hits;
max_early_idx = i;
}
}
@@ -300,8 +335,7 @@ static int teo_select(struct cpuidle_dri
* "early hits" metric, but if that cannot be determined, just use the
* state selected so far.
*/
- if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
- max_early_idx >= 0) {
+ if (hits <= misses && max_early_idx >= 0) {
idx = max_early_idx;
duration_us = drv->states[idx].target_residency;
}
@@ -316,10 +350,9 @@ static int teo_select(struct cpuidle_dri
if (idx < 0) {
idx = 0; /* No states enabled. Must use 0. */
} else if (idx > 0) {
+ unsigned int count = 0;
u64 sum = 0;
- count = 0;
-
/*
* Count and sum the most recent idle duration values less than
* the current expected idle duration value.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-08 23:19 ` Rafael J. Wysocki
@ 2019-10-09 13:36 ` Rafael J. Wysocki
2019-10-10 7:05 ` Doug Smythies
0 siblings, 1 reply; 15+ messages in thread
From: Rafael J. Wysocki @ 2019-10-09 13:36 UTC (permalink / raw)
To: Doug Smythies
Cc: Srinivas Pandruvada, Peter Zijlstra, LKML, Frederic Weisbecker,
Mel Gorman, Daniel Lezcano, Chen, Hu, Quentin Perret, Linux PM,
Giovanni Gherdovich
On Wednesday, October 9, 2019 1:19:51 AM CEST Rafael J. Wysocki wrote:
> On Tuesday, October 8, 2019 12:49:01 PM CEST Rafael J. Wysocki wrote:
> > On Tue, Oct 8, 2019 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Tue, Oct 8, 2019 at 8:20 AM Doug Smythies <dsmythies@telus.net> wrote:
> > > >
> > > > On 2019.10.06 08:34 Rafael J. Wysocki wrote:
> > > > > On Sun, Oct 6, 2019 at 4:46 PM Doug Smythies <dsmythies@telus.net> wrote:
> > > > >> On 2019.10.01 02:32 Rafael J. Wysocki wrote:
> > > > >>> On Sun, Sep 29, 2019 at 6:05 PM Doug Smythies <dsmythies@telus.net> wrote:
> > > > >>>> On 2019.09.26 09:32 Doug Smythies wrote:
> > > > >>>>
> > > > >>>>> If the deepest idle state is disabled, the system
> > > > >>>>> can become somewhat unstable, with anywhere between no problem
> > > > >>>>> at all, to the occasional temporary jump using a lot more
> > > > >>>>> power for a few seconds, to a permanent jump using a lot more
> > > > >>>>> power continuously. I have been unable to isolate the exact
> > > > >>>>> test load conditions under which this will occur. However,
> > > > >>>>> temporarily disabling and then enabling other idle states
> > > > >>>>> seems to make for a somewhat repeatable test. It is important
> > > > >>>>> to note that the issue occurs with only ever disabling the deepest
> > > > >>>>> idle state, just not reliably.
> > > > >>>>>
> > > > >>>>> I want to know how you want to proceed before I do a bunch of
> > > > >>>>> regression testing.
> > > > >>>>
> > > > >> I do not think I stated it clearly before: The problem here is that some CPUs
> > > > >> seem to get stuck in idle state 0, and when they do power consumption spikes,
> > > > >> often by several hundred % and often indefinitely.
> > > > >
> > > > > That indeed has not been clear to me, thanks for the clarification!
> > > >
> > > > >
> > > > >> I made a hack job automated test:
> > > > >> Kernel tests fail rate
> > > > >> 5.4-rc1 6616 13.45%
> > > > >> 5.3 2376 4.50%
> > > > >> 5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
> > > > >> 5.4-rc1-ds 11168 0.00% <<< [old] proposed patch (> 7 hours test time)
> > > >
> > > >
> > > > 5.4-rc1-ds12 4224 0.005 <<< new proposed patch
> > > >
> > > > >>
> > > > >> [old] Proposed patch (on top of kernel 5.4-rc1): [deleted]
> > > >
> > > > > This change may cause the deepest state to be selected even if its
> > > > > "hits" metric is less than the "misses" one AFAICS, in which case the
> > > > > max_early_index state should be selected instead.
> > > > >
> > > > > It looks like the max_early_index computation is broken when the
> > > > > deepest state is disabled.
> > > >
> > > > O.K. Thanks for your quick reply, and insight.
> > > >
> > > > I think long durations always need to be counted, but currently if
> > > > the deepest idle state is disabled, they are not.
> > > > How about this?:
> > > > (test results added above, more tests pending if this might be a path forward.)
> > > >
> > > > diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> > > > index b5a0e49..a970d2c 100644
> > > > --- a/drivers/cpuidle/governors/teo.c
> > > > +++ b/drivers/cpuidle/governors/teo.c
> > > > @@ -155,10 +155,12 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> > > >
> > > > cpu_data->states[i].early_hits -= early_hits >> DECAY_SHIFT;
> > > >
> > > > - if (drv->states[i].target_residency <= sleep_length_us) {
> > > > - idx_timer = i;
> > > > - if (drv->states[i].target_residency <= measured_us)
> > > > - idx_hit = i;
> > > > + if (!(drv->states[i].disabled || dev->states_usage[i].disable)){
> > > > + if (drv->states[i].target_residency <= sleep_length_us) {
> > > > + idx_timer = i;
> > > > + if (drv->states[i].target_residency <= measured_us)
> > > > + idx_hit = i;
> > > > + }
> > >
> > > What if the state is enabled again after some time?
> >
> > Actually, the states are treated as "bins" here, so for the metrics it
> > doesn't matter whether or not they are enabled at the moment.
> >
> > > > }
> > > > }
> > > >
> > > > @@ -256,39 +258,25 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> > > > struct cpuidle_state *s = &drv->states[i];
> > > > struct cpuidle_state_usage *su = &dev->states_usage[i];
> > > >
> > > > - if (s->disabled || su->disable) {
> > > > - /*
> > > > - * If the "early hits" metric of a disabled state is
> > > > - * greater than the current maximum, it should be taken
> > > > - * into account, because it would be a mistake to select
> > > > - * a deeper state with lower "early hits" metric. The
> > > > - * index cannot be changed to point to it, however, so
> > > > - * just increase the max count alone and let the index
> > > > - * still point to a shallower idle state.
> > > > - */
> > > > - if (max_early_idx >= 0 &&
> > > > - count < cpu_data->states[i].early_hits)
> > > > - count = cpu_data->states[i].early_hits;
> > > > -
> > > > - continue;
> > >
> > > AFAICS, adding early_hits to count is not a mistake if there are still
> > > enabled states deeper than the current one.
> >
> > And the mistake appears to be that the "hits" and "misses" metrics
> > aren't handled in analogy with the "early_hits" one when the current
> > state is disabled.
> >
> > Let me try to cut a patch to address that.
>
> Appended below, not tested.
>
> It is meant to address two problems, one of which is that the "hits" and
> "misses" metrics of disabled states need to be taken into account too in
> some cases, and the other is an issue with the handling of "early hits"
> which may lead to suboptimal state selection if some states are disabled.
Well, it still misses a couple of points.
First, disable states that are too deep should not be taken into consideration
at all.
Second, the "hits" and "misses" metrics of disabled states need to be used for
idle duration ranges corresponding to them regardless of whether or not the
"hits" value is greater than the "misses" one.
Updated patch is below (still not tested), but it tries to do too much in one
go, so I need to split it into a series of smaller changes.
---
drivers/cpuidle/governors/teo.c | 78 ++++++++++++++++++++++++++++++----------
1 file changed, 59 insertions(+), 19 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/teo.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/teo.c
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -233,7 +233,7 @@ static int teo_select(struct cpuidle_dri
{
struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
int latency_req = cpuidle_governor_latency_req(dev->cpu);
- unsigned int duration_us, count;
+ unsigned int duration_us, hits, misses, early_hits;
int max_early_idx, constraint_idx, idx, i;
ktime_t delta_tick;
@@ -247,7 +247,9 @@ static int teo_select(struct cpuidle_dri
cpu_data->sleep_length_ns = tick_nohz_get_sleep_length(&delta_tick);
duration_us = ktime_to_us(cpu_data->sleep_length_ns);
- count = 0;
+ hits = 0;
+ misses = 0;
+ early_hits = 0;
max_early_idx = -1;
constraint_idx = drv->state_count;
idx = -1;
@@ -258,23 +260,61 @@ static int teo_select(struct cpuidle_dri
if (s->disabled || su->disable) {
/*
- * If the "early hits" metric of a disabled state is
- * greater than the current maximum, it should be taken
- * into account, because it would be a mistake to select
- * a deeper state with lower "early hits" metric. The
- * index cannot be changed to point to it, however, so
- * just increase the max count alone and let the index
- * still point to a shallower idle state.
+ * Ignore disabled states with target residencies beyond
+ * the anticipated idle duration.
*/
- if (max_early_idx >= 0 &&
- count < cpu_data->states[i].early_hits)
- count = cpu_data->states[i].early_hits;
+ if (s->target_residency > duration_us)
+ continue;
+
+ /*
+ * This state is disabled, so the range of idle duration
+ * values corresponding to it is covered by the current
+ * candidate state, but still the "hits" and "misses"
+ * metrics of the disabled state need to be used to
+ * decide whether or not the state covering the range in
+ * question is good enough.
+ */
+ hits = cpu_data->states[i].hits;
+ misses = cpu_data->states[i].misses;
+
+ if (early_hits >= cpu_data->states[i].early_hits ||
+ idx < 0)
+ continue;
+
+ /*
+ * If the current candidate state has been the one with
+ * the maximum "early hits" metric so far, the "early
+ * hits" metric of the disabled state replaces the
+ * current "early hits" count to avoid selecting a
+ * deeper state with lower "early hits" metric.
+ */
+ if (max_early_idx == idx) {
+ early_hits = cpu_data->states[i].early_hits;
+ continue;
+ }
+
+ /*
+ * The current candidate state is closer to the disabled
+ * one than the current maximum "early hits" state, so
+ * replace the latter with it, but in case the maximum
+ * "early hits" state index has not been set so far,
+ * check if the current candidate state is not too
+ * shallow for that role.
+ */
+ if (!(tick_nohz_tick_stopped() &&
+ drv->states[idx].target_residency < TICK_USEC)) {
+ early_hits = cpu_data->states[i].early_hits;
+ max_early_idx = idx;
+ }
continue;
}
- if (idx < 0)
+ if (idx < 0) {
idx = i; /* first enabled state */
+ hits = cpu_data->states[i].hits;
+ misses = cpu_data->states[i].misses;
+ }
if (s->target_residency > duration_us)
break;
@@ -283,11 +323,13 @@ static int teo_select(struct cpuidle_dri
constraint_idx = i;
idx = i;
+ hits = cpu_data->states[i].hits;
+ misses = cpu_data->states[i].misses;
- if (count < cpu_data->states[i].early_hits &&
+ if (early_hits < cpu_data->states[i].early_hits &&
!(tick_nohz_tick_stopped() &&
drv->states[i].target_residency < TICK_USEC)) {
- count = cpu_data->states[i].early_hits;
+ early_hits = cpu_data->states[i].early_hits;
max_early_idx = i;
}
}
@@ -300,8 +342,7 @@ static int teo_select(struct cpuidle_dri
* "early hits" metric, but if that cannot be determined, just use the
* state selected so far.
*/
- if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
- max_early_idx >= 0) {
+ if (hits <= misses && max_early_idx >= 0) {
idx = max_early_idx;
duration_us = drv->states[idx].target_residency;
}
@@ -316,10 +357,9 @@ static int teo_select(struct cpuidle_dri
if (idx < 0) {
idx = 0; /* No states enabled. Must use 0. */
} else if (idx > 0) {
+ unsigned int count = 0;
u64 sum = 0;
- count = 0;
-
/*
* Count and sum the most recent idle duration values less than
* the current expected idle duration value.
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-09 13:36 ` Rafael J. Wysocki
@ 2019-10-10 7:05 ` Doug Smythies
2019-10-10 8:42 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Doug Smythies @ 2019-10-10 7:05 UTC (permalink / raw)
To: 'Rafael J. Wysocki'
Cc: 'Srinivas Pandruvada', 'Peter Zijlstra',
'LKML', 'Frederic Weisbecker',
'Mel Gorman', 'Daniel Lezcano',
'Chen, Hu', 'Quentin Perret', 'Linux PM',
'Giovanni Gherdovich'
On 2019.10.09 06:37 Rafael J. Wysocki wrote:
> On Wednesday, October 9, 2019 1:19:51 AM CEST Rafael J. Wysocki wrote:
>> On Tuesday, October 8, 2019 12:49:01 PM CEST Rafael J. Wysocki wrote:
>>> On Tue, Oct 8, 2019 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> On Tue, Oct 8, 2019 at 8:20 AM Doug Smythies <dsmythies@telus.net> wrote:
>>>>> O.K. Thanks for your quick reply, and insight.
>>>>>
>>>>> I think long durations always need to be counted, but currently if
>>>>> the deepest idle state is disabled, they are not.
...
>>>> AFAICS, adding early_hits to count is not a mistake if there are still
>>>> enabled states deeper than the current one.
>>>
>>> And the mistake appears to be that the "hits" and "misses" metrics
>>> aren't handled in analogy with the "early_hits" one when the current
>>> state is disabled.
I only know how to exploit and test the "hits" and "misses" path
that should use the deepest available idle state upon transition
to an idle system. Even so, the test has a low probability of
failing, and so needs to be run many times.
I do not know how to demonstrate and/or test any "early_hits" path
to confirm that an issue exists or that it is fixed.
>>>
>>> Let me try to cut a patch to address that.
>>
>> Appended below, not tested.
Reference as: rjw1
>>
>> It is meant to address two problems, one of which is that the "hits" and
>> "misses" metrics of disabled states need to be taken into account too in
>> some cases, and the other is an issue with the handling of "early hits"
>> which may lead to suboptimal state selection if some states are disabled.
>
> Well, it still misses a couple of points.
>
> First, disable states that are too deep should not be taken into consideration
> at all.
>
> Second, the "hits" and "misses" metrics of disabled states need to be used for
> idle duration ranges corresponding to them regardless of whether or not the
> "hits" value is greater than the "misses" one.
>
> Updated patch is below (still not tested), but it tries to do too much in one
> go, so I need to split it into a series of smaller changes.
Thanks for your continued look at this.
Reference as: rjw2
Test 1, hack job statistical test (old tests re-stated):
Kernel tests fail rate
5.4-rc1 6616 13.45%
5.3 2376 4.50%
5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
5.4-rc1-ds 11168 0.00% <<< [old] ds proposed patch (> 7 hours test time)
5.4-rc1-ds12 4224 0.00% <<< [old] new ds proposed patch
5.4-rc2-rjw1 11280 0.00%
5.4-rc2-rjw2 640 0.00% <<< Will be run again, for longer.
Test 2: I also looked at every possible enable/disable idle combination,
and they all seemed O.K.
No other tests have been run yet.
System:
Processor: i7-2600K
Deepest idle state: 4 (C6)
... Doug
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2019-10-10 7:05 ` Doug Smythies
@ 2019-10-10 8:42 ` Rafael J. Wysocki
0 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2019-10-10 8:42 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Srinivas Pandruvada, Peter Zijlstra, LKML,
Frederic Weisbecker, Mel Gorman, Daniel Lezcano, Chen, Hu,
Quentin Perret, Linux PM, Giovanni Gherdovich
On Thu, Oct 10, 2019 at 9:05 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2019.10.09 06:37 Rafael J. Wysocki wrote:
> > On Wednesday, October 9, 2019 1:19:51 AM CEST Rafael J. Wysocki wrote:
> >> On Tuesday, October 8, 2019 12:49:01 PM CEST Rafael J. Wysocki wrote:
> >>> On Tue, Oct 8, 2019 at 11:51 AM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>>> On Tue, Oct 8, 2019 at 8:20 AM Doug Smythies <dsmythies@telus.net> wrote:
> >>>>> O.K. Thanks for your quick reply, and insight.
> >>>>>
> >>>>> I think long durations always need to be counted, but currently if
> >>>>> the deepest idle state is disabled, they are not.
> ...
> >>>> AFAICS, adding early_hits to count is not a mistake if there are still
> >>>> enabled states deeper than the current one.
> >>>
> >>> And the mistake appears to be that the "hits" and "misses" metrics
> >>> aren't handled in analogy with the "early_hits" one when the current
> >>> state is disabled.
>
> I only know how to exploit and test the "hits" and "misses" path
> that should use the deepest available idle state upon transition
> to an idle system. Even so, the test has a low probability of
> failing, and so needs to be run many times.
>
> I do not know how to demonstrate and/or test any "early_hits" path
> to confirm that an issue exists or that it is fixed.
>
> >>>
> >>> Let me try to cut a patch to address that.
> >>
> >> Appended below, not tested.
>
> Reference as: rjw1
>
> >>
> >> It is meant to address two problems, one of which is that the "hits" and
> >> "misses" metrics of disabled states need to be taken into account too in
> >> some cases, and the other is an issue with the handling of "early hits"
> >> which may lead to suboptimal state selection if some states are disabled.
> >
> > Well, it still misses a couple of points.
> >
> > First, disable states that are too deep should not be taken into consideration
> > at all.
> >
> > Second, the "hits" and "misses" metrics of disabled states need to be used for
> > idle duration ranges corresponding to them regardless of whether or not the
> > "hits" value is greater than the "misses" one.
> >
> > Updated patch is below (still not tested), but it tries to do too much in one
> > go, so I need to split it into a series of smaller changes.
>
> Thanks for your continued look at this.
>
> Reference as: rjw2
>
> Test 1, hack job statistical test (old tests re-stated):
>
> Kernel tests fail rate
> 5.4-rc1 6616 13.45%
> 5.3 2376 4.50%
> 5.3-teov7 12136 0.00% <<< teo.c reverted and teov7 put in its place.
> 5.4-rc1-ds 11168 0.00% <<< [old] ds proposed patch (> 7 hours test time)
> 5.4-rc1-ds12 4224 0.00% <<< [old] new ds proposed patch
> 5.4-rc2-rjw1 11280 0.00%
> 5.4-rc2-rjw2 640 0.00% <<< Will be run again, for longer.
>
> Test 2: I also looked at every possible enable/disable idle combination,
> and they all seemed O.K.
>
> No other tests have been run yet.
>
> System:
> Processor: i7-2600K
> Deepest idle state: 4 (C6)
Thanks a lot for sharing the results!
Cheers,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
2018-12-17 1:53 Doug Smythies
@ 2018-12-17 11:59 ` Rafael J. Wysocki
0 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2018-12-17 11:59 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Giovanni Gherdovich, Srinivas Pandruvada,
Peter Zijlstra, Linux Kernel Mailing List, Frederic Weisbecker,
Mel Gorman, Daniel Lezcano, hu1.chen, Quentin Perret, Linux PM
On Mon, Dec 17, 2018 at 2:53 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2018.12.11 03:50 Rafael J. Wysocki wrote:
>
> ...[snip]...
>
> > With this version of the TEO governor I'm observing slight, but consistent
> > performance improvements in some different benchmarks on a few different
> > systems with respect to menu
>
> Same here.
>
> > and the corresponding power draw differences
> > appear to be in the noise. The idle power doesn't seem to change either.
>
> Same here.
>
> > Also the "above" and "below" metrics (introduced by a separate patch under
> > discussion) indicate changes in the "good" direction.
>
> My graphs now include the "above" and "below" metrics.
> In particular see Idle State 1 "above" (was too deep) graphs in the links below.
> However, performance is up and power about the same, so O.K.
Thanks a lot for the comprehensive results, much appreciated as always!
This basically confirms my own observations, so my overall conclusion
is that what we have here is as good as it can get without changing
the approach entirely or adding complications that would be difficult
to justify in general.
> > Overall, I like this one, so I may consider dropping the RFC/RFT tag from the
> > next submission. :-)
And so that's what I'm going to do.
Cheers,
Rafael
^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
@ 2018-12-17 1:53 Doug Smythies
2018-12-17 11:59 ` Rafael J. Wysocki
0 siblings, 1 reply; 15+ messages in thread
From: Doug Smythies @ 2018-12-17 1:53 UTC (permalink / raw)
To: 'Rafael J. Wysocki', 'Giovanni Gherdovich'
Cc: 'Srinivas Pandruvada', 'Peter Zijlstra',
'LKML', 'Frederic Weisbecker',
'Mel Gorman', 'Daniel Lezcano',
'Chen, Hu', 'Quentin Perret', 'Linux PM',
Doug Smythies
On 2018.12.11 03:50 Rafael J. Wysocki wrote:
...[snip]...
> With this version of the TEO governor I'm observing slight, but consistent
> performance improvements in some different benchmarks on a few different
> systems with respect to menu
Same here.
> and the corresponding power draw differences
> appear to be in the noise. The idle power doesn't seem to change either.
Same here.
> Also the "above" and "below" metrics (introduced by a separate patch under
> discussion) indicate changes in the "good" direction.
My graphs now include the "above" and "below" metrics.
In particular see Idle State 1 "above" (was too deep) graphs in the links below.
However, performance is up and power about the same, so O.K.
> Overall, I like this one, so I may consider dropping the RFC/RFT tag from the
> next submission. :-)
>
> v7 -> v8:
> * Apply the selection rules to the idle deepest state as well as to
> the shallower ones (the deepest idle state was treated differently
> before by mistake).
> * Subtract 1/2 of the exit latency from the measured idle duration
> in teo_update() (instead of subtracting the entire exit latency).
> This makes the idle state selection be slightly more performance-
> oriented.
I cherry picked a couple of the mmtests that Giovanni was doing:
Test kernels:
"stock" kernel 4.20-rc5 + a couple of rjw patches. Specifically:
2a110ed cpuidle: poll_state: Disregard disable idle states
8f09875 cpuidle: Add 'above' and 'below' idle state metrics
d6851a5 Documentation: admin-guide: PM: Add cpuidle document
2595646 Linux 4.20-rc5
"teov6" above + teov6 patch
"teov7" above + teov7 patch
"teov8" above + teov8 patch
1.) mmtests - netperf-unbound test (UDP):
4.20-rc5 4.20-rc5 4.20-rc5 4.20-rc5
stock teo6 teo7 teo8
Hmean send-64 129.64 ( 0.00%) 132.45 * 2.17%* 130.55 * 0.71%* 132.87 * 2.49%*
Hmean send-128 259.53 ( 0.00%) 264.90 * 2.07%* 261.61 * 0.80%* 264.94 * 2.09%*
Hmean send-256 515.24 ( 0.00%) 525.41 * 1.97%* 517.41 * 0.42%* 524.88 * 1.87%*
Hmean send-1024 2041.01 ( 0.00%) 2079.22 * 1.87%* 2045.03 * 0.20%* 2077.25 * 1.78%*
Hmean send-2048 3980.04 ( 0.00%) 4071.09 * 2.29%* 4041.15 * 1.54%* 4057.09 * 1.94%*
Hmean send-3312 6321.90 ( 0.00%) 6460.23 * 2.19%* 6395.71 * 1.17%* 6409.09 * 1.38%*
Hmean send-4096 7695.18 ( 0.00%) 7882.81 * 2.44%* 7813.72 * 1.54%* 7832.77 * 1.79%*
Hmean send-8192 13920.53 ( 0.00%) 14146.47 * 1.62%* 13986.72 * 0.48%* 14079.07 * 1.14%*
Hmean send-16384 24714.99 ( 0.00%) 25225.95 * 2.07%* 24896.10 * 0.73%* 25131.52 * 1.69%*
Hmean recv-64 129.64 ( 0.00%) 132.45 * 2.17%* 130.55 * 0.71%* 132.87 * 2.49%*
Hmean recv-128 259.53 ( 0.00%) 264.90 * 2.07%* 261.61 * 0.80%* 264.94 * 2.09%*
Hmean recv-256 515.24 ( 0.00%) 525.41 * 1.97%* 517.41 * 0.42%* 524.88 * 1.87%*
Hmean recv-1024 2041.01 ( 0.00%) 2079.22 * 1.87%* 2045.03 * 0.20%* 2077.25 * 1.78%*
Hmean recv-2048 3980.04 ( 0.00%) 4071.09 * 2.29%* 4041.15 * 1.54%* 4057.09 * 1.94%*
Hmean recv-3312 6321.88 ( 0.00%) 6460.23 * 2.19%* 6395.71 * 1.17%* 6409.09 * 1.38%*
Hmean recv-4096 7695.15 ( 0.00%) 7882.81 * 2.44%* 7813.72 * 1.54%* 7832.75 * 1.79%*
Hmean recv-8192 13920.52 ( 0.00%) 14146.43 * 1.62%* 13986.72 * 0.48%* 14079.07 * 1.14%*
Hmean recv-16384 24714.99 ( 0.00%) 25225.90 * 2.07%* 24896.07 * 0.73%* 25131.49 * 1.69%*
Graphs: http://www.smythies.com/~doug/linux/idle/teo8/net-pref-udp-unbound/index.html
(note: slow upload speed from my server)
2.) mmtests - sockperf-udp-throughput test:
4.20-rc5 4.20-rc5 4.20-rc5 4.20-rc5
stock teo6 teo7 teo8
Hmean 14 24.57 ( 0.00%) 25.91 * 5.46%* 25.99 * 5.78%* 25.73 * 4.75%*
Hmean 100 175.37 ( 0.00%) 185.09 * 5.54%* 185.89 * 6.00%* 184.48 * 5.19%*
Hmean 300 523.81 ( 0.00%) 553.47 * 5.66%* 554.70 * 5.90%* 550.16 * 5.03%*
Hmean 500 870.08 ( 0.00%) 918.88 * 5.61%* 924.33 * 6.24%* 914.53 * 5.11%*
Hmean 850 1449.44 ( 0.00%) 1530.84 * 5.62%* 1535.40 * 5.93%* 1522.53 * 5.04%*
Graphs: http://www.smythies.com/~doug/linux/idle/teo8/sockperf-udp-throughput/index.html
(note: slow upload speed from my server)
The above results tables are also here:
http://www.smythies.com/~doug/linux/idle/teo8/index.html
I wanted to also do the tbench on loopback test, but have not been able to get it working on my system yet.
I'll supply more test results at a later date.
... Doug
^ permalink raw reply [flat|nested] 15+ messages in thread
* [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems
@ 2018-12-11 11:49 Rafael J. Wysocki
0 siblings, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2018-12-11 11:49 UTC (permalink / raw)
To: Linux PM, Giovanni Gherdovich, Doug Smythies
Cc: Srinivas Pandruvada, Peter Zijlstra, LKML, Frederic Weisbecker,
Mel Gorman, Daniel Lezcano, Chen, Hu, Quentin Perret
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
The venerable menu governor does some thigns that are quite
questionable in my view.
First, it includes timer wakeups in the pattern detection data and
mixes them up with wakeups from other sources which in some cases
causes it to expect what essentially would be a timer wakeup in a
time frame in which no timer wakeups are possible (becuase it knows
the time until the next timer event and that is later than the
expected wakeup time).
Second, it uses the extra exit latency limit based on the predicted
idle duration and depending on the number of tasks waiting on I/O,
even though those tasks may run on a different CPU when they are
woken up. Moreover, the time ranges used by it for the sleep length
correction factors depend on whether or not there are tasks waiting
on I/O, which again doesn't imply anything in particular, and they
are not correlated to the list of available idle states in any way
whatever.
Also, the pattern detection code in menu may end up considering
values that are too large to matter at all, in which cases running
it is a waste of time.
A major rework of the menu governor would be required to address
these issues and the performance of at least some workloads (tuned
specifically to the current behavior of the menu governor) is likely
to suffer from that. It is thus better to introduce an entirely new
governor without them and let everybody use the governor that works
better with their actual workloads.
The new governor introduced here, the timer events oriented (TEO)
governor, uses the same basic strategy as menu: it always tries to
find the deepest idle state that can be used in the given conditions.
However, it applies a different approach to that problem.
First, it doesn't use "correction factors" for the time till the
closest timer, but instead it tries to correlate the measured idle
duration values with the available idle states and use that
information to pick up the idle state that is most likely to "match"
the upcoming CPU idle interval.
Second, it doesn't take the number of "I/O waiters" into account at
all and the pattern detection code in it avoids taking timer wakeups
into account. It also only uses idle duration values less than the
current time till the closest timer (with the tick excluded) for that
purpose.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
With this version of the TEO governor I'm observing slight, but consistent
performance improvements in some different benchmarks on a few different
systems with respect to menu and the corresponding power draw differences
appear to be in the noise. The idle power doesn't seem to change either.
Also the "above" and "below" metrics (introduced by a separate patch under
discussion) indicate changes in the "good" direction.
Overall, I like this one, so I may consider dropping the RFC/RFT tag from the
next submission. :-)
v7 -> v8:
* Apply the selection rules to the idle deepest state as well as to
the shallower ones (the deepest idle state was treated differently
before by mistake).
* Subtract 1/2 of the exit latency from the measured idle duration
in teo_update() (instead of subtracting the entire exit latency).
This makes the idle state selection be slightly more performance-
oriented.
v6 -> v7:
* Actually use idle duration measured by the governor for everything (as
it likely is more accurate than the one measured by the core).
* Fix teo_find_shallower_state() to work if state 0 is disabled (among
other cases).
* Avoid using the most recent values of observed idle duration to refine
the state selection unless the majority of them are below the target
residency of the candidate idle state selected so far. [This reduces
the likelihood of triggering the refinement computation, so it may also
reduce the overhead slightly (on average).]
* Do not disregard the maximum observed idle duration falling in the
interesting range. [This should cause slightly deeper idle state to be
selected through the selection refinement.]
v5 -> v6:
* Avoid applying poll_time_limit to non-polling idle states by mistake.
* Rename SPIKE to PULSE.
* Do not run pattern detection upfront. Instead, use recent idle duration
values to refine the state selection after finding a candidate idle state.
* Do not use the expected idle duration as an extra latency constraint
(exit latency is less than the target residency for all of the idle states
known to me anyway, so this doesn't change anything in practice).
v4 -> v5:
* Avoid using shallow idle states when the tick has been stopped already.
v3 -> v4:
* Make the pattern detection avoid returning too early if the minimum
sample is too far from the average.
* Reformat the changelog (as requested by Peter).
v2 -> v3:
* Simplify the pattern detection code and make it return a value
lower than the time to the closest timer if the majority of recent
idle intervals are below it regardless of their variance (that should
cause it to be slightly more aggressive).
* Do not count wakeups from state 0 due to the time limit in poll_idle()
as non-timer.
---
drivers/cpuidle/Kconfig | 11
drivers/cpuidle/governors/Makefile | 1
drivers/cpuidle/governors/teo.c | 443 +++++++++++++++++++++++++++++++++++++
3 files changed, 455 insertions(+)
Index: linux-pm/drivers/cpuidle/governors/teo.c
===================================================================
--- /dev/null
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -0,0 +1,443 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Timer events oriented CPU idle governor
+ *
+ * Copyright (C) 2018 Intel Corporation
+ * Author: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
+ *
+ * The idea of this governor is based on the observation that on many systems
+ * timer events are two or more orders of magnitude more frequent than any
+ * other interrupts, so they are likely to be the most significant source of CPU
+ * wakeups from idle states. Moreover, information about what happened in the
+ * (relatively recent) past can be used to estimate whether or not the deepest
+ * idle state with target residency within the time to the closest timer is
+ * likely to be suitable for the upcoming idle time of the CPU and, if not, then
+ * which of the shallower idle states to choose.
+ *
+ * Of course, non-timer wakeup sources are more important in some use cases and
+ * they can be covered by taking a few most recent idle time intervals of the
+ * CPU into account. However, even in that case it is not necessary to consider
+ * idle duration values greater than the time till the closest timer, as the
+ * patterns that they may belong to produce average values close enough to
+ * the time till the closest timer (sleep length) anyway.
+ *
+ * Thus this governor estimates whether or not the upcoming idle time of the CPU
+ * is likely to be significantly shorter than the sleep length and selects an
+ * idle state for it in accordance with that, as follows:
+ *
+ * - Find an idle state on the basis of the sleep length and state statistics
+ * collected over time:
+ *
+ * o Find the deepest idle state whose target residency is less than or euqal
+ * to the sleep length.
+ *
+ * o Select it if it matched both the sleep length and the observed idle
+ * duration in the past more often than it matched the sleep length alone
+ * (i.e. the observed idle duration was significantly shorter than the sleep
+ * length matched by it).
+ *
+ * o Otherwise, select the shallower state with the greatest matched "early"
+ * wakeups metric.
+ *
+ * - If the majority of the most recent idle duration values are below the
+ * target residency of the idle state selected so far, use those values to
+ * compute the new expected idle duration and find an idle state matching it
+ * (which has to be shallower than the one selected so far).
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/jiffies.h>
+#include <linux/kernel.h>
+#include <linux/sched/clock.h>
+#include <linux/tick.h>
+
+/*
+ * The PULSE value is added to metrics when they grow and the DECAY_SHIFT value
+ * is used for decreasing metrics on a regular basis.
+ */
+#define PULSE 1024
+#define DECAY_SHIFT 3
+
+/*
+ * Number of the most recent idle duration values to take into consideration for
+ * the detection of wakeup patterns.
+ */
+#define INTERVALS 8
+
+/**
+ * struct teo_idle_state - Idle state data used by the TEO cpuidle governor.
+ * @early_hits: "Early" CPU wakeups matched by this state.
+ * @hits: "On time" CPU wakeups matched by this state.
+ * @misses: CPU wakeups "missed" by this state.
+ *
+ * A CPU wakeup is "matched" by a given idle state if the idle duration measured
+ * after the wakeup is between the target residency of that state and the target
+ * residnecy of the next one (or if this is the deepest available idle state, it
+ * "matches" a CPU wakeup when the measured idle duration is at least equal to
+ * its target residency).
+ *
+ * Also, from the TEO governor prespective, a CPU wakeup from idle is "early" if
+ * it occurs significantly earlier than the closest expected timer event (that
+ * is, early enough to match an idle state shallower than the one matching the
+ * time till the closest timer event). Otherwise, the wakeup is "on time", or
+ * it is a "hit".
+ *
+ * A "miss" occurs when the given state doesn't match the wakeup, but it matches
+ * the time till the closest timer event used for idle state selection.
+ */
+struct teo_idle_state {
+ unsigned int early_hits;
+ unsigned int hits;
+ unsigned int misses;
+};
+
+/**
+ * struct teo_cpu - CPU data used by the TEO cpuidle governor.
+ * @time_span_ns: Time between idle state selection and post-wakeup update.
+ * @sleep_length_ns: Time till the closest timer event (at the selection time).
+ * @states: Idle states data corresponding to this CPU.
+ * @last_state: Idle state entered by the CPU last time.
+ * @interval_idx: Index of the most recent saved idle interval.
+ * @intervals: Saved idle duration values.
+ */
+struct teo_cpu {
+ u64 time_span_ns;
+ u64 sleep_length_ns;
+ struct teo_idle_state states[CPUIDLE_STATE_MAX];
+ int last_state;
+ int interval_idx;
+ unsigned int intervals[INTERVALS];
+};
+
+static DEFINE_PER_CPU(struct teo_cpu, teo_cpus);
+
+/**
+ * teo_update - Update CPU data after wakeup.
+ * @drv: cpuidle driver containing state data.
+ * @dev: Target CPU.
+ */
+static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+{
+ struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
+ unsigned int sleep_length_us = ktime_to_us(cpu_data->sleep_length_ns);
+ int i, idx_hit = -1, idx_timer = -1;
+ unsigned int measured_us;
+
+ if (cpu_data->time_span_ns >= cpu_data->sleep_length_ns) {
+ /*
+ * One of the safety nets has triggered or this was a timer
+ * wakeup (or equivalent).
+ */
+ measured_us = sleep_length_us;
+ } else {
+ unsigned int lat = drv->states[cpu_data->last_state].exit_latency;
+
+ measured_us = ktime_to_us(cpu_data->time_span_ns);
+ /*
+ * The delay between the wakeup and the first instruction
+ * executed by the CPU is not likely to be worst-case every
+ * time, so take 1/2 of the exit latency as a very rough
+ * approximation of the average of it.
+ */
+ if (measured_us >= lat)
+ measured_us -= lat / 2;
+ else
+ measured_us /= 2;
+ }
+
+ /*
+ * Decay the "early hits" metric for all of the states and find the
+ * states matching the sleep length and the measured idle duration.
+ */
+ for (i = 0; i < drv->state_count; i++) {
+ unsigned int early_hits = cpu_data->states[i].early_hits;
+
+ cpu_data->states[i].early_hits -= early_hits >> DECAY_SHIFT;
+
+ if (drv->states[i].target_residency <= sleep_length_us) {
+ idx_timer = i;
+ if (drv->states[i].target_residency <= measured_us)
+ idx_hit = i;
+ }
+ }
+
+ /*
+ * Update the "hits" and "misses" data for the state matching the sleep
+ * length. If it matches the measured idle duration too, this is a hit,
+ * so increase the "hits" metric for it then. Otherwise, this is a
+ * miss, so increase the "misses" metric for it. In the latter case
+ * also increase the "early hits" metric for the state that actually
+ * matches the measured idle duration.
+ */
+ if (idx_timer >= 0) {
+ unsigned int hits = cpu_data->states[idx_timer].hits;
+ unsigned int misses = cpu_data->states[idx_timer].misses;
+
+ hits -= hits >> DECAY_SHIFT;
+ misses -= misses >> DECAY_SHIFT;
+
+ if (idx_timer > idx_hit) {
+ misses += PULSE;
+ if (idx_hit >= 0)
+ cpu_data->states[idx_hit].early_hits += PULSE;
+ } else {
+ hits += PULSE;
+ }
+
+ cpu_data->states[idx_timer].misses = misses;
+ cpu_data->states[idx_timer].hits = hits;
+ }
+
+ /*
+ * Save idle duration values corresponding to non-timer wakeups for
+ * pattern detection.
+ *
+ * If the total time span between idle state selection and the "reflect"
+ * callback is greater than or equal to the sleep length determined at
+ * the idle state selection time, the wakeup is likely to be due to a
+ * timer event.
+ */
+ if (cpu_data->time_span_ns >= cpu_data->sleep_length_ns)
+ measured_us = UINT_MAX;
+
+ cpu_data->intervals[cpu_data->interval_idx++] = measured_us;
+ if (cpu_data->interval_idx > INTERVALS)
+ cpu_data->interval_idx = 0;
+}
+
+/**
+ * teo_find_shallower_state - Find shallower idle state matching given duration.
+ * @drv: cpuidle driver containing state data.
+ * @dev: Target CPU.
+ * @state_idx: Index of the capping idle state.
+ * @duration_us: Idle duration value to match.
+ */
+static int teo_find_shallower_state(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev, int state_idx,
+ unsigned int duration_us)
+{
+ int i;
+
+ for (i = state_idx - 1; i >= 0; i--) {
+ if (drv->states[i].disabled || dev->states_usage[i].disable)
+ continue;
+
+ state_idx = i;
+ if (drv->states[i].target_residency <= duration_us)
+ break;
+ }
+ return state_idx;
+}
+
+/**
+ * teo_select - Selects the next idle state to enter.
+ * @drv: cpuidle driver containing state data.
+ * @dev: Target CPU.
+ * @stop_tick: Indication on whether or not to stop the scheduler tick.
+ */
+static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
+ bool *stop_tick)
+{
+ struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
+ int latency_req = cpuidle_governor_latency_req(dev->cpu);
+ unsigned int duration_us, count;
+ int max_early_idx, idx, i;
+ ktime_t delta_tick;
+
+ if (cpu_data->last_state >= 0) {
+ teo_update(drv, dev);
+ cpu_data->last_state = -1;
+ }
+
+ cpu_data->time_span_ns = local_clock();
+
+ cpu_data->sleep_length_ns = tick_nohz_get_sleep_length(&delta_tick);
+ duration_us = ktime_to_us(cpu_data->sleep_length_ns);
+
+ count = 0;
+ max_early_idx = -1;
+ idx = -1;
+
+ for (i = 0; i < drv->state_count; i++) {
+ struct cpuidle_state *s = &drv->states[i];
+ struct cpuidle_state_usage *su = &dev->states_usage[i];
+
+ if (s->disabled || su->disable) {
+ /*
+ * If the "early hits" metric of a disabled state is
+ * greater than the current maximum, it should be taken
+ * into account, because it would be a mistake to select
+ * a deeper state with lower "early hits" metric. The
+ * index cannot be changed to point to it, however, so
+ * just increase the max count alone and let the index
+ * still point to a shallower idle state.
+ */
+ if (max_early_idx >= 0 &&
+ count < cpu_data->states[i].early_hits)
+ count = cpu_data->states[i].early_hits;
+
+ continue;
+ }
+
+ if (idx < 0)
+ idx = i; /* first enabled state */
+
+ if (s->target_residency > duration_us)
+ break;
+
+ if (s->exit_latency > latency_req) {
+ /*
+ * If we break out of the loop for latency reasons, use
+ * the target residency of the selected state as the
+ * expected idle duration to avoid stopping the tick
+ * as long as that target residency is low enough.
+ */
+ duration_us = drv->states[idx].target_residency;
+ goto refine;
+ }
+
+ idx = i;
+
+ if (count < cpu_data->states[i].early_hits &&
+ !(tick_nohz_tick_stopped() &&
+ drv->states[i].target_residency < TICK_USEC)) {
+ count = cpu_data->states[i].early_hits;
+ max_early_idx = i;
+ }
+ }
+
+ /*
+ * If the "hits" metric of the idle state matching the sleep length is
+ * greater than its "misses" metric, that is the one to use. Otherwise,
+ * it is more likely that one of the shallower states will match the
+ * idle duration observed after wakeup, so take the one with the maximum
+ * "early hits" metric, but if that cannot be determined, just use the
+ * state selected so far.
+ */
+ if (cpu_data->states[idx].hits <= cpu_data->states[idx].misses &&
+ max_early_idx >= 0) {
+ idx = max_early_idx;
+ duration_us = drv->states[idx].target_residency;
+ }
+
+refine:
+ if (idx < 0) {
+ idx = 0; /* No states enabled. Must use 0. */
+ } else if (idx > 0) {
+ u64 sum = 0;
+
+ count = 0;
+
+ /*
+ * Count and sum the most recent idle duration values less than
+ * the target residency of the state selected so far, find the
+ * max.
+ */
+ for (i = 0; i < INTERVALS; i++) {
+ unsigned int val = cpu_data->intervals[i];
+
+ if (val >= drv->states[idx].target_residency)
+ continue;
+
+ count++;
+ sum += val;
+ }
+
+ /*
+ * Give up unless the majority of the most recent idle duration
+ * values are in the interesting range.
+ */
+ if (count > INTERVALS / 2) {
+ unsigned int avg_us = div64_u64(sum, count);
+
+ /*
+ * Avoid spending too much time in an idle state that
+ * would be too shallow.
+ */
+ if (!(tick_nohz_tick_stopped() && avg_us < TICK_USEC)) {
+ idx = teo_find_shallower_state(drv, dev, idx, avg_us);
+ duration_us = avg_us;
+ }
+ }
+ }
+
+ /*
+ * Don't stop the tick if the selected state is a polling one or if the
+ * expected idle duration is shorter than the tick period length.
+ */
+ if (((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) ||
+ duration_us < TICK_USEC) && !tick_nohz_tick_stopped()) {
+ unsigned int delta_tick_us = ktime_to_us(delta_tick);
+
+ *stop_tick = false;
+
+ /*
+ * The tick is not going to be stopped, so if the target
+ * residency of the state to be returned is not within the time
+ * till the closest timer including the tick, try to correct
+ * that.
+ */
+ if (idx > 0 && drv->states[idx].target_residency > delta_tick_us)
+ idx = teo_find_shallower_state(drv, dev, idx, delta_tick_us);
+ }
+
+ return idx;
+}
+
+/**
+ * teo_reflect - Note that governor data for the CPU need to be updated.
+ * @dev: Target CPU.
+ * @state: Entered state.
+ */
+static void teo_reflect(struct cpuidle_device *dev, int state)
+{
+ struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
+
+ cpu_data->last_state = state;
+ /*
+ * If the wakeup was not "natural", but triggered by one of the safety
+ * nets, assume that the CPU might have been idle for the entire sleep
+ * length time.
+ */
+ if (dev->poll_time_limit ||
+ (tick_nohz_idle_got_tick() && cpu_data->sleep_length_ns > TICK_NSEC)) {
+ dev->poll_time_limit = false;
+ cpu_data->time_span_ns = cpu_data->sleep_length_ns;
+ } else {
+ cpu_data->time_span_ns = local_clock() - cpu_data->time_span_ns;
+ }
+}
+
+/**
+ * teo_enable_device - Initialize the governor's data for the target CPU.
+ * @drv: cpuidle driver (not used).
+ * @dev: Target CPU.
+ */
+static int teo_enable_device(struct cpuidle_driver *drv,
+ struct cpuidle_device *dev)
+{
+ struct teo_cpu *cpu_data = per_cpu_ptr(&teo_cpus, dev->cpu);
+ int i;
+
+ memset(cpu_data, 0, sizeof(*cpu_data));
+
+ for (i = 0; i < INTERVALS; i++)
+ cpu_data->intervals[i] = UINT_MAX;
+
+ return 0;
+}
+
+static struct cpuidle_governor teo_governor = {
+ .name = "teo",
+ .rating = 22,
+ .enable = teo_enable_device,
+ .select = teo_select,
+ .reflect = teo_reflect,
+};
+
+static int __init teo_governor_init(void)
+{
+ return cpuidle_register_governor(&teo_governor);
+}
+
+postcore_initcall(teo_governor_init);
Index: linux-pm/drivers/cpuidle/Kconfig
===================================================================
--- linux-pm.orig/drivers/cpuidle/Kconfig
+++ linux-pm/drivers/cpuidle/Kconfig
@@ -23,6 +23,17 @@ config CPU_IDLE_GOV_LADDER
config CPU_IDLE_GOV_MENU
bool "Menu governor (for tickless system)"
+config CPU_IDLE_GOV_TEO
+ bool "Timer events oriented governor (for tickless systems)"
+ help
+ Menu governor derivative that uses a simplified idle state
+ selection method focused on timer events and does not do any
+ interactivity boosting.
+
+ Some workloads benefit from using this governor and it generally
+ should be safe to use. Say Y here if you are not happy with the
+ alternatives.
+
config DT_IDLE_STATES
bool
Index: linux-pm/drivers/cpuidle/governors/Makefile
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/Makefile
+++ linux-pm/drivers/cpuidle/governors/Makefile
@@ -4,3 +4,4 @@
obj-$(CONFIG_CPU_IDLE_GOV_LADDER) += ladder.o
obj-$(CONFIG_CPU_IDLE_GOV_MENU) += menu.o
+obj-$(CONFIG_CPU_IDLE_GOV_TEO) += teo.o
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-10-10 8:43 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26 16:31 [RFC/RFT][PATCH v8] cpuidle: New timer events oriented governor for tickless systems Doug Smythies
2019-09-29 16:04 ` Doug Smythies
2019-10-01 9:31 ` Rafael J. Wysocki
2019-10-06 14:46 ` Doug Smythies
2019-10-06 15:34 ` Rafael J. Wysocki
2019-10-08 6:20 ` Doug Smythies
2019-10-08 9:51 ` Rafael J. Wysocki
2019-10-08 10:49 ` Rafael J. Wysocki
2019-10-08 23:19 ` Rafael J. Wysocki
2019-10-09 13:36 ` Rafael J. Wysocki
2019-10-10 7:05 ` Doug Smythies
2019-10-10 8:42 ` Rafael J. Wysocki
-- strict thread matches above, loose matches on Subject: below --
2018-12-17 1:53 Doug Smythies
2018-12-17 11:59 ` Rafael J. Wysocki
2018-12-11 11:49 Rafael J. Wysocki
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).