| Message ID | 20160311153044.1a3be1c6@annuminas.surriel.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Mar 11, 2016 at 9:30 PM, Rik van Riel <riel@redhat.com> wrote: > On Fri, 11 Mar 2016 10:22:30 -0800 > "Doug Smythies" <dsmythies@telus.net> wrote: > >> On 2016.03.11 06:03 Rik van Riel wrote: >> > On Thu, 10 Mar 2016 00:59:01 +0100 "Rafael J. Wysocki" <rafael@kernel.org> wrote: > >> > The patch below should fix that. >> > >> > It didn't for Arto, due to the other issues on his system, but >> > it might resolve the issue for Doug, where cstate/pstate is >> > otherwise working fine. >> > >> > Doug, does the patch below solve your issue? >> >> No. > > OK, lets try doing this check a little more aggressively, since there > almost certainly are good reasons why the main selection loop in > menu_select() so aggressively tries to stay with shallower C states. > > With the patch below, we compare the (not corrected for load) expected > sleep time against the target residency of the C1 (hlt) state on the CPU. > > This might resolve the issue, while still doing the right thing on CPUs > that have really high C1 latencies (eg. Atom). > > Does this resolve the issue for you? > > ---8<--- > > Subject: cpuidle: use predicted_us not interactivity_req to consider polling > > The interactivity_req variable is the expected sleep time, divided > by the CPU load. This can be too aggressive a factor in deciding > whether or not to consider polling in the cpuidle state selection. > > Use the (not corrected for load) predicted_us instead, and compare > it against the target residency of C1 (hlt). > > Signed-off-by: Rik van Riel <riel@redhat.com> > --- > drivers/cpuidle/governors/menu.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c > index 0742b3296673..7dda15cfe547 100644 > --- a/drivers/cpuidle/governors/menu.c > +++ b/drivers/cpuidle/governors/menu.c > @@ -330,7 +330,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) > * We want to default to C1 (hlt), not to busy polling > * unless the timer is happening really really soon. > */ > - if (interactivity_req > 20 && > + if (data->predicted_us > drv->states[CPUIDLE_DRIVER_STATE_START].target_residency && If we do this, we probably should check the exit latency (against latency_req) too. > !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && > dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) > data->last_state_idx = CPUIDLE_DRIVER_STATE_START; > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2106.03.11 15:55 Rafael J. Wysocki wrote: > On Fri, Mar 11, 2016 at 9:30 PM, Rik van Riel <riel@redhat.com> wrote: >> On Fri, 11 Mar 2016 10:22:30 -0800 Doug Smythies wrote: >>> On 2016.03.11 06:03 Rik van Riel wrote: >> >>>> The patch below should fix that. >>>> >>>> It didn't for Arto, due to the other issues on his system, but >>>> it might resolve the issue for Doug, where cstate/pstate is >>>> otherwise working fine. >>>> >>>> Doug, does the patch below solve your issue? >>> >>> No. >> >> OK, lets try doing this check a little more aggressively, since there >> almost certainly are good reasons why the main selection loop in >> menu_select() so aggressively tries to stay with shallower C states. >> >> With the patch below, we compare the (not corrected for load) expected >> sleep time against the target residency of the C1 (hlt) state on the CPU. >> >> This might resolve the issue, while still doing the right thing on CPUs >> that have really high C1 latencies (eg. Atom). >> >> Does this resolve the issue for you? No, but it seems better. Old data restated with new data added below: Aggregate times in each idle state for the 2000 second test: State k45rc7 (mins) reverted (mins) rvr(mins) rvr2(mins) 0.00 20.18 2.64 19.11 14.09 1.00 13.03 21.81 13.56 12.05 2.00 3.43 3.95 3.70 3.93 3.00 1.45 1.55 1.53 1.52 4.00 134.91 143.55 138.53 142.80 total 172.99 173.51 176.42 174.37 Energy (old restated, plus new added): Kernel 4.5-rc7 Reverted: 56178 Joules Kernel 4.5-rc7: 63269 Joules (revert saves 12.6% energy) Kernel 4.5-rc7 + rvr patch: 62914 Joules Kernel 4.5-rc7 + rvr patch version 2: 60416 Joules > If we do this, we probably should check the exit latency > (against latency_req) too. I already had the above data before I noticed Rafael's e-mail. ... Doug -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c index 0742b3296673..7dda15cfe547 100644 --- a/drivers/cpuidle/governors/menu.c +++ b/drivers/cpuidle/governors/menu.c @@ -330,7 +330,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) * We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ - if (interactivity_req > 20 && + if (data->predicted_us > drv->states[CPUIDLE_DRIVER_STATE_START].target_residency && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START;