* [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) @ 2015-11-03 22:34 riel 2015-11-03 22:34 ` [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us riel ` (3 more replies) 0 siblings, 4 replies; 16+ messages in thread From: riel @ 2015-11-03 22:34 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, rafael.j.wysocki, arjan, len.brown, daniel.lezcano While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code. These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies. I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us 2015-11-03 22:34 [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) riel @ 2015-11-03 22:34 ` riel 2015-11-04 16:00 ` Arjan van de Ven 2015-11-03 22:34 ` [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling riel ` (2 subsequent siblings) 3 siblings, 1 reply; 16+ messages in thread From: riel @ 2015-11-03 22:34 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, rafael.j.wysocki, arjan, len.brown, daniel.lezcano From: Rik van Riel <riel@redhat.com> The cpuidle menu governor has a forced cut-off for polling at 5us, in order to deal with firmware that gives the OS bad information on cpuidle states, leading to the system spending way too much time in polling. However, at least one x86 CPU family (Atom) has chips that have a 20us break-even point for C1. Forcing the polling cut-off to less than that wastes performance and power. Increase the polling cut-off to 20us. Systems with a lower C1 latency will be found in the states table by the menu governor, which will pick those states as appropriate. 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 22e4463d1787..ecc242a586c9 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 (data->next_timer_us > 5 && + if (data->next_timer_us > 20 && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us 2015-11-03 22:34 ` [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us riel @ 2015-11-04 16:00 ` Arjan van de Ven 0 siblings, 0 replies; 16+ messages in thread From: Arjan van de Ven @ 2015-11-04 16:00 UTC (permalink / raw) To: riel, linux-pm; +Cc: linux-kernel, rafael.j.wysocki, len.brown, daniel.lezcano Acked-by: Arjan van de Ven <arjan@linux.intel.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2015-11-03 22:34 [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) riel 2015-11-03 22:34 ` [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us riel @ 2015-11-03 22:34 ` riel 2015-11-04 16:01 ` Arjan van de Ven 2016-01-13 17:27 ` Sudeep Holla 2015-11-03 22:34 ` [PATCH 3/3] cpuidle,menu: smooth out measured_us calculation riel 2015-11-05 22:34 ` [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) Rafael J. Wysocki 3 siblings, 2 replies; 16+ messages in thread From: riel @ 2015-11-03 22:34 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, rafael.j.wysocki, arjan, len.brown, daniel.lezcano From: Rik van Riel <riel@redhat.com> The menu governor carefully figures out how much time we typically sleep for an estimated sleep interval, or whether there is a repeating pattern going on, and corrects that estimate for the CPU load. Then it proceeds to ignore that information when determining whether or not to consider polling. This is not a big deal on most x86 CPUs, which have very low C1 latencies, and the patch should not have any effect on those CPUs. However, certain CPUs (eg. Atom) have much higher C1 latencies, and it would be good to not waste performance and power on those CPUs if we are expecting a very low wakeup latency. Disable polling based on the estimated interactivity requirement, not on the time to the next timer interrupt. 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 ecc242a586c9..b1a55731f921 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 (data->next_timer_us > 20 && + if (interactivity_req > 20 && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2015-11-03 22:34 ` [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling riel @ 2015-11-04 16:01 ` Arjan van de Ven 2016-01-13 17:27 ` Sudeep Holla 1 sibling, 0 replies; 16+ messages in thread From: Arjan van de Ven @ 2015-11-04 16:01 UTC (permalink / raw) To: riel, linux-pm; +Cc: linux-kernel, rafael.j.wysocki, len.brown, daniel.lezcano On 11/3/2015 2:34 PM, riel@redhat.com wrote: > From: Rik van Riel <riel@redhat.com> > > The menu governor carefully figures out how much time we typically > sleep for an estimated sleep interval, or whether there is a repeating > pattern going on, and corrects that estimate for the CPU load. > > Then it proceeds to ignore that information when determining whether > or not to consider polling. This is not a big deal on most x86 CPUs, > which have very low C1 latencies, and the patch should not have any > effect on those CPUs. > > However, certain CPUs (eg. Atom) have much higher C1 latencies, and > it would be good to not waste performance and power on those CPUs if > we are expecting a very low wakeup latency. > > Disable polling based on the estimated interactivity requirement, not > on the time to the next timer interrupt. > good catch! Acked-by: Arjan van de Ven <arjan@linux.intel.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2015-11-03 22:34 ` [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling riel 2015-11-04 16:01 ` Arjan van de Ven @ 2016-01-13 17:27 ` Sudeep Holla 2016-01-13 21:58 ` Rafael J. Wysocki 1 sibling, 1 reply; 16+ messages in thread From: Sudeep Holla @ 2016-01-13 17:27 UTC (permalink / raw) To: riel, rafael.j.wysocki Cc: Linux PM list, open list, arjan, Len Brown, daniel.lezcano, Sudeep Holla Hi Rik, This change break idle on ARM64(may be on other ARM?) platform. Sorry for reporting late, but missed to check cpuidle in -next. On Tue, Nov 3, 2015 at 10:34 PM, <riel@redhat.com> wrote: > From: Rik van Riel <riel@redhat.com> > > The menu governor carefully figures out how much time we typically > sleep for an estimated sleep interval, or whether there is a repeating > pattern going on, and corrects that estimate for the CPU load. > > Then it proceeds to ignore that information when determining whether > or not to consider polling. This is not a big deal on most x86 CPUs, > which have very low C1 latencies, and the patch should not have any > effect on those CPUs. > > However, certain CPUs (eg. Atom) have much higher C1 latencies, and > it would be good to not waste performance and power on those CPUs if > we are expecting a very low wakeup latency. > > Disable polling based on the estimated interactivity requirement, not > on the time to the next timer interrupt. > > 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 ecc242a586c9..b1a55731f921 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 (data->next_timer_us > 20 && > + if (interactivity_req > 20 && I found that data->predicted_us is gets overwritten in get_typical_interval when avg computed = 0 which is the case initially on boot when the past intervals are not yet accumulated. I just tried a hack and that seem to work and proved what I anticipated (i.e. interactivity_req = 0). Let me know if you have any clues on how to solve it ? I can help you getting the change tested. Regards, Sudeep --->8 diff --git i/drivers/cpuidle/governors/menu.c w/drivers/cpuidle/governors/menu.c index 7b0971d97cc3..7c7f4059705a 100644 --- i/drivers/cpuidle/governors/menu.c +++ w/drivers/cpuidle/governors/menu.c @@ -330,7 +330,8 @@ 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 (((interactivity_req && interactivity_req > 20) || + data->next_timer_us > 20) && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2016-01-13 17:27 ` Sudeep Holla @ 2016-01-13 21:58 ` Rafael J. Wysocki 2016-01-13 22:07 ` Rafael J. Wysocki 2016-01-14 10:33 ` Sudeep Holla 0 siblings, 2 replies; 16+ messages in thread From: Rafael J. Wysocki @ 2016-01-13 21:58 UTC (permalink / raw) To: Sudeep Holla Cc: riel, Rafael Wysocki, Linux PM list, open list, arjan, Len Brown, Daniel Lezcano Hi, On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > Hi Rik, > > This change break idle on ARM64(may be on other ARM?) platform. > Sorry for reporting late, but missed to check cpuidle in -next. OK, so first of all, how exactly is idle broken on those systems? Do they crash or does something different happen? If something different happens, then what's that? > On Tue, Nov 3, 2015 at 10:34 PM, <riel@redhat.com> wrote: >> From: Rik van Riel <riel@redhat.com> >> >> The menu governor carefully figures out how much time we typically >> sleep for an estimated sleep interval, or whether there is a repeating >> pattern going on, and corrects that estimate for the CPU load. >> >> Then it proceeds to ignore that information when determining whether >> or not to consider polling. This is not a big deal on most x86 CPUs, >> which have very low C1 latencies, and the patch should not have any >> effect on those CPUs. >> >> However, certain CPUs (eg. Atom) have much higher C1 latencies, and >> it would be good to not waste performance and power on those CPUs if >> we are expecting a very low wakeup latency. >> >> Disable polling based on the estimated interactivity requirement, not >> on the time to the next timer interrupt. >> >> 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 ecc242a586c9..b1a55731f921 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 (data->next_timer_us > 20 && >> + if (interactivity_req > 20 && > > I found that data->predicted_us is gets overwritten in get_typical_interval > when avg computed = 0 which is the case initially on boot when the past > intervals are not yet accumulated. > > I just tried a hack and that seem to work and proved what I anticipated > (i.e. interactivity_req = 0). Let me know if you have any clues on how to > solve it ? I can help you getting the change tested. > > Regards, > Sudeep > > --->8 > > diff --git i/drivers/cpuidle/governors/menu.c w/drivers/cpuidle/governors/menu.c > index 7b0971d97cc3..7c7f4059705a 100644 > --- i/drivers/cpuidle/governors/menu.c > +++ w/drivers/cpuidle/governors/menu.c > @@ -330,7 +330,8 @@ 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 (((interactivity_req && interactivity_req > 20) || Well, if interactivity_req > 20, then it also is different from 0, so the first check should not be necessary here. > + data->next_timer_us > 20) && I guess that this simply restores the previous behavior on the platforms in question. Now, the reason why it may matter is because CPUIDLE_DRIVER_STATE_START is 0 and so data->last_state_idx may end up as -1 on them. So I think this piece of code only ever makes sense if CPUIDLE_DRIVER_STATE_START is 1. > !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && > dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) > data->last_state_idx = CPUIDLE_DRIVER_STATE_START; > -- Thanks, Rafael ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2016-01-13 21:58 ` Rafael J. Wysocki @ 2016-01-13 22:07 ` Rafael J. Wysocki 2016-01-14 10:40 ` Sudeep Holla 2016-01-14 10:33 ` Sudeep Holla 1 sibling, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2016-01-13 22:07 UTC (permalink / raw) To: Sudeep Holla Cc: Rafael J. Wysocki, riel, Rafael Wysocki, Linux PM list, open list, arjan, Len Brown, Daniel Lezcano On Wednesday, January 13, 2016 10:58:13 PM Rafael J. Wysocki wrote: > Hi, > > On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: > > Hi Rik, [cut] > > > > diff --git i/drivers/cpuidle/governors/menu.c w/drivers/cpuidle/governors/menu.c > > index 7b0971d97cc3..7c7f4059705a 100644 > > --- i/drivers/cpuidle/governors/menu.c > > +++ w/drivers/cpuidle/governors/menu.c > > @@ -330,7 +330,8 @@ 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 (((interactivity_req && interactivity_req > 20) || > > Well, if interactivity_req > 20, then it also is different from 0, so > the first check should not be necessary here. > > > + data->next_timer_us > 20) && > > I guess that this simply restores the previous behavior on the > platforms in question. > > Now, the reason why it may matter is because > CPUIDLE_DRIVER_STATE_START is 0 and so data->last_state_idx may end up > as -1 on them. So I think this piece of code only ever makes sense if > CPUIDLE_DRIVER_STATE_START is 1. > > > !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && > > dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) > > data->last_state_idx = CPUIDLE_DRIVER_STATE_START; > > -- So I'm wondering if the appended patch helps by any chance? Rafael --- drivers/cpuidle/governors/menu.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) Index: linux-pm/drivers/cpuidle/governors/menu.c =================================================================== --- linux-pm.orig/drivers/cpuidle/governors/menu.c +++ linux-pm/drivers/cpuidle/governors/menu.c @@ -294,8 +294,6 @@ static int menu_select(struct cpuidle_dr data->needs_update = 0; } - data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; - /* Special case when user has set very strict latency requirement */ if (unlikely(latency_req == 0)) return 0; @@ -326,14 +324,19 @@ static int menu_select(struct cpuidle_dr if (latency_req > interactivity_req) latency_req = interactivity_req; - /* - * We want to default to C1 (hlt), not to busy polling - * unless the timer is happening really really soon. - */ - if (interactivity_req > 20 && - !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && - dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) + if (CPUIDLE_DRIVER_STATE_START > 0) { + data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; + /* + * We want to default to C1 (hlt), not to busy polling + * unless the timer is happening really really soon. + */ + if (interactivity_req > 20 && + !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && + dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) + data->last_state_idx = CPUIDLE_DRIVER_STATE_START; + } else { data->last_state_idx = CPUIDLE_DRIVER_STATE_START; + } /* * Find the idle state with the lowest power while satisfying ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2016-01-13 22:07 ` Rafael J. Wysocki @ 2016-01-14 10:40 ` Sudeep Holla 2016-01-15 0:46 ` Rafael J. Wysocki 0 siblings, 1 reply; 16+ messages in thread From: Sudeep Holla @ 2016-01-14 10:40 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Sudeep Holla, Rafael J. Wysocki, riel, Rafael Wysocki, Linux PM list, open list, arjan, Len Brown, Daniel Lezcano On 13/01/16 22:07, Rafael J. Wysocki wrote: > On Wednesday, January 13, 2016 10:58:13 PM Rafael J. Wysocki wrote: [..] > > So I'm wondering if the appended patch helps by any chance? > Yes it does fix. As you mentioned earlier, CPUIDLE_DRIVER_STATE_START is 0 on ARM platforms and hence data->last_state_idx ended up as -1. You can add by Tested-by when you push this change. Thanks for the quick fix. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2016-01-14 10:40 ` Sudeep Holla @ 2016-01-15 0:46 ` Rafael J. Wysocki 0 siblings, 0 replies; 16+ messages in thread From: Rafael J. Wysocki @ 2016-01-15 0:46 UTC (permalink / raw) To: Sudeep Holla Cc: Rafael J. Wysocki, riel, Rafael Wysocki, Linux PM list, open list, arjan, Len Brown, Daniel Lezcano On Thursday, January 14, 2016 10:40:44 AM Sudeep Holla wrote: > > On 13/01/16 22:07, Rafael J. Wysocki wrote: > > On Wednesday, January 13, 2016 10:58:13 PM Rafael J. Wysocki wrote: > > [..] > > > > > So I'm wondering if the appended patch helps by any chance? > > > > Yes it does fix. As you mentioned earlier, CPUIDLE_DRIVER_STATE_START is > 0 on ARM platforms and hence data->last_state_idx ended up as -1. > > You can add by Tested-by when you push this change. Thanks for the quick > fix. No problem, thanks for testing! Below it goes again with a changelog and all. --- From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Subject: [PATCH] cpuidle: menu: Fix menu_select() for CPUIDLE_DRIVER_STATE_START == 0 Commit a9ceb78bc75c (cpuidle,menu: use interactivity_req to disable polling) exposed a bug in menu_select() causing it to return -1 on systems with CPUIDLE_DRIVER_STATE_START equal to zero, although it should have returned 0. As a result, idle states are not entered by CPUs on those systems. Namely, on the systems in question data->last_state_idx is initially equal to -1 and the above commit modified the condition that would have caused it to be changed to 0 to be less likely to trigger which exposed the problem. However, setting data->last_state_idx initially to -1 doesn't make sense at all and on the affected systems it should always be set to CPUIDLE_DRIVER_STATE_START (ie. 0) unconditionally, so make that happen. Fixes: a9ceb78bc75c (cpuidle,menu: use interactivity_req to disable polling) Reported-and-tested-by: Sudeep Holla <sudeep.holla@arm.com> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- drivers/cpuidle/governors/menu.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) Index: linux-pm/drivers/cpuidle/governors/menu.c =================================================================== --- linux-pm.orig/drivers/cpuidle/governors/menu.c +++ linux-pm/drivers/cpuidle/governors/menu.c @@ -294,8 +294,6 @@ static int menu_select(struct cpuidle_dr data->needs_update = 0; } - data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; - /* Special case when user has set very strict latency requirement */ if (unlikely(latency_req == 0)) return 0; @@ -326,14 +324,19 @@ static int menu_select(struct cpuidle_dr if (latency_req > interactivity_req) latency_req = interactivity_req; - /* - * We want to default to C1 (hlt), not to busy polling - * unless the timer is happening really really soon. - */ - if (interactivity_req > 20 && - !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && - dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) + if (CPUIDLE_DRIVER_STATE_START > 0) { + data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1; + /* + * We want to default to C1 (hlt), not to busy polling + * unless the timer is happening really really soon. + */ + if (interactivity_req > 20 && + !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && + dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) + data->last_state_idx = CPUIDLE_DRIVER_STATE_START; + } else { data->last_state_idx = CPUIDLE_DRIVER_STATE_START; + } /* * Find the idle state with the lowest power while satisfying ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2016-01-13 21:58 ` Rafael J. Wysocki 2016-01-13 22:07 ` Rafael J. Wysocki @ 2016-01-14 10:33 ` Sudeep Holla 1 sibling, 0 replies; 16+ messages in thread From: Sudeep Holla @ 2016-01-14 10:33 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Sudeep Holla, riel, Rafael Wysocki, Linux PM list, open list, arjan, Len Brown, Daniel Lezcano On 13/01/16 21:58, Rafael J. Wysocki wrote: > Hi, > > On Wed, Jan 13, 2016 at 6:27 PM, Sudeep Holla <sudeep.holla@arm.com> wrote: >> Hi Rik, >> >> This change break idle on ARM64(may be on other ARM?) platform. >> Sorry for reporting late, but missed to check cpuidle in -next. > > OK, so first of all, how exactly is idle broken on those systems? > Sorry for the hasty bug report. > Do they crash or does something different happen? If something > different happens, then what's that? > No they just don't enter any idle states(as if CPUIdle was disabled) [...] >> >> diff --git i/drivers/cpuidle/governors/menu.c w/drivers/cpuidle/governors/menu.c >> index 7b0971d97cc3..7c7f4059705a 100644 >> --- i/drivers/cpuidle/governors/menu.c >> +++ w/drivers/cpuidle/governors/menu.c >> @@ -330,7 +330,8 @@ 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 (((interactivity_req && interactivity_req > 20) || > > Well, if interactivity_req > 20, then it also is different from 0, so > the first check should not be necessary here. > True, I just wanted to avoid using interactivity_req when 0, it was just a quick hack. >> + data->next_timer_us > 20) && > > I guess that this simply restores the previous behavior on the > platforms in question. > Yes. > Now, the reason why it may matter is because > CPUIDLE_DRIVER_STATE_START is 0 and so data->last_state_idx may end up > as -1 on them. So I think this piece of code only ever makes sense if > CPUIDLE_DRIVER_STATE_START is 1. > That's correct. -- Regards, Sudeep ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 3/3] cpuidle,menu: smooth out measured_us calculation 2015-11-03 22:34 [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) riel 2015-11-03 22:34 ` [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us riel 2015-11-03 22:34 ` [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling riel @ 2015-11-03 22:34 ` riel 2015-11-04 16:02 ` Arjan van de Ven 2015-11-05 22:34 ` [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) Rafael J. Wysocki 3 siblings, 1 reply; 16+ messages in thread From: riel @ 2015-11-03 22:34 UTC (permalink / raw) To: linux-pm; +Cc: linux-kernel, rafael.j.wysocki, arjan, len.brown, daniel.lezcano From: Rik van Riel <riel@redhat.com> The cpuidle state tables contain the maximum exit latency for each cpuidle state. On x86, that is the exit latency for when the entire package goes into that same idle state. However, a lot of the time we only go into the core idle state, not the package idle state. This means we see a much smaller exit latency. We have no way to detect whether we went into the core or package idle state while idle, and that is ok. However, the current menu_update logic does have the potential to trip up the repeating pattern detection in get_typical_interval. If the system is experiencing an exit latency near the idle state's exit latency, some of the samples will have exit_us subtracted, while others will not. This turns a repeating pattern into mush, potentially breaking get_typical_interval. Furthermore, for smaller sleep intervals, we know the chance that all the cores in the package went to the same idle state are fairly small. Dividing the measured_us by two, instead of subtracting the full exit latency when hitting a small measured_us, will reduce the error. Signed-off-by: Rik van Riel <riel@redhat.com> --- drivers/cpuidle/governors/menu.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c index b1a55731f921..7b0971d97cc3 100644 --- a/drivers/cpuidle/governors/menu.c +++ b/drivers/cpuidle/governors/menu.c @@ -404,8 +404,10 @@ static void menu_update(struct cpuidle_driver *drv, struct cpuidle_device *dev) measured_us = cpuidle_get_last_residency(dev); /* Deduct exit latency */ - if (measured_us > target->exit_latency) + if (measured_us > 2 * target->exit_latency) measured_us -= target->exit_latency; + else + measured_us /= 2; /* Make sure our coefficients do not exceed unity */ if (measured_us > data->next_timer_us) -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 3/3] cpuidle,menu: smooth out measured_us calculation 2015-11-03 22:34 ` [PATCH 3/3] cpuidle,menu: smooth out measured_us calculation riel @ 2015-11-04 16:02 ` Arjan van de Ven 0 siblings, 0 replies; 16+ messages in thread From: Arjan van de Ven @ 2015-11-04 16:02 UTC (permalink / raw) To: riel, linux-pm; +Cc: linux-kernel, rafael.j.wysocki, len.brown, daniel.lezcano On 11/3/2015 2:34 PM, riel@redhat.com wrote: > Furthermore, for smaller sleep intervals, we know the chance that > all the cores in the package went to the same idle state are fairly > small. Dividing the measured_us by two, instead of subtracting the > full exit latency when hitting a small measured_us, will reduce the > error. there is no perfect answer for this issue; but at least this makes the situation a lot better, so Acked-by: Arjan van de Ven <arjan@linux.intel.com> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) 2015-11-03 22:34 [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) riel ` (2 preceding siblings ...) 2015-11-03 22:34 ` [PATCH 3/3] cpuidle,menu: smooth out measured_us calculation riel @ 2015-11-05 22:34 ` Rafael J. Wysocki 2015-11-06 2:26 ` Rik van Riel 3 siblings, 1 reply; 16+ messages in thread From: Rafael J. Wysocki @ 2015-11-05 22:34 UTC (permalink / raw) To: riel Cc: linux-pm, linux-kernel, rafael.j.wysocki, arjan, len.brown, daniel.lezcano On Tuesday, November 03, 2015 05:34:16 PM riel@redhat.com wrote: > While working on a paravirt cpuidle driver for KVM guests, I > noticed a number of small logic errors in the menu governor > code. > > These patches should get rid of some artifacts that can break > the logic in the menu governor under certain corner cases, and > make idle state selection work better on CPUs with long C1 exit > latencies. > > I have not seen any adverse effects with them in my (quick) > tests. As expected, they do not seem to do much on systems with > many power states and very low C1 exit latencies and target residencies. Thanks! The patches look good to me. I might apply [1-2/3] right away, but I'm a bit hesitant about the [3/3] (I'd like it to spend some time in linux-next before it goes to Linus). Also, we've lived without these changes for quite some time and I don't want to stretch the process too much, so I'll queue them up for v4.5 if that's not a problem. Thanks, Rafael ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) 2015-11-05 22:34 ` [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) Rafael J. Wysocki @ 2015-11-06 2:26 ` Rik van Riel 0 siblings, 0 replies; 16+ messages in thread From: Rik van Riel @ 2015-11-06 2:26 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, linux-kernel, rafael.j.wysocki, arjan, len.brown, daniel.lezcano On 11/05/2015 05:34 PM, Rafael J. Wysocki wrote: > On Tuesday, November 03, 2015 05:34:16 PM riel@redhat.com wrote: >> While working on a paravirt cpuidle driver for KVM guests, I >> noticed a number of small logic errors in the menu governor >> code. >> >> These patches should get rid of some artifacts that can break >> the logic in the menu governor under certain corner cases, and >> make idle state selection work better on CPUs with long C1 exit >> latencies. >> >> I have not seen any adverse effects with them in my (quick) >> tests. As expected, they do not seem to do much on systems with >> many power states and very low C1 exit latencies and target residencies. > > Thanks! > > The patches look good to me. > > I might apply [1-2/3] right away, but I'm a bit hesitant about the [3/3] (I'd > like it to spend some time in linux-next before it goes to Linus). Also, we've > lived without these changes for quite some time and I don't want to stretch the > process too much, so I'll queue them up for v4.5 if that's not a problem. Not a problem at all. I am all for taking these changes carefully, and seeing what happens. I did some basic testing with it, but the permutations of what can happen with cpuidle management are just too many to predict in advance everything that could happen. -- All rights reversed ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/3] cpuidle: small improvements & fixes for menu governor @ 2015-10-28 22:46 riel 2015-10-28 22:46 ` [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling riel 0 siblings, 1 reply; 16+ messages in thread From: riel @ 2015-10-28 22:46 UTC (permalink / raw) To: linux-kernel Cc: arjan, khilman, len.brown, rafael.j.wysocki, daniel.lezcano, javi.merino, tuukka.tikkanen While working on a paravirt cpuidle driver for KVM guests, I noticed a number of small logic errors in the menu governor code. These patches should get rid of some artifacts that can break the logic in the menu governor under certain corner cases, and make idle state selection work better on CPUs with long C1 exit latencies. I have not seen any adverse effects with them in my (quick) tests. As expected, they do not seem to do much on systems with many power states and very low C1 exit latencies and target residencies. ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling 2015-10-28 22:46 [PATCH 0/3] cpuidle: small improvements & fixes for menu governor riel @ 2015-10-28 22:46 ` riel 0 siblings, 0 replies; 16+ messages in thread From: riel @ 2015-10-28 22:46 UTC (permalink / raw) To: linux-kernel Cc: arjan, khilman, len.brown, rafael.j.wysocki, daniel.lezcano, javi.merino, tuukka.tikkanen From: Rik van Riel <riel@redhat.com> The menu governor carefully figures out how much time we typically sleep for an estimated sleep interval, or whether there is a repeating pattern going on, and corrects that estimate for the CPU load. Then it proceeds to ignore that information when determining whether or not to consider polling. This is not a big deal on most x86 CPUs, which have very low C1 latencies, and the patch should not have any effect on those CPUs. However, certain CPUs (eg. Atom) have much higher C1 latencies, and it would be good to not waste performance and power on those CPUs if we are expecting a very low wakeup latency. Disable polling based on the estimated interactivity requirement, not on the time to the next timer interrupt. 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 ecc242a586c9..b1a55731f921 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 (data->next_timer_us > 20 && + if (interactivity_req > 20 && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; -- 2.1.0 ^ permalink raw reply related [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-01-15 0:45 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-11-03 22:34 [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) riel 2015-11-03 22:34 ` [PATCH 1/3] cpuidle,x86: increase forced cut-off for polling to 20us riel 2015-11-04 16:00 ` Arjan van de Ven 2015-11-03 22:34 ` [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling riel 2015-11-04 16:01 ` Arjan van de Ven 2016-01-13 17:27 ` Sudeep Holla 2016-01-13 21:58 ` Rafael J. Wysocki 2016-01-13 22:07 ` Rafael J. Wysocki 2016-01-14 10:40 ` Sudeep Holla 2016-01-15 0:46 ` Rafael J. Wysocki 2016-01-14 10:33 ` Sudeep Holla 2015-11-03 22:34 ` [PATCH 3/3] cpuidle,menu: smooth out measured_us calculation riel 2015-11-04 16:02 ` Arjan van de Ven 2015-11-05 22:34 ` [PATCH 0/3] cpuidle: small improvements & fixes for menu governor (resend) Rafael J. Wysocki 2015-11-06 2:26 ` Rik van Riel -- strict thread matches above, loose matches on Subject: below -- 2015-10-28 22:46 [PATCH 0/3] cpuidle: small improvements & fixes for menu governor riel 2015-10-28 22:46 ` [PATCH 2/3] cpuidle,menu: use interactivity_req to disable polling riel
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.