| Message ID | 1867445.PYKUYFuaPT@kreacher |
|---|---|
| Headers | show |
| Series | cpuidle: teo: Rework the idle state selection logic | expand |
Hi Rafael, On 2021.06.02 11:14 Rafael J. Wysocki wrote: > Hi All, > > This series of patches addresses some theoretical shortcoming in the > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > state selection logic to some extent. > > Patches [1-2/5] are introductory cleanups and the substantial changes are > made in patches [3-4/5] (please refer to the changelogs of these two > patches for details). The last patch only deals with documentation. > > Even though this work is mostly based on theoretical considerations, it > shows a measurable reduction of the number of cases in which the shallowest > idle state is selected while it would be more beneficial to select a deeper > one or the deepest idle state is selected while it would be more beneficial to > select a shallower one, which should be a noticeable improvement. Do you have any test results to share? Or test methods that I can try? I have done a few tests, and generally don't notice much difference. Perhaps an increase in idle state 2 below (was to shallow) numbers. I am searching for some results that would offset the below: The difficulty I am having with this patch set is the additional overhead which becomes significant at the extremes, where idle state 0 is dominant. Throughout the history of teo, I have used multiple one core pipe-tests for this particular test. Some results: CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz HWP: disabled CPU frequency scaling driver: intel_pstate, active, powersave Pipe-tests are run forever, printing average loop time for the Last 2.5 million loops. 1021 of those are again averaged. Total = 2.5525e9 loops The power and idle data is sampled for 100 minutes. Note 1: other tests were also done and also with passive, schedutil, but it isn't relevant for this test because the CPU frequency stays pinned at maximum. Note 2: I use TCC offset for thermal throttling, but I disabled it for these tests, because the temperature needed to go higher than my normal throttling point. Idle configuration 1: As a COMETLAKE processor, with 4 idle states. Kernel 5.13-RC4. Before patch set average: 2.8014 uSec/loop 113.9 watts Idle state 0 residency: 9.450% Idle state 0 entries per minute: 256,812,896.6 After patch set average: 2.8264 uSec/loop, 0.89% slower 114.0 watts Idle state 0 residency: 8.677% Idle state 0 entries per minute: 254,560,049.9 Menu governor: 2.8051 uSec/loop, 0.13% slower 113.9 watts Idle state 0 residency: 8.437% Idle state 0 entries per minute: 256,436,417.2 O.K., perhaps not so bad, but also not many idle states. Idle configuration 2: As a SKYLAKE processor, with 9 idle states. i.e.: /drivers/idle/intel_idle.c static const struct x86_cpu_id intel_idle_ids[] __initconst ... X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, &idle_cpu_skx), + X86_MATCH_INTEL_FAM6_MODEL(COMETLAKE, &idle_cpu_skl), Purpose: To demonstrate increasing overhead as a function of number of idle states. Kernel 5.13. Before patch set average: 2.8394 uSec/loop 114.2 watts Idle state 0 residency: 7.212% Idle state 0 entries per minute: 253,391,954.3 After patch set average: 2.9103 uSec/loop, 2.5% slower 114.4 watts, 0.18% more Idle state 0 residency: 6.152%, 14.7% less. Idle state 0 entries per minute: 244,024,752.1 Menu governor: 2.8141 uSec/loop, 0.89% faster 113.9 watts, 0.26% less Idle state 0 residency: 7.167%, 0.6% less Idle state 0 entries per minute: 255,650,610.7 Another potentially interesting test was the ebizzy test: Records per second, averaged over many tests, varying threads and intervals: passive, schedutil: Before: 6771.977 After: 5502.643, -18.7% Menu: 10728.89, +58.4% Active, powersave: Before: 8361.82 After: 8463.31, +1.2% Menu: 8225.58, -1.6% I think it has more to do with CPU scaling governors than this patch set, so: performance: Before: 12137.33 After: 12083.26, -0.4% Menu: 11983.73, -1.3% These and other test results available here: (encoded to prevent a barrage of bots) double u double u double u dot smythies dot com /~doug/linux/idle/teo-2021-06/ ... a day later ... I might have an answer to my own question. By switching to cross core pipe-tests, and only loading down one CPU per core, I was able to get a lot more activity in other idle states. The test runs for 100 minutes, and the results change with time, but I'll leave that investigation for another day (there is no throttling): 1st 50 tests: Before: 3.888 uSec/loop After: 3.764 uSec/loop Menu: 3.464 uSec/loop Tests 50 to 100: Before: 4.329 uSec/loop After: 3.919 uSec/loop Menu: 3.514 uSec/loop Tests 200 to 250: Before: 5.089 uSec/loop After: 4.364 uSec/loop Menu: 4.619 uSec/loop Tests 280 to 330: Before: 5.142 uSec/loop After: 4.464 uSec/loop Menu: 4.619 uSec/loop Notice that the "after" this patch set is applied eventually does better than using the menu governor. Its processor package power always remains less, than the menu governor. The results can be viewed graphically at the above link, but the most dramatic results are: Idle state 3 above % goes from 70% to 5%. Idle state 2 below % goes from 13% to less than 1%. ... Doug
On Sun, Jul 4, 2021 at 11:01 PM Doug Smythies <dsmythies@telus.net> wrote: > > Hi Rafael, > > On 2021.06.02 11:14 Rafael J. Wysocki wrote: > > > Hi All, > > > > This series of patches addresses some theoretical shortcoming in the > > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > > state selection logic to some extent. > > > > Patches [1-2/5] are introductory cleanups and the substantial changes are > > made in patches [3-4/5] (please refer to the changelogs of these two > > patches for details). The last patch only deals with documentation. > > > > Even though this work is mostly based on theoretical considerations, it > > shows a measurable reduction of the number of cases in which the > shallowest > > idle state is selected while it would be more beneficial to select a > deeper > > one or the deepest idle state is selected while it would be more > beneficial to > > select a shallower one, which should be a noticeable improvement. > > Do you have any test results to share? Or test methods that I can try? > I have done a few tests, and generally don't notice much difference. > Perhaps an increase in idle state 2 below (was to shallow) numbers. > I am searching for some results that would offset the below: > > The difficulty I am having with this patch set is the additional overhead > which becomes significant at the extremes, where idle state 0 is dominant. > Throughout the history of teo, I have used multiple one core pipe-tests > for this particular test. Some results: > > CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz > HWP: disabled > CPU frequency scaling driver: intel_pstate, active, powersave > Pipe-tests are run forever, printing average loop time for the > Last 2.5 million loops. 1021 of those are again averaged. > Total = 2.5525e9 loops > The power and idle data is sampled for 100 minutes. > > Note 1: other tests were also done and also with passive, > schedutil, but it isn't relevant for this test because the > CPU frequency stays pinned at maximum. > > Note 2: I use TCC offset for thermal throttling, but I disabled it > for these tests, because the temperature needed to go higher > than my normal throttling point. > > Idle configuration 1: As a COMETLAKE processor, with 4 idle states. > Kernel 5.13-RC4. > > Before patch set average: > 2.8014 uSec/loop > 113.9 watts > Idle state 0 residency: 9.450% > Idle state 0 entries per minute: 256,812,896.6 > > After patch set average: > 2.8264 uSec/loop, 0.89% slower > 114.0 watts > Idle state 0 residency: 8.677% > Idle state 0 entries per minute: 254,560,049.9 > > Menu governor: > 2.8051 uSec/loop, 0.13% slower > 113.9 watts > Idle state 0 residency: 8.437% > Idle state 0 entries per minute: 256,436,417.2 > > O.K., perhaps not so bad, but also not many idle states. > > Idle configuration 2: As a SKYLAKE processor, with 9 idle states. > i.e.: > /drivers/idle/intel_idle.c > static const struct x86_cpu_id intel_idle_ids[] __initconst > ... > X86_MATCH_INTEL_FAM6_MODEL(SKYLAKE_X, &idle_cpu_skx), > + X86_MATCH_INTEL_FAM6_MODEL(COMETLAKE, &idle_cpu_skl), > > Purpose: To demonstrate increasing overhead as a function of number > of idle states. > Kernel 5.13. > > Before patch set average: > 2.8394 uSec/loop > 114.2 watts > Idle state 0 residency: 7.212% > Idle state 0 entries per minute: 253,391,954.3 > > After patch set average: > 2.9103 uSec/loop, 2.5% slower > 114.4 watts, 0.18% more > Idle state 0 residency: 6.152%, 14.7% less. > Idle state 0 entries per minute: 244,024,752.1 > > Menu governor: > 2.8141 uSec/loop, 0.89% faster > 113.9 watts, 0.26% less > Idle state 0 residency: 7.167%, 0.6% less > Idle state 0 entries per minute: 255,650,610.7 > > Another potentially interesting test was the ebizzy test: > Records per second, averaged over many tests, varying > threads and intervals: > > passive, schedutil: > Before: 6771.977 > After: 5502.643, -18.7% > Menu: 10728.89, +58.4% > > Active, powersave: > Before: 8361.82 > After: 8463.31, +1.2% > Menu: 8225.58, -1.6% > > I think it has more to do with CPU scaling governors > than this patch set, so: > > performance: > Before: 12137.33 > After: 12083.26, -0.4% > Menu: 11983.73, -1.3% > > These and other test results available here: > (encoded to prevent a barrage of bots) > > double u double u double u dot smythies dot com > /~doug/linux/idle/teo-2021-06/ > > ... a day later ... > > I might have an answer to my own question. > By switching to cross core pipe-tests, and only loading down one > CPU per core, I was able to get a lot more activity in other idle states. > The test runs for 100 minutes, and the results change with time, but > I'll leave that investigation for another day (there is no throttling): > > 1st 50 tests: > Before: 3.888 uSec/loop > After: 3.764 uSec/loop > Menu: 3.464 uSec/loop > > Tests 50 to 100: > Before: 4.329 uSec/loop > After: 3.919 uSec/loop > Menu: 3.514 uSec/loop > > Tests 200 to 250: > Before: 5.089 uSec/loop > After: 4.364 uSec/loop > Menu: 4.619 uSec/loop > > Tests 280 to 330: > Before: 5.142 uSec/loop > After: 4.464 uSec/loop > Menu: 4.619 uSec/loop > > Notice that the "after" this patch set is applied eventually does > better than using the menu governor. Its processor package power > always remains less, than the menu governor. That's good news, thanks! > The results can be viewed graphically at the above link, but the > most dramatic results are: > > Idle state 3 above % goes from 70% to 5%. > Idle state 2 below % goes from 13% to less than 1%. This also looks promising. Thank you for all of the results!
Hi Rafael, Further to my reply of 2021.07.04 on this, I have continued to work with and test this patch set. On 2021.06.02 11:14 Rafael J. Wysocki wrote: >This series of patches addresses some theoretical shortcoming in the > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > state selection logic to some extent. > > Patches [1-2/5] are introductory cleanups and the substantial changes are > made in patches [3-4/5] (please refer to the changelogs of these two > patches for details). The last patch only deals with documentation. > > Even though this work is mostly based on theoretical considerations, it > shows a measurable reduction of the number of cases in which the shallowest > idle state is selected while it would be more beneficial to select a deeper > one or the deepest idle state is selected while it would be more beneficial to > select a shallower one, which should be a noticeable improvement. I am concentrating in the idle state 0 and 1 area. When I disable idle state 0, the expectation is its usage will fall to idle state 1. It doesn't. Conditions: CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz HWP: disabled CPU frequency scaling driver: intel_pstate, active CPU frequency scaling governor: performance. Idle configuration: As a COMETLAKE processor, with 4 idle states. Sample time for below: 1 minute. Workflow: Cross core named pipe token passing, 12 threads. Kernel 5.14-rc3: idle: teo governor All idle states enabled: PASS Processor: 97 watts Idle state 0 entries: 811151 Idle state 1 entries: 140300776 Idle state 2 entries: 889 Idle state 3 entries: 8 Idle state 0 disabled: FAIL <<<<< Processor: 96 watts Idle state 0 entries: 0 Idle state 1 entries: 65599283 Idle state 2 entries: 364399 Idle state 3 entries: 65112651 Kernel 5.14-rc3: idle: menu governor All idle states enabled: PASS Processor: 102 watts Idle state 0 entries: 169320747 Idle state 1 entries: 1860110 Idle state 2 entries: 14 Idle state 3 entries: 54 Idle state 0 disabled: PASS Processor: 96.7 watts Idle state 0 entries: 0 Idle state 1 entries: 141936790 Idle state 2 entries: 0 Idle state 3 entries: 6 Prior to this patch set: Kernel 5.13: idle: teo governor All idle states enabled: PASS Processor: 97 watts Idle state 0 entries: 446735 Idle state 1 entries: 140903027 Idle state 2 entries: 0 Idle state 3 entries: 0 Idle state 0 disabled: PASS Processor: 96 watts Idle state 0 entries: 0 Idle state 1 entries: 139308125 Idle state 2 entries: 0 Idle state 3 entries: 0 I haven't tried to isolate the issue in the code, yet. Nor have I explored to determine if there might be other potential idle state disabled issues. ... Doug
On Tue, Jul 27, 2021 at 10:06 PM Doug Smythies <dsmythies@telus.net> wrote: > > Hi Rafael, > > Further to my reply of 2021.07.04 on this, I have > continued to work with and test this patch set. > > On 2021.06.02 11:14 Rafael J. Wysocki wrote: > > >This series of patches addresses some theoretical shortcoming in the > > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > > state selection logic to some extent. > > > > Patches [1-2/5] are introductory cleanups and the substantial changes are > > made in patches [3-4/5] (please refer to the changelogs of these two > > patches for details). The last patch only deals with documentation. > > > > Even though this work is mostly based on theoretical considerations, it > > shows a measurable reduction of the number of cases in which the shallowest > > idle state is selected while it would be more beneficial to select a deeper > > one or the deepest idle state is selected while it would be more beneficial to > > select a shallower one, which should be a noticeable improvement. > > I am concentrating in the idle state 0 and 1 area. > When I disable idle state 0, the expectation is its > usage will fall to idle state 1. It doesn't. > > Conditions: > CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz > HWP: disabled > CPU frequency scaling driver: intel_pstate, active > CPU frequency scaling governor: performance. > Idle configuration: As a COMETLAKE processor, with 4 idle states. > Sample time for below: 1 minute. > Workflow: Cross core named pipe token passing, 12 threads. > > Kernel 5.14-rc3: idle: teo governor > > All idle states enabled: PASS > Processor: 97 watts > Idle state 0 entries: 811151 > Idle state 1 entries: 140300776 > Idle state 2 entries: 889 > Idle state 3 entries: 8 > > Idle state 0 disabled: FAIL <<<<< > Processor: 96 watts > Idle state 0 entries: 0 > Idle state 1 entries: 65599283 > Idle state 2 entries: 364399 > Idle state 3 entries: 65112651 This looks odd. Thanks for the report, I'll take a look at this.
On Wednesday, July 28, 2021 3:52:51 PM CEST Rafael J. Wysocki wrote: > On Tue, Jul 27, 2021 at 10:06 PM Doug Smythies <dsmythies@telus.net> wrote: > > > > Hi Rafael, > > > > Further to my reply of 2021.07.04 on this, I have > > continued to work with and test this patch set. > > > > On 2021.06.02 11:14 Rafael J. Wysocki wrote: > > > > >This series of patches addresses some theoretical shortcoming in the > > > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > > > state selection logic to some extent. > > > > > > Patches [1-2/5] are introductory cleanups and the substantial changes are > > > made in patches [3-4/5] (please refer to the changelogs of these two > > > patches for details). The last patch only deals with documentation. > > > > > > Even though this work is mostly based on theoretical considerations, it > > > shows a measurable reduction of the number of cases in which the shallowest > > > idle state is selected while it would be more beneficial to select a deeper > > > one or the deepest idle state is selected while it would be more beneficial to > > > select a shallower one, which should be a noticeable improvement. > > > > I am concentrating in the idle state 0 and 1 area. > > When I disable idle state 0, the expectation is its > > usage will fall to idle state 1. It doesn't. > > > > Conditions: > > CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz > > HWP: disabled > > CPU frequency scaling driver: intel_pstate, active > > CPU frequency scaling governor: performance. > > Idle configuration: As a COMETLAKE processor, with 4 idle states. > > Sample time for below: 1 minute. > > Workflow: Cross core named pipe token passing, 12 threads. > > > > Kernel 5.14-rc3: idle: teo governor > > > > All idle states enabled: PASS > > Processor: 97 watts > > Idle state 0 entries: 811151 > > Idle state 1 entries: 140300776 > > Idle state 2 entries: 889 > > Idle state 3 entries: 8 > > > > Idle state 0 disabled: FAIL <<<<< > > Processor: 96 watts > > Idle state 0 entries: 0 > > Idle state 1 entries: 65599283 > > Idle state 2 entries: 364399 > > Idle state 3 entries: 65112651 > > This looks odd. > > Thanks for the report, I'll take a look at this. I have found an issue in the code that may be responsible for the observed behavior and should be addressed by the appended patch (not tested yet). Basically, the "disabled" check in the second loop over states in teo_select() needs to exclude the first enabled state, because there are no more states to check after that. Plus the time span check needs to be done when the given state is about to be selected, because otherwise the function may end up returning a state for which the sums are too low. Thanks! --- drivers/cpuidle/governors/teo.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) Index: linux-pm/drivers/cpuidle/governors/teo.c =================================================================== --- linux-pm.orig/drivers/cpuidle/governors/teo.c +++ linux-pm/drivers/cpuidle/governors/teo.c @@ -404,25 +404,27 @@ static int teo_select(struct cpuidle_dri intercept_sum += bin->intercepts; recent_sum += bin->recent; - if (dev->states_usage[i].disable) + if (dev->states_usage[i].disable && i > idx0) continue; span_ns = teo_middle_of_bin(i, drv); - if (!teo_time_ok(span_ns)) { - /* - * The current state is too shallow, so select - * the first enabled deeper state. - */ - duration_ns = last_enabled_span_ns; - idx = last_enabled_idx; - break; - } if ((!alt_recent || 2 * recent_sum > idx_recent_sum) && (!alt_intercepts || 2 * intercept_sum > idx_intercept_sum)) { - idx = i; - duration_ns = span_ns; + if (!teo_time_ok(span_ns) || + dev->states_usage[i].disable) { + /* + * The current state is too shallow or + * disabled, so select the first enabled + * deeper state. + */ + duration_ns = last_enabled_span_ns; + idx = last_enabled_idx; + } else { + idx = i; + duration_ns = span_ns; + } break; }
On Wed, Jul 28, 2021 at 10:47 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > On Wednesday, July 28, 2021 3:52:51 PM CEST Rafael J. Wysocki wrote: > > On Tue, Jul 27, 2021 at 10:06 PM Doug Smythies <dsmythies@telus.net> wrote: > > > > > > Hi Rafael, > > > > > > Further to my reply of 2021.07.04 on this, I have > > > continued to work with and test this patch set. > > > > > > On 2021.06.02 11:14 Rafael J. Wysocki wrote: > > > > > > >This series of patches addresses some theoretical shortcoming in the > > > > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > > > > state selection logic to some extent. > > > > > > > > Patches [1-2/5] are introductory cleanups and the substantial changes are > > > > made in patches [3-4/5] (please refer to the changelogs of these two > > > > patches for details). The last patch only deals with documentation. > > > > > > > > Even though this work is mostly based on theoretical considerations, it > > > > shows a measurable reduction of the number of cases in which the shallowest > > > > idle state is selected while it would be more beneficial to select a deeper > > > > one or the deepest idle state is selected while it would be more beneficial to > > > > select a shallower one, which should be a noticeable improvement. > > > > > > I am concentrating in the idle state 0 and 1 area. > > > When I disable idle state 0, the expectation is its > > > usage will fall to idle state 1. It doesn't. > > > > > > Conditions: > > > CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz > > > HWP: disabled > > > CPU frequency scaling driver: intel_pstate, active > > > CPU frequency scaling governor: performance. > > > Idle configuration: As a COMETLAKE processor, with 4 idle states. > > > Sample time for below: 1 minute. > > > Workflow: Cross core named pipe token passing, 12 threads. > > > > > > Kernel 5.14-rc3: idle: teo governor > > > > > > All idle states enabled: PASS > > > Processor: 97 watts > > > Idle state 0 entries: 811151 > > > Idle state 1 entries: 140300776 > > > Idle state 2 entries: 889 > > > Idle state 3 entries: 8 > > > > > > Idle state 0 disabled: FAIL <<<<< > > > Processor: 96 watts > > > Idle state 0 entries: 0 > > > Idle state 1 entries: 65599283 > > > Idle state 2 entries: 364399 > > > Idle state 3 entries: 65112651 > > > > This looks odd. > > > > Thanks for the report, I'll take a look at this. > > I have found an issue in the code that may be responsible for the > observed behavior and should be addressed by the appended patch (not > tested yet). > > Basically, the "disabled" check in the second loop over states in > teo_select() needs to exclude the first enabled state, because > there are no more states to check after that. > > Plus the time span check needs to be done when the given state > is about to be selected, because otherwise the function may end up > returning a state for which the sums are too low. > > Thanks! > > --- > drivers/cpuidle/governors/teo.c | 26 ++++++++++++++------------ > 1 file changed, 14 insertions(+), 12 deletions(-) > > Index: linux-pm/drivers/cpuidle/governors/teo.c > =================================================================== > --- linux-pm.orig/drivers/cpuidle/governors/teo.c > +++ linux-pm/drivers/cpuidle/governors/teo.c > @@ -404,25 +404,27 @@ static int teo_select(struct cpuidle_dri > intercept_sum += bin->intercepts; > recent_sum += bin->recent; > > - if (dev->states_usage[i].disable) > + if (dev->states_usage[i].disable && i > idx0) > continue; > > span_ns = teo_middle_of_bin(i, drv); > - if (!teo_time_ok(span_ns)) { > - /* > - * The current state is too shallow, so select > - * the first enabled deeper state. > - */ > - duration_ns = last_enabled_span_ns; > - idx = last_enabled_idx; > - break; > - } > > if ((!alt_recent || 2 * recent_sum > idx_recent_sum) && > (!alt_intercepts || > 2 * intercept_sum > idx_intercept_sum)) { > - idx = i; > - duration_ns = span_ns; > + if (!teo_time_ok(span_ns) || > + dev->states_usage[i].disable) { > + /* > + * The current state is too shallow or > + * disabled, so select the first enabled > + * deeper state. > + */ > + duration_ns = last_enabled_span_ns; > + idx = last_enabled_idx; > + } else { > + idx = i; > + duration_ns = span_ns; > + } > break; > } Hi Rafael, I tried the patch and when I disabled idle state 0 got, very similar to before: Idle state 0 disabled: FAIL Processor: 95 watts Idle state 0 entries: 0 Idle state 1 entries: 65,475,534 Idle state 2 entries: 333144 Idle state 3 entries: 65,247,048 However, I accidently left it for about 30 minutes and noticed: Idle state 0 disabled: Processor: 83 watts Idle state 0 entries: 0 Idle state 1 entries: 88,706,831 Idle state 2 entries: 100 Idle state 3 entries: 662 I went back to unmodified kernel 5.13-rc3 and let it run longer with idle state 0 disabled, and after 30 minutes it had changed but nowhere near as much: Idle state 0 disabled: Processor: 87 watts Idle state 0 entries: 0 Idle state 1 entries: 70,361,020 Idle state 2 entries: 71219 Idle state 3 entries: 27,249,975 ... Doug
On Wed, Jul 28, 2021 at 11:34 PM Doug Smythies <dsmythies@telus.net> wrote: > > On Wed, Jul 28, 2021 at 10:47 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > On Wednesday, July 28, 2021 3:52:51 PM CEST Rafael J. Wysocki wrote: > > > On Tue, Jul 27, 2021 at 10:06 PM Doug Smythies <dsmythies@telus.net> wrote: > > > > > > > > Hi Rafael, > > > > > > > > Further to my reply of 2021.07.04 on this, I have > > > > continued to work with and test this patch set. > > > > > > > > On 2021.06.02 11:14 Rafael J. Wysocki wrote: > > > > > > > > >This series of patches addresses some theoretical shortcoming in the > > > > > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > > > > > state selection logic to some extent. > > > > > > > > > > Patches [1-2/5] are introductory cleanups and the substantial changes are > > > > > made in patches [3-4/5] (please refer to the changelogs of these two > > > > > patches for details). The last patch only deals with documentation. > > > > > > > > > > Even though this work is mostly based on theoretical considerations, it > > > > > shows a measurable reduction of the number of cases in which the shallowest > > > > > idle state is selected while it would be more beneficial to select a deeper > > > > > one or the deepest idle state is selected while it would be more beneficial to > > > > > select a shallower one, which should be a noticeable improvement. > > > > > > > > I am concentrating in the idle state 0 and 1 area. > > > > When I disable idle state 0, the expectation is its > > > > usage will fall to idle state 1. It doesn't. > > > > > > > > Conditions: > > > > CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz > > > > HWP: disabled > > > > CPU frequency scaling driver: intel_pstate, active > > > > CPU frequency scaling governor: performance. > > > > Idle configuration: As a COMETLAKE processor, with 4 idle states. > > > > Sample time for below: 1 minute. > > > > Workflow: Cross core named pipe token passing, 12 threads. > > > > > > > > Kernel 5.14-rc3: idle: teo governor > > > > > > > > All idle states enabled: PASS > > > > Processor: 97 watts > > > > Idle state 0 entries: 811151 > > > > Idle state 1 entries: 140300776 > > > > Idle state 2 entries: 889 > > > > Idle state 3 entries: 8 > > > > > > > > Idle state 0 disabled: FAIL <<<<< > > > > Processor: 96 watts > > > > Idle state 0 entries: 0 > > > > Idle state 1 entries: 65599283 > > > > Idle state 2 entries: 364399 > > > > Idle state 3 entries: 65112651 > > > > > > This looks odd. > > > > > > Thanks for the report, I'll take a look at this. > > > > I have found an issue in the code that may be responsible for the > > observed behavior and should be addressed by the appended patch (not > > tested yet). > > > > Basically, the "disabled" check in the second loop over states in > > teo_select() needs to exclude the first enabled state, because > > there are no more states to check after that. > > > > Plus the time span check needs to be done when the given state > > is about to be selected, because otherwise the function may end up > > returning a state for which the sums are too low. > > > > Thanks! > > > > --- > > drivers/cpuidle/governors/teo.c | 26 ++++++++++++++------------ > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > Index: linux-pm/drivers/cpuidle/governors/teo.c > > =================================================================== > > --- linux-pm.orig/drivers/cpuidle/governors/teo.c > > +++ linux-pm/drivers/cpuidle/governors/teo.c > > @@ -404,25 +404,27 @@ static int teo_select(struct cpuidle_dri > > intercept_sum += bin->intercepts; > > recent_sum += bin->recent; > > > > - if (dev->states_usage[i].disable) > > + if (dev->states_usage[i].disable && i > idx0) > > continue; > > > > span_ns = teo_middle_of_bin(i, drv); > > - if (!teo_time_ok(span_ns)) { > > - /* > > - * The current state is too shallow, so select > > - * the first enabled deeper state. > > - */ > > - duration_ns = last_enabled_span_ns; > > - idx = last_enabled_idx; > > - break; > > - } > > > > if ((!alt_recent || 2 * recent_sum > idx_recent_sum) && > > (!alt_intercepts || > > 2 * intercept_sum > idx_intercept_sum)) { > > - idx = i; > > - duration_ns = span_ns; > > + if (!teo_time_ok(span_ns) || > > + dev->states_usage[i].disable) { > > + /* > > + * The current state is too shallow or > > + * disabled, so select the first enabled > > + * deeper state. > > + */ > > + duration_ns = last_enabled_span_ns; > > + idx = last_enabled_idx; > > + } else { > > + idx = i; > > + duration_ns = span_ns; > > + } > > break; > > } > > Hi Rafael, > > I tried the patch and when I disabled idle state 0 > got, very similar to before: > > Idle state 0 disabled: FAIL > Processor: 95 watts > Idle state 0 entries: 0 > Idle state 1 entries: 65,475,534 > Idle state 2 entries: 333144 > Idle state 3 entries: 65,247,048 > > However, I accidently left it for about 30 minutes > and noticed: > > Idle state 0 disabled: > Processor: 83 watts > Idle state 0 entries: 0 > Idle state 1 entries: 88,706,831 > Idle state 2 entries: 100 > Idle state 3 entries: 662 > > I went back to unmodified kernel 5.13-rc3 and Sorry, 5.14-rc3. > let it run longer with idle state 0 disabled, and > after 30 minutes it had changed but nowhere > near as much: > > Idle state 0 disabled: > Processor: 87 watts > Idle state 0 entries: 0 > Idle state 1 entries: 70,361,020 > Idle state 2 entries: 71219 > Idle state 3 entries: 27,249,975 Addendum: So far the workflow used for this thread has been event based. If I switch to a timer based workflow, everything works as expected for both kernels, 5.14-rc3 unmodified and modified with the patch from herein. ... Doug
On Thursday, July 29, 2021 8:34:37 AM CEST Doug Smythies wrote: > On Wed, Jul 28, 2021 at 10:47 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > On Wednesday, July 28, 2021 3:52:51 PM CEST Rafael J. Wysocki wrote: > > > On Tue, Jul 27, 2021 at 10:06 PM Doug Smythies <dsmythies@telus.net> wrote: > > > > > > > > Hi Rafael, > > > > > > > > Further to my reply of 2021.07.04 on this, I have > > > > continued to work with and test this patch set. > > > > > > > > On 2021.06.02 11:14 Rafael J. Wysocki wrote: > > > > > > > > >This series of patches addresses some theoretical shortcoming in the > > > > > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > > > > > state selection logic to some extent. > > > > > > > > > > Patches [1-2/5] are introductory cleanups and the substantial changes are > > > > > made in patches [3-4/5] (please refer to the changelogs of these two > > > > > patches for details). The last patch only deals with documentation. > > > > > > > > > > Even though this work is mostly based on theoretical considerations, it > > > > > shows a measurable reduction of the number of cases in which the shallowest > > > > > idle state is selected while it would be more beneficial to select a deeper > > > > > one or the deepest idle state is selected while it would be more beneficial to > > > > > select a shallower one, which should be a noticeable improvement. > > > > > > > > I am concentrating in the idle state 0 and 1 area. > > > > When I disable idle state 0, the expectation is its > > > > usage will fall to idle state 1. It doesn't. > > > > > > > > Conditions: > > > > CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz > > > > HWP: disabled > > > > CPU frequency scaling driver: intel_pstate, active > > > > CPU frequency scaling governor: performance. > > > > Idle configuration: As a COMETLAKE processor, with 4 idle states. > > > > Sample time for below: 1 minute. > > > > Workflow: Cross core named pipe token passing, 12 threads. > > > > > > > > Kernel 5.14-rc3: idle: teo governor > > > > > > > > All idle states enabled: PASS > > > > Processor: 97 watts > > > > Idle state 0 entries: 811151 > > > > Idle state 1 entries: 140300776 > > > > Idle state 2 entries: 889 > > > > Idle state 3 entries: 8 > > > > > > > > Idle state 0 disabled: FAIL <<<<< > > > > Processor: 96 watts > > > > Idle state 0 entries: 0 > > > > Idle state 1 entries: 65599283 > > > > Idle state 2 entries: 364399 > > > > Idle state 3 entries: 65112651 > > > > > > This looks odd. > > > > > > Thanks for the report, I'll take a look at this. > > > > I have found an issue in the code that may be responsible for the > > observed behavior and should be addressed by the appended patch (not > > tested yet). > > > > Basically, the "disabled" check in the second loop over states in > > teo_select() needs to exclude the first enabled state, because > > there are no more states to check after that. > > > > Plus the time span check needs to be done when the given state > > is about to be selected, because otherwise the function may end up > > returning a state for which the sums are too low. > > > > Thanks! > > > > --- > > drivers/cpuidle/governors/teo.c | 26 ++++++++++++++------------ > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > Index: linux-pm/drivers/cpuidle/governors/teo.c > > =================================================================== > > --- linux-pm.orig/drivers/cpuidle/governors/teo.c > > +++ linux-pm/drivers/cpuidle/governors/teo.c > > @@ -404,25 +404,27 @@ static int teo_select(struct cpuidle_dri > > intercept_sum += bin->intercepts; > > recent_sum += bin->recent; > > > > - if (dev->states_usage[i].disable) > > + if (dev->states_usage[i].disable && i > idx0) > > continue; > > > > span_ns = teo_middle_of_bin(i, drv); > > - if (!teo_time_ok(span_ns)) { > > - /* > > - * The current state is too shallow, so select > > - * the first enabled deeper state. > > - */ > > - duration_ns = last_enabled_span_ns; > > - idx = last_enabled_idx; > > - break; > > - } > > > > if ((!alt_recent || 2 * recent_sum > idx_recent_sum) && > > (!alt_intercepts || > > 2 * intercept_sum > idx_intercept_sum)) { > > - idx = i; > > - duration_ns = span_ns; > > + if (!teo_time_ok(span_ns) || > > + dev->states_usage[i].disable) { > > + /* > > + * The current state is too shallow or > > + * disabled, so select the first enabled > > + * deeper state. > > + */ > > + duration_ns = last_enabled_span_ns; > > + idx = last_enabled_idx; > > + } else { > > + idx = i; > > + duration_ns = span_ns; > > + } > > break; > > } > > Hi Rafael, Hi Doug, Thanks for the feedback, much appreciated! > I tried the patch and when I disabled idle state 0 > got, very similar to before: > > Idle state 0 disabled: FAIL > Processor: 95 watts > Idle state 0 entries: 0 > Idle state 1 entries: 65,475,534 > Idle state 2 entries: 333144 > Idle state 3 entries: 65,247,048 > > However, I accidently left it for about 30 minutes > and noticed: > > Idle state 0 disabled: > Processor: 83 watts > Idle state 0 entries: 0 > Idle state 1 entries: 88,706,831 > Idle state 2 entries: 100 > Idle state 3 entries: 662 This means that idle state 0 data are disregarded after disabling it and that most likely is because the second loop in teo_select() should be over all states down to idle state 0 (not only down to the first enabled one). So below is an updated patch (not tested yet). --- drivers/cpuidle/governors/teo.c | 28 +++++++++++++++------------- 1 file changed, 15 insertions(+), 13 deletions(-) Index: linux-pm/drivers/cpuidle/governors/teo.c =================================================================== --- linux-pm.orig/drivers/cpuidle/governors/teo.c +++ linux-pm/drivers/cpuidle/governors/teo.c @@ -397,32 +397,34 @@ static int teo_select(struct cpuidle_dri intercept_sum = 0; recent_sum = 0; - for (i = idx - 1; i >= idx0; i--) { + for (i = idx - 1; i >= 0; i--) { struct teo_bin *bin = &cpu_data->state_bins[i]; s64 span_ns; intercept_sum += bin->intercepts; recent_sum += bin->recent; - if (dev->states_usage[i].disable) + if (dev->states_usage[i].disable && i > 0) continue; span_ns = teo_middle_of_bin(i, drv); - if (!teo_time_ok(span_ns)) { - /* - * The current state is too shallow, so select - * the first enabled deeper state. - */ - duration_ns = last_enabled_span_ns; - idx = last_enabled_idx; - break; - } if ((!alt_recent || 2 * recent_sum > idx_recent_sum) && (!alt_intercepts || 2 * intercept_sum > idx_intercept_sum)) { - idx = i; - duration_ns = span_ns; + if (!teo_time_ok(span_ns) || + dev->states_usage[i].disable) { + /* + * The current state is too shallow or + * disabled, so select the first enabled + * deeper state. + */ + duration_ns = last_enabled_span_ns; + idx = last_enabled_idx; + } else { + idx = i; + duration_ns = span_ns; + } break; }
On Thu, Jul 29, 2021 at 5:24 PM Doug Smythies <dsmythies@telus.net> wrote: > > On Wed, Jul 28, 2021 at 11:34 PM Doug Smythies <dsmythies@telus.net> wrote: > > > > On Wed, Jul 28, 2021 at 10:47 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > > > > On Wednesday, July 28, 2021 3:52:51 PM CEST Rafael J. Wysocki wrote: > > > > On Tue, Jul 27, 2021 at 10:06 PM Doug Smythies <dsmythies@telus.net> wrote: > > > > > > > > > > Hi Rafael, > > > > > > > > > > Further to my reply of 2021.07.04 on this, I have > > > > > continued to work with and test this patch set. > > > > > > > > > > On 2021.06.02 11:14 Rafael J. Wysocki wrote: > > > > > > > > > > >This series of patches addresses some theoretical shortcoming in the > > > > > > TEO (Timer Events Oriented) cpuidle governor by reworking its idle > > > > > > state selection logic to some extent. > > > > > > > > > > > > Patches [1-2/5] are introductory cleanups and the substantial changes are > > > > > > made in patches [3-4/5] (please refer to the changelogs of these two > > > > > > patches for details). The last patch only deals with documentation. > > > > > > > > > > > > Even though this work is mostly based on theoretical considerations, it > > > > > > shows a measurable reduction of the number of cases in which the shallowest > > > > > > idle state is selected while it would be more beneficial to select a deeper > > > > > > one or the deepest idle state is selected while it would be more beneficial to > > > > > > select a shallower one, which should be a noticeable improvement. > > > > > > > > > > I am concentrating in the idle state 0 and 1 area. > > > > > When I disable idle state 0, the expectation is its > > > > > usage will fall to idle state 1. It doesn't. > > > > > > > > > > Conditions: > > > > > CPU: Intel(R) Core(TM) i5-10600K CPU @ 4.10GHz > > > > > HWP: disabled > > > > > CPU frequency scaling driver: intel_pstate, active > > > > > CPU frequency scaling governor: performance. > > > > > Idle configuration: As a COMETLAKE processor, with 4 idle states. > > > > > Sample time for below: 1 minute. > > > > > Workflow: Cross core named pipe token passing, 12 threads. > > > > > > > > > > Kernel 5.14-rc3: idle: teo governor > > > > > > > > > > All idle states enabled: PASS > > > > > Processor: 97 watts > > > > > Idle state 0 entries: 811151 > > > > > Idle state 1 entries: 140300776 > > > > > Idle state 2 entries: 889 > > > > > Idle state 3 entries: 8 > > > > > > > > > > Idle state 0 disabled: FAIL <<<<< > > > > > Processor: 96 watts > > > > > Idle state 0 entries: 0 > > > > > Idle state 1 entries: 65599283 > > > > > Idle state 2 entries: 364399 > > > > > Idle state 3 entries: 65112651 > > > > > > > > This looks odd. > > > > > > > > Thanks for the report, I'll take a look at this. > > > > > > I have found an issue in the code that may be responsible for the > > > observed behavior and should be addressed by the appended patch (not > > > tested yet). > > > > > > Basically, the "disabled" check in the second loop over states in > > > teo_select() needs to exclude the first enabled state, because > > > there are no more states to check after that. > > > > > > Plus the time span check needs to be done when the given state > > > is about to be selected, because otherwise the function may end up > > > returning a state for which the sums are too low. > > > > > > Thanks! > > > > > > --- > > > drivers/cpuidle/governors/teo.c | 26 ++++++++++++++------------ > > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > > > Index: linux-pm/drivers/cpuidle/governors/teo.c > > > =================================================================== > > > --- linux-pm.orig/drivers/cpuidle/governors/teo.c > > > +++ linux-pm/drivers/cpuidle/governors/teo.c > > > @@ -404,25 +404,27 @@ static int teo_select(struct cpuidle_dri > > > intercept_sum += bin->intercepts; > > > recent_sum += bin->recent; > > > > > > - if (dev->states_usage[i].disable) > > > + if (dev->states_usage[i].disable && i > idx0) > > > continue; > > > > > > span_ns = teo_middle_of_bin(i, drv); > > > - if (!teo_time_ok(span_ns)) { > > > - /* > > > - * The current state is too shallow, so select > > > - * the first enabled deeper state. > > > - */ > > > - duration_ns = last_enabled_span_ns; > > > - idx = last_enabled_idx; > > > - break; > > > - } > > > > > > if ((!alt_recent || 2 * recent_sum > idx_recent_sum) && > > > (!alt_intercepts || > > > 2 * intercept_sum > idx_intercept_sum)) { > > > - idx = i; > > > - duration_ns = span_ns; > > > + if (!teo_time_ok(span_ns) || > > > + dev->states_usage[i].disable) { > > > + /* > > > + * The current state is too shallow or > > > + * disabled, so select the first enabled > > > + * deeper state. > > > + */ > > > + duration_ns = last_enabled_span_ns; > > > + idx = last_enabled_idx; > > > + } else { > > > + idx = i; > > > + duration_ns = span_ns; > > > + } > > > break; > > > } > > > > Hi Rafael, > > > > I tried the patch and when I disabled idle state 0 > > got, very similar to before: > > > > Idle state 0 disabled: FAIL > > Processor: 95 watts > > Idle state 0 entries: 0 > > Idle state 1 entries: 65,475,534 > > Idle state 2 entries: 333144 > > Idle state 3 entries: 65,247,048 > > > > However, I accidently left it for about 30 minutes > > and noticed: > > > > Idle state 0 disabled: > > Processor: 83 watts > > Idle state 0 entries: 0 > > Idle state 1 entries: 88,706,831 > > Idle state 2 entries: 100 > > Idle state 3 entries: 662 > > > > I went back to unmodified kernel 5.13-rc3 and > > Sorry, 5.14-rc3. > > > let it run longer with idle state 0 disabled, and > > after 30 minutes it had changed but nowhere > > near as much: > > > > Idle state 0 disabled: > > Processor: 87 watts > > Idle state 0 entries: 0 > > Idle state 1 entries: 70,361,020 > > Idle state 2 entries: 71219 > > Idle state 3 entries: 27,249,975 > > Addendum: So far the workflow used for this > thread has been event based. If I switch to > a timer based workflow, everything works as > expected for both kernels, 5.14-rc3 unmodified > and modified with the patch from herein. Yes, the affected case is when the governor selects states that are shallower than indicated by the time till the next timer.
On Thu, Jul 29, 2021 at 9:14 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > ... [snip]... > > This means that idle state 0 data are disregarded after disabling it > and that most likely is because the second loop in teo_select() should > be over all states down to idle state 0 (not only down to the first > enabled one). > > So below is an updated patch (not tested yet). Hi Rafael, This updated patch works great / solves the problem. Tested-by: Doug Smythies <dsmythies@telus.net> Thank you very much. ... Doug > > --- > drivers/cpuidle/governors/teo.c | 28 +++++++++++++++------------- > 1 file changed, 15 insertions(+), 13 deletions(-) > > Index: linux-pm/drivers/cpuidle/governors/teo.c > =================================================================== > --- linux-pm.orig/drivers/cpuidle/governors/teo.c > +++ linux-pm/drivers/cpuidle/governors/teo.c > @@ -397,32 +397,34 @@ static int teo_select(struct cpuidle_dri > intercept_sum = 0; > recent_sum = 0; > > - for (i = idx - 1; i >= idx0; i--) { > + for (i = idx - 1; i >= 0; i--) { > struct teo_bin *bin = &cpu_data->state_bins[i]; > s64 span_ns; > > intercept_sum += bin->intercepts; > recent_sum += bin->recent; > > - if (dev->states_usage[i].disable) > + if (dev->states_usage[i].disable && i > 0) > continue; > > span_ns = teo_middle_of_bin(i, drv); > - if (!teo_time_ok(span_ns)) { > - /* > - * The current state is too shallow, so select > - * the first enabled deeper state. > - */ > - duration_ns = last_enabled_span_ns; > - idx = last_enabled_idx; > - break; > - } > > if ((!alt_recent || 2 * recent_sum > idx_recent_sum) && > (!alt_intercepts || > 2 * intercept_sum > idx_intercept_sum)) { > - idx = i; > - duration_ns = span_ns; > + if (!teo_time_ok(span_ns) || > + dev->states_usage[i].disable) { > + /* > + * The current state is too shallow or > + * disabled, so select the first enabled > + * deeper state. > + */ > + duration_ns = last_enabled_span_ns; > + idx = last_enabled_idx; > + } else { > + idx = i; > + duration_ns = span_ns; > + } > break; > }
On Fri, Jul 30, 2021 at 5:36 AM Doug Smythies <dsmythies@telus.net> wrote: > > On Thu, Jul 29, 2021 at 9:14 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > > > ... [snip]... > > > > This means that idle state 0 data are disregarded after disabling it > > and that most likely is because the second loop in teo_select() should > > be over all states down to idle state 0 (not only down to the first > > enabled one). > > > > So below is an updated patch (not tested yet). > > Hi Rafael, > > This updated patch works great / solves the problem. > Tested-by: Doug Smythies <dsmythies@telus.net> > > Thank you very much. Thank you and you're welcome! I've found a small issue in the patch though (it needs to check the time span before setting the "last enabled" index), so let me submit a proper patch with a changelog based on this one.