* [PATCH 1/6] cpuidle: menu: Fix wakeup statistics updates for polling state
2018-10-02 21:41 [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
@ 2018-10-02 21:42 ` Rafael J. Wysocki
2018-10-04 8:19 ` Daniel Lezcano
2018-10-02 21:42 ` [PATCH 2/6] cpuidle: menu: Compute first_idx when latency_req is known Rafael J. Wysocki
` (5 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-02 21:42 UTC (permalink / raw)
To: Linux PM; +Cc: Peter Zijlstra, LKML, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
If the CPU exits the "polling" state due to the time limit in the
loop in poll_idle(), this is not a real wakeup and it just means
that the "polling" state selection was not adequate. The governor
mispredicted short idle duration, but had a more suitable state been
selected, the CPU might have spent more time in it. In fact, there
is no reason to expect that there would have been a wakeup event
earlier than the next timer in that case.
Handling such cases as regular wakeups in menu_update() may cause the
menu governor to make suboptimal decisions going forward, but ignoring
them altogether would not be correct either, because every time
menu_select() is invoked, it makes a separate new attempt to predict
the idle duration taking distinct time to the closest timer event as
input and the outcomes of all those attempts should be recorded.
For this reason, make menu_update() always assume that if the
"polling" state was exited due to the time limit, the next proper
wakeup event for the CPU would be the next timer event (not
including the tick).
Fixes: a37b969a61c1 "cpuidle: poll_state: Add time limit to poll_idle()"
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governors/menu.c | 10 ++++++++++
drivers/cpuidle/poll_state.c | 6 +++++-
include/linux/cpuidle.h | 1 +
3 files changed, 16 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -511,6 +511,16 @@ static void menu_update(struct cpuidle_d
* duration predictor do a better job next time.
*/
measured_us = 9 * MAX_INTERESTING / 10;
+ } else if ((drv->states[last_idx].flags & CPUIDLE_FLAG_POLLING) &&
+ dev->poll_time_limit) {
+ /*
+ * The CPU exited the "polling" state due to a time limit, so
+ * the idle duration prediction leading to the selection of that
+ * state was inaccurate. If a better prediction had been made,
+ * the CPU might have been woken up from idle by the next timer.
+ * Assume that to be the case.
+ */
+ measured_us = data->next_timer_us;
} else {
/* measured value */
measured_us = dev->last_residency;
Index: linux-pm/include/linux/cpuidle.h
===================================================================
--- linux-pm.orig/include/linux/cpuidle.h
+++ linux-pm/include/linux/cpuidle.h
@@ -81,6 +81,7 @@ struct cpuidle_device {
unsigned int registered:1;
unsigned int enabled:1;
unsigned int use_deepest_state:1;
+ unsigned int poll_time_limit:1;
unsigned int cpu;
int last_residency;
Index: linux-pm/drivers/cpuidle/poll_state.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/poll_state.c
+++ linux-pm/drivers/cpuidle/poll_state.c
@@ -17,6 +17,8 @@ static int __cpuidle poll_idle(struct cp
{
u64 time_start = local_clock();
+ dev->poll_time_limit = false;
+
local_irq_enable();
if (!current_set_polling_and_test()) {
unsigned int loop_count = 0;
@@ -27,8 +29,10 @@ static int __cpuidle poll_idle(struct cp
continue;
loop_count = 0;
- if (local_clock() - time_start > POLL_IDLE_TIME_LIMIT)
+ if (local_clock() - time_start > POLL_IDLE_TIME_LIMIT) {
+ dev->poll_time_limit = true;
break;
+ }
}
}
current_clr_polling();
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/6] cpuidle: menu: Fix wakeup statistics updates for polling state
2018-10-02 21:42 ` [PATCH 1/6] cpuidle: menu: Fix wakeup statistics updates for polling state Rafael J. Wysocki
@ 2018-10-04 8:19 ` Daniel Lezcano
0 siblings, 0 replies; 28+ messages in thread
From: Daniel Lezcano @ 2018-10-04 8:19 UTC (permalink / raw)
To: Rafael J. Wysocki, Linux PM; +Cc: Peter Zijlstra, LKML
On 02/10/2018 23:42, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If the CPU exits the "polling" state due to the time limit in the
> loop in poll_idle(), this is not a real wakeup and it just means
> that the "polling" state selection was not adequate. The governor
> mispredicted short idle duration, but had a more suitable state been
> selected, the CPU might have spent more time in it. In fact, there
> is no reason to expect that there would have been a wakeup event
> earlier than the next timer in that case.
>
> Handling such cases as regular wakeups in menu_update() may cause the
> menu governor to make suboptimal decisions going forward, but ignoring
> them altogether would not be correct either, because every time
> menu_select() is invoked, it makes a separate new attempt to predict
> the idle duration taking distinct time to the closest timer event as
> input and the outcomes of all those attempts should be recorded.
>
> For this reason, make menu_update() always assume that if the
> "polling" state was exited due to the time limit, the next proper
> wakeup event for the CPU would be the next timer event (not
> including the tick).
>
> Fixes: a37b969a61c1 "cpuidle: poll_state: Add time limit to poll_idle()"
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> drivers/cpuidle/governors/menu.c | 10 ++++++++++
> drivers/cpuidle/poll_state.c | 6 +++++-
> include/linux/cpuidle.h | 1 +
> 3 files changed, 16 insertions(+), 1 deletion(-)
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -511,6 +511,16 @@ static void menu_update(struct cpuidle_d
> * duration predictor do a better job next time.
> */
> measured_us = 9 * MAX_INTERESTING / 10;
> + } else if ((drv->states[last_idx].flags & CPUIDLE_FLAG_POLLING) &&
> + dev->poll_time_limit) {
> + /*
> + * The CPU exited the "polling" state due to a time limit, so
> + * the idle duration prediction leading to the selection of that
> + * state was inaccurate. If a better prediction had been made,
> + * the CPU might have been woken up from idle by the next timer.
> + * Assume that to be the case.
> + */
> + measured_us = data->next_timer_us;
> } else {
> /* measured value */
> measured_us = dev->last_residency;
> Index: linux-pm/include/linux/cpuidle.h
> ===================================================================
> --- linux-pm.orig/include/linux/cpuidle.h
> +++ linux-pm/include/linux/cpuidle.h
> @@ -81,6 +81,7 @@ struct cpuidle_device {
> unsigned int registered:1;
> unsigned int enabled:1;
> unsigned int use_deepest_state:1;
> + unsigned int poll_time_limit:1;
> unsigned int cpu;
>
> int last_residency;
> Index: linux-pm/drivers/cpuidle/poll_state.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/poll_state.c
> +++ linux-pm/drivers/cpuidle/poll_state.c
> @@ -17,6 +17,8 @@ static int __cpuidle poll_idle(struct cp
> {
> u64 time_start = local_clock();
>
> + dev->poll_time_limit = false;
> +
> local_irq_enable();
> if (!current_set_polling_and_test()) {
> unsigned int loop_count = 0;
> @@ -27,8 +29,10 @@ static int __cpuidle poll_idle(struct cp
> continue;
>
> loop_count = 0;
> - if (local_clock() - time_start > POLL_IDLE_TIME_LIMIT)
> + if (local_clock() - time_start > POLL_IDLE_TIME_LIMIT) {
> + dev->poll_time_limit = true;
> break;
> + }
> }
> }
> current_clr_polling();
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/6] cpuidle: menu: Compute first_idx when latency_req is known
2018-10-02 21:41 [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
2018-10-02 21:42 ` [PATCH 1/6] cpuidle: menu: Fix wakeup statistics updates for polling state Rafael J. Wysocki
@ 2018-10-02 21:42 ` Rafael J. Wysocki
2018-10-04 14:22 ` Daniel Lezcano
2018-10-02 21:44 ` [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select() Rafael J. Wysocki
` (4 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-02 21:42 UTC (permalink / raw)
To: Linux PM; +Cc: Peter Zijlstra, LKML, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Since menu_select() can only set first_idx to 1 if the exit latency
of the second state is not greater than the latency limit, it should
first determine that limit. Thus first_idx should be computed after
the "interactivity" factor has been taken into account.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governors/menu.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -322,22 +322,6 @@ static int menu_select(struct cpuidle_dr
expected_interval = get_typical_interval(data);
expected_interval = min(expected_interval, data->next_timer_us);
- first_idx = 0;
- if (drv->states[0].flags & CPUIDLE_FLAG_POLLING) {
- struct cpuidle_state *s = &drv->states[1];
- unsigned int polling_threshold;
-
- /*
- * Default to a physical idle state, not to busy polling, unless
- * a timer is going to trigger really really soon.
- */
- polling_threshold = max_t(unsigned int, 20, s->target_residency);
- if (data->next_timer_us > polling_threshold &&
- latency_req > s->exit_latency && !s->disabled &&
- !dev->states_usage[1].disable)
- first_idx = 1;
- }
-
/*
* Use the lowest expected idle interval to pick the idle state.
*/
@@ -364,6 +348,22 @@ static int menu_select(struct cpuidle_dr
latency_req = interactivity_req;
}
+ first_idx = 0;
+ if (drv->states[0].flags & CPUIDLE_FLAG_POLLING) {
+ struct cpuidle_state *s = &drv->states[1];
+ unsigned int polling_threshold;
+
+ /*
+ * Default to a physical idle state, not to busy polling, unless
+ * a timer is going to trigger really really soon.
+ */
+ polling_threshold = max_t(unsigned int, 20, s->target_residency);
+ if (data->next_timer_us > polling_threshold &&
+ latency_req > s->exit_latency && !s->disabled &&
+ !dev->states_usage[1].disable)
+ first_idx = 1;
+ }
+
/*
* Find the idle state with the lowest power while satisfying
* our constraints.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-02 21:41 [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
2018-10-02 21:42 ` [PATCH 1/6] cpuidle: menu: Fix wakeup statistics updates for polling state Rafael J. Wysocki
2018-10-02 21:42 ` [PATCH 2/6] cpuidle: menu: Compute first_idx when latency_req is known Rafael J. Wysocki
@ 2018-10-02 21:44 ` Rafael J. Wysocki
2018-10-04 7:46 ` Peter Zijlstra
2018-10-04 14:51 ` Daniel Lezcano
2018-10-02 21:45 ` [PATCH 4/6] cpuidle: menu: Do not update last_state_idx in menu_select() Rafael J. Wysocki
` (3 subsequent siblings)
6 siblings, 2 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-02 21:44 UTC (permalink / raw)
To: Linux PM; +Cc: Peter Zijlstra, LKML, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Rearrange the code in menu_select() so that the loop over idle states
always starts from 0 and get rid of the first_idx variable.
While at it, add two empty lines to separate conditional statements
one another.
No intentional behavior changes.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governors/menu.c | 32 ++++++++++++++------------------
1 file changed, 14 insertions(+), 18 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -285,7 +285,6 @@ static int menu_select(struct cpuidle_dr
struct menu_device *data = this_cpu_ptr(&menu_devices);
int latency_req = cpuidle_governor_latency_req(dev->cpu);
int i;
- int first_idx;
int idx;
unsigned int interactivity_req;
unsigned int expected_interval;
@@ -348,36 +347,33 @@ static int menu_select(struct cpuidle_dr
latency_req = interactivity_req;
}
- first_idx = 0;
- if (drv->states[0].flags & CPUIDLE_FLAG_POLLING) {
- struct cpuidle_state *s = &drv->states[1];
- unsigned int polling_threshold;
-
- /*
- * Default to a physical idle state, not to busy polling, unless
- * a timer is going to trigger really really soon.
- */
- polling_threshold = max_t(unsigned int, 20, s->target_residency);
- if (data->next_timer_us > polling_threshold &&
- latency_req > s->exit_latency && !s->disabled &&
- !dev->states_usage[1].disable)
- first_idx = 1;
- }
-
/*
* Find the idle state with the lowest power while satisfying
* our constraints.
*/
idx = -1;
- for (i = first_idx; i < drv->state_count; i++) {
+ 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)
continue;
+
if (idx == -1)
idx = i; /* first enabled state */
+
if (s->target_residency > predicted_us) {
+ /*
+ * Use a physical idle state, not busy polling, unless
+ * a timer is going to trigger really really soon.
+ */
+ if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
+ i == idx + 1 && latency_req > s->exit_latency &&
+ data->next_timer_us > max_t(unsigned int, 20,
+ s->target_residency)) {
+ idx = i;
+ break;
+ }
if (predicted_us < TICK_USEC)
break;
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-02 21:44 ` [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select() Rafael J. Wysocki
@ 2018-10-04 7:46 ` Peter Zijlstra
2018-10-04 7:53 ` Rafael J. Wysocki
2018-10-04 14:51 ` Daniel Lezcano
1 sibling, 1 reply; 28+ messages in thread
From: Peter Zijlstra @ 2018-10-04 7:46 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM, LKML, Daniel Lezcano
On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote:
> idx = -1;
> - for (i = first_idx; i < drv->state_count; i++) {
> + 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)
> continue;
> +
> if (idx == -1)
> idx = i; /* first enabled state */
> +
> if (s->target_residency > predicted_us) {
> + /*
> + * Use a physical idle state, not busy polling, unless
> + * a timer is going to trigger really really soon.
> + */
> + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
> + i == idx + 1 && latency_req > s->exit_latency &&
> + data->next_timer_us > max_t(unsigned int, 20,
> + s->target_residency)) {
Not new in this patch, but this is where I really noticed it; that 20,
should that not be something like: POLL_IDLE_TIME_LIMIT / NSEC_PER_USEC
?
> + idx = i;
> + break;
> + }
> if (predicted_us < TICK_USEC)
> break;
>
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-04 7:46 ` Peter Zijlstra
@ 2018-10-04 7:53 ` Rafael J. Wysocki
2018-10-04 8:00 ` Peter Zijlstra
0 siblings, 1 reply; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-04 7:53 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Rafael J. Wysocki, Linux PM, Linux Kernel Mailing List, Daniel Lezcano
On Thu, Oct 4, 2018 at 9:46 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote:
> > idx = -1;
> > - for (i = first_idx; i < drv->state_count; i++) {
> > + 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)
> > continue;
> > +
> > if (idx == -1)
> > idx = i; /* first enabled state */
> > +
> > if (s->target_residency > predicted_us) {
> > + /*
> > + * Use a physical idle state, not busy polling, unless
> > + * a timer is going to trigger really really soon.
> > + */
> > + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
> > + i == idx + 1 && latency_req > s->exit_latency &&
> > + data->next_timer_us > max_t(unsigned int, 20,
> > + s->target_residency)) {
>
> Not new in this patch, but this is where I really noticed it; that 20,
> should that not be something like: POLL_IDLE_TIME_LIMIT / NSEC_PER_USEC
> ?
The POLL_IDLE_TIME_LIMIT is how much time we allow it to spin in
idle_poll() and I'm not sure it is related. Besides, I want it to go
away actually (https://patchwork.kernel.org/patch/10624117/).
I could use a separate symbol for this particular magic number, but it
has been magic forever and it is used just in this one place, so ...
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-04 7:53 ` Rafael J. Wysocki
@ 2018-10-04 8:00 ` Peter Zijlstra
0 siblings, 0 replies; 28+ messages in thread
From: Peter Zijlstra @ 2018-10-04 8:00 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Linux PM, Linux Kernel Mailing List, Daniel Lezcano
On Thu, Oct 04, 2018 at 09:53:39AM +0200, Rafael J. Wysocki wrote:
> On Thu, Oct 4, 2018 at 9:46 AM Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote:
> > > idx = -1;
> > > - for (i = first_idx; i < drv->state_count; i++) {
> > > + 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)
> > > continue;
> > > +
> > > if (idx == -1)
> > > idx = i; /* first enabled state */
> > > +
> > > if (s->target_residency > predicted_us) {
> > > + /*
> > > + * Use a physical idle state, not busy polling, unless
> > > + * a timer is going to trigger really really soon.
> > > + */
> > > + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
> > > + i == idx + 1 && latency_req > s->exit_latency &&
> > > + data->next_timer_us > max_t(unsigned int, 20,
> > > + s->target_residency)) {
> >
> > Not new in this patch, but this is where I really noticed it; that 20,
> > should that not be something like: POLL_IDLE_TIME_LIMIT / NSEC_PER_USEC
> > ?
>
> The POLL_IDLE_TIME_LIMIT is how much time we allow it to spin in
> idle_poll() and I'm not sure it is related. Besides, I want it to go
> away actually (https://patchwork.kernel.org/patch/10624117/).
Ah, ok. Making it go away is better still!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-02 21:44 ` [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select() Rafael J. Wysocki
2018-10-04 7:46 ` Peter Zijlstra
@ 2018-10-04 14:51 ` Daniel Lezcano
2018-10-04 17:19 ` Rafael J. Wysocki
1 sibling, 1 reply; 28+ messages in thread
From: Daniel Lezcano @ 2018-10-04 14:51 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM, Peter Zijlstra, LKML
On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Rearrange the code in menu_select() so that the loop over idle states
> always starts from 0 and get rid of the first_idx variable.
>
> While at it, add two empty lines to separate conditional statements
> one another.
>
> No intentional behavior changes.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
This code is becoming a bit complex to follow :/
May be I missed something, but it is not possible to enter the condition without
idx != 0, no ? (I meant the condition if ((drv->states[idx].flags &
FLAG_POLLING))
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> drivers/cpuidle/governors/menu.c | 32 ++++++++++++++------------------
> 1 file changed, 14 insertions(+), 18 deletions(-)
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -285,7 +285,6 @@ static int menu_select(struct cpuidle_dr
> struct menu_device *data = this_cpu_ptr(&menu_devices);
> int latency_req = cpuidle_governor_latency_req(dev->cpu);
> int i;
> - int first_idx;
> int idx;
> unsigned int interactivity_req;
> unsigned int expected_interval;
> @@ -348,36 +347,33 @@ static int menu_select(struct cpuidle_dr
> latency_req = interactivity_req;
> }
>
> - first_idx = 0;
> - if (drv->states[0].flags & CPUIDLE_FLAG_POLLING) {
> - struct cpuidle_state *s = &drv->states[1];
> - unsigned int polling_threshold;
> -
> - /*
> - * Default to a physical idle state, not to busy polling, unless
> - * a timer is going to trigger really really soon.
> - */
> - polling_threshold = max_t(unsigned int, 20, s->target_residency);
> - if (data->next_timer_us > polling_threshold &&
> - latency_req > s->exit_latency && !s->disabled &&
> - !dev->states_usage[1].disable)
> - first_idx = 1;
> - }
> -
> /*
> * Find the idle state with the lowest power while satisfying
> * our constraints.
> */
> idx = -1;
> - for (i = first_idx; i < drv->state_count; i++) {
> + 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)
> continue;
> +
> if (idx == -1)
> idx = i; /* first enabled state */
> +
> if (s->target_residency > predicted_us) {
> + /*
> + * Use a physical idle state, not busy polling, unless
> + * a timer is going to trigger really really soon.
> + */
> + if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
> + i == idx + 1 && latency_req > s->exit_latency &&
> + data->next_timer_us > max_t(unsigned int, 20,
> + s->target_residency)) {
> + idx = i;
> + break;
> + }
> if (predicted_us < TICK_USEC)
> break;
>
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-04 14:51 ` Daniel Lezcano
@ 2018-10-04 17:19 ` Rafael J. Wysocki
2018-10-05 8:35 ` Daniel Lezcano
0 siblings, 1 reply; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-04 17:19 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Rafael J. Wysocki, Linux PM, Peter Zijlstra, Linux Kernel Mailing List
On Thu, Oct 4, 2018 at 4:51 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Rearrange the code in menu_select() so that the loop over idle states
> > always starts from 0 and get rid of the first_idx variable.
> >
> > While at it, add two empty lines to separate conditional statements
> > one another.
> >
> > No intentional behavior changes.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
>
> This code is becoming a bit complex to follow :/
>
> May be I missed something, but it is not possible to enter the condition without
> idx != 0, no ? (I meant the condition if ((drv->states[idx].flags &
> FLAG_POLLING))
Not sure what you mean.
We start with idx = -1, i = 0. If state[0] is enabled, idx becomes 0.
If the target residency or exit latency of state[0] are already beyond
the limits, we just bail out and state[0] will be returned.
If not, we go to i = 1, but idx is still 0. If the target residency
of state[1] is beyond predicted_us (which is the interesting case), we
check (drv->states[0].flags & FLAG_POLLING) and so on.
Currently, the polling state must be state[0] (if used at all) anyway.
> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>
Thanks!
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-04 17:19 ` Rafael J. Wysocki
@ 2018-10-05 8:35 ` Daniel Lezcano
2018-10-05 8:49 ` Rafael J. Wysocki
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Lezcano @ 2018-10-05 8:35 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Linux PM, Peter Zijlstra, Linux Kernel Mailing List
On 04/10/2018 19:19, Rafael J. Wysocki wrote:
> On Thu, Oct 4, 2018 at 4:51 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>>
>> On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote:
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Rearrange the code in menu_select() so that the loop over idle states
>>> always starts from 0 and get rid of the first_idx variable.
>>>
>>> While at it, add two empty lines to separate conditional statements
>>> one another.
>>>
>>> No intentional behavior changes.
>>>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>
>> This code is becoming a bit complex to follow :/
>>
>> May be I missed something, but it is not possible to enter the condition without
>> idx != 0, no ? (I meant the condition if ((drv->states[idx].flags &
>> FLAG_POLLING))
>
> Not sure what you mean.
Yes, sorry let me clarify.
I meant the flag polling is always on state[0], so if the flag is set
then idx == 0.
We have the conditions:
drv->states[idx].flags & CPUIDLE_FLAG_POLLING
If it is true then idx is zero.
Then comes the second condition:
i == idx + 1
because of the above, idx is zero, then it can become i == 1.
Then the variable assignation:
idx = i can be replaced by idx = 1
> We start with idx = -1, i = 0. If state[0] is enabled, idx becomes 0.
>
> If the target residency or exit latency of state[0] are already beyond
> the limits, we just bail out and state[0] will be returned.
>
> If not, we go to i = 1, but idx is still 0. If the target residency
> of state[1] is beyond predicted_us (which is the interesting case), we
> check (drv->states[0].flags & FLAG_POLLING) and so on.
>
> Currently, the polling state must be state[0] (if used at all) anyway.
>
>> Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>>
>
> Thanks!
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select()
2018-10-05 8:35 ` Daniel Lezcano
@ 2018-10-05 8:49 ` Rafael J. Wysocki
0 siblings, 0 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-05 8:49 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Linux PM, Peter Zijlstra,
Linux Kernel Mailing List
On Fri, Oct 5, 2018 at 10:35 AM Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
>
> On 04/10/2018 19:19, Rafael J. Wysocki wrote:
> > On Thu, Oct 4, 2018 at 4:51 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
> >>
> >> On Tue, Oct 02, 2018 at 11:44:06PM +0200, Rafael J. Wysocki wrote:
> >>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>>
> >>> Rearrange the code in menu_select() so that the loop over idle states
> >>> always starts from 0 and get rid of the first_idx variable.
> >>>
> >>> While at it, add two empty lines to separate conditional statements
> >>> one another.
> >>>
> >>> No intentional behavior changes.
> >>>
> >>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>> ---
> >>
> >> This code is becoming a bit complex to follow :/
> >>
> >> May be I missed something, but it is not possible to enter the condition without
> >> idx != 0, no ? (I meant the condition if ((drv->states[idx].flags &
> >> FLAG_POLLING))
> >
> > Not sure what you mean.
>
> Yes, sorry let me clarify.
>
> I meant the flag polling is always on state[0], so if the flag is set
> then idx == 0.
>
> We have the conditions:
>
> drv->states[idx].flags & CPUIDLE_FLAG_POLLING
>
> If it is true then idx is zero.
>
>
> Then comes the second condition:
>
> i == idx + 1
>
> because of the above, idx is zero, then it can become i == 1.
>
> Then the variable assignation:
>
> idx = i can be replaced by idx = 1
Yes, but the former works too. :-)
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/6] cpuidle: menu: Do not update last_state_idx in menu_select()
2018-10-02 21:41 [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
` (2 preceding siblings ...)
2018-10-02 21:44 ` [PATCH 3/6] cpuidle: menu: Get rid of first_idx from menu_select() Rafael J. Wysocki
@ 2018-10-02 21:45 ` Rafael J. Wysocki
2018-10-04 14:57 ` Daniel Lezcano
2018-10-02 21:46 ` [PATCH 5/6] cpuidle: menu: Avoid computations for very close timers Rafael J. Wysocki
` (2 subsequent siblings)
6 siblings, 1 reply; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-02 21:45 UTC (permalink / raw)
To: Linux PM; +Cc: Peter Zijlstra, LKML, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
It is not necessary to update data->last_state_idx in menu_select()
as it only is used in menu_update() which only runs when
data->needs_update is set and that is set only when updating
data->last_state_idx in menu_reflect().
Accordingly, drop the update of data->last_state_idx from
menu_select() and get rid of the (now redundant) "out" label
from it.
No intentional behavior changes.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governors/menu.c | 7 ++-----
1 file changed, 2 insertions(+), 5 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -398,7 +398,7 @@ static int menu_select(struct cpuidle_dr
s->target_residency <= ktime_to_us(delta_next))
idx = i;
- goto out;
+ return idx;
}
if (s->exit_latency > latency_req) {
/*
@@ -445,10 +445,7 @@ static int menu_select(struct cpuidle_dr
}
}
-out:
- data->last_state_idx = idx;
-
- return data->last_state_idx;
+ return idx;
}
/**
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/6] cpuidle: menu: Avoid computations for very close timers
2018-10-02 21:41 [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
` (3 preceding siblings ...)
2018-10-02 21:45 ` [PATCH 4/6] cpuidle: menu: Do not update last_state_idx in menu_select() Rafael J. Wysocki
@ 2018-10-02 21:46 ` Rafael J. Wysocki
2018-10-04 15:50 ` Daniel Lezcano
2018-10-02 21:47 ` [PATCH 6/6] cpuidle: menu: Move the latency_req == 0 special case check Rafael J. Wysocki
2018-10-04 6:55 ` [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
6 siblings, 1 reply; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-02 21:46 UTC (permalink / raw)
To: Linux PM; +Cc: Peter Zijlstra, LKML, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
If the next timer event (with the tick excluded) is closer than the
target residency of the second state or the PM QoS latency constraint
is below its exit latency, state[0] will be used regardless of any
other factors, so skip the computations in menu_select() then and
return 0 straight away from it.
Still, do that after the bucket has been determined to avoid
disturbing the wakeup statistics in general.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governors/menu.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -309,6 +309,18 @@ static int menu_select(struct cpuidle_dr
get_iowait_load(&nr_iowaiters, &cpu_load);
data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
+ if (unlikely(drv->state_count <= 1) ||
+ ((data->next_timer_us < drv->states[1].target_residency ||
+ latency_req < drv->states[1].exit_latency) &&
+ !drv->states[0].disabled && !dev->states_usage[0].disable)) {
+ /*
+ * In this case state[0] will be used no matter what, so return
+ * it right away and keep the tick running.
+ */
+ *stop_tick = false;
+ return 0;
+ }
+
/*
* Force the result of multiplication to be 64 bits even if both
* operands are 32 bits.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/6] cpuidle: menu: Avoid computations for very close timers
2018-10-02 21:46 ` [PATCH 5/6] cpuidle: menu: Avoid computations for very close timers Rafael J. Wysocki
@ 2018-10-04 15:50 ` Daniel Lezcano
2018-10-04 17:11 ` Rafael J. Wysocki
0 siblings, 1 reply; 28+ messages in thread
From: Daniel Lezcano @ 2018-10-04 15:50 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: Linux PM, Peter Zijlstra, LKML
On Tue, Oct 02, 2018 at 11:46:28PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> If the next timer event (with the tick excluded) is closer than the
> target residency of the second state or the PM QoS latency constraint
> is below its exit latency, state[0] will be used regardless of any
> other factors, so skip the computations in menu_select() then and
> return 0 straight away from it.
>
> Still, do that after the bucket has been determined to avoid
> disturbing the wakeup statistics in general.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/cpuidle/governors/menu.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -309,6 +309,18 @@ static int menu_select(struct cpuidle_dr
> get_iowait_load(&nr_iowaiters, &cpu_load);
> data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
>
> + if (unlikely(drv->state_count <= 1) ||
I'm not sure this test is necessary.
If state_count == 0, we don't have to select anything as the cpuidle can't register because of:
static int __cpuidle_register_driver(struct cpuidle_driver *drv)
{
int ret;
if (!drv || !drv->state_count)
return -EINVAL;
[ ... ]
}
If state_count == 1, then there is nothing to select, it is always the state 0.
> + ((data->next_timer_us < drv->states[1].target_residency ||
> + latency_req < drv->states[1].exit_latency) &&
> + !drv->states[0].disabled && !dev->states_usage[0].disable)) {
> + /*
> + * In this case state[0] will be used no matter what, so return
> + * it right away and keep the tick running.
> + */
> + *stop_tick = false;
> + return 0;
> + }
> +
> /*
> * Force the result of multiplication to be 64 bits even if both
> * operands are 32 bits.
>
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 5/6] cpuidle: menu: Avoid computations for very close timers
2018-10-04 15:50 ` Daniel Lezcano
@ 2018-10-04 17:11 ` Rafael J. Wysocki
0 siblings, 0 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-04 17:11 UTC (permalink / raw)
To: Daniel Lezcano
Cc: Rafael J. Wysocki, Linux PM, Peter Zijlstra, Linux Kernel Mailing List
On Thu, Oct 4, 2018 at 5:50 PM Daniel Lezcano <daniel.lezcano@linaro.org> wrote:
>
> On Tue, Oct 02, 2018 at 11:46:28PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > If the next timer event (with the tick excluded) is closer than the
> > target residency of the second state or the PM QoS latency constraint
> > is below its exit latency, state[0] will be used regardless of any
> > other factors, so skip the computations in menu_select() then and
> > return 0 straight away from it.
> >
> > Still, do that after the bucket has been determined to avoid
> > disturbing the wakeup statistics in general.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > drivers/cpuidle/governors/menu.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > Index: linux-pm/drivers/cpuidle/governors/menu.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> > +++ linux-pm/drivers/cpuidle/governors/menu.c
> > @@ -309,6 +309,18 @@ static int menu_select(struct cpuidle_dr
> > get_iowait_load(&nr_iowaiters, &cpu_load);
> > data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
> >
> > + if (unlikely(drv->state_count <= 1) ||
>
> I'm not sure this test is necessary.
Yes, it is IMO.
Strictly speaking it prevents state[1] from being accessed if the
count is not 2 at least.
> If state_count == 0, we don't have to select anything as the cpuidle can't register because of:
>
> static int __cpuidle_register_driver(struct cpuidle_driver *drv)
> {
> int ret;
>
> if (!drv || !drv->state_count)
> return -EINVAL;
>
> [ ... ]
> }
>
> If state_count == 1, then there is nothing to select, it is always the state 0.
Which is why it is better to simply return 0 right away in this case. :-)
I guess I could compare state_count just to 1, but <= 1 works too.
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 6/6] cpuidle: menu: Move the latency_req == 0 special case check
2018-10-02 21:41 [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
` (4 preceding siblings ...)
2018-10-02 21:46 ` [PATCH 5/6] cpuidle: menu: Avoid computations for very close timers Rafael J. Wysocki
@ 2018-10-02 21:47 ` Rafael J. Wysocki
2018-10-04 6:55 ` [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
6 siblings, 0 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-02 21:47 UTC (permalink / raw)
To: Linux PM; +Cc: Peter Zijlstra, LKML, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
It is better to always update data->bucket before returning from
menu_select() so as to prevent disturbing the wakeup statistics
collected by the governor, so combine the latency_req == 0 special
check with the more general check below.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/governors/menu.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -297,19 +297,13 @@ static int menu_select(struct cpuidle_dr
data->needs_update = 0;
}
- /* Special case when user has set very strict latency requirement */
- if (unlikely(latency_req == 0)) {
- *stop_tick = false;
- return 0;
- }
-
/* determine the expected residency time, round up */
data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length(&delta_next));
get_iowait_load(&nr_iowaiters, &cpu_load);
data->bucket = which_bucket(data->next_timer_us, nr_iowaiters);
- if (unlikely(drv->state_count <= 1) ||
+ if (unlikely(drv->state_count <= 1 || latency_req == 0) ||
((data->next_timer_us < drv->states[1].target_residency ||
latency_req < drv->states[1].exit_latency) &&
!drv->states[0].disabled && !dev->states_usage[0].disable)) {
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-02 21:41 [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
` (5 preceding siblings ...)
2018-10-02 21:47 ` [PATCH 6/6] cpuidle: menu: Move the latency_req == 0 special case check Rafael J. Wysocki
@ 2018-10-04 6:55 ` Rafael J. Wysocki
2018-10-04 7:51 ` Peter Zijlstra
6 siblings, 1 reply; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-04 6:55 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM, Peter Zijlstra, Linux Kernel Mailing List, Daniel Lezcano
On Tue, Oct 2, 2018 at 11:51 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi All,
>
> This series fixes a couple of issues with the menu governor, optimizes it
> somewhat and makes a couple of cleanups in it. Please refer to the
> patch changelogs for details.
>
> All of the changes in the series are straightforward in my view. The
> first two patches are fixes, the rest is optimizations and cleanups.
I'm inclined to take this stuff in for 4.20 if nobody has problems
with it, so please have a look if you care (and you should, because
the code in question is run on all tickless systems out there).
Thanks,
Rafael
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-04 6:55 ` [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups Rafael J. Wysocki
@ 2018-10-04 7:51 ` Peter Zijlstra
0 siblings, 0 replies; 28+ messages in thread
From: Peter Zijlstra @ 2018-10-04 7:51 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Rafael J. Wysocki, Linux PM, Linux Kernel Mailing List, Daniel Lezcano
On Thu, Oct 04, 2018 at 08:55:45AM +0200, Rafael J. Wysocki wrote:
> On Tue, Oct 2, 2018 at 11:51 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi All,
> >
> > This series fixes a couple of issues with the menu governor, optimizes it
> > somewhat and makes a couple of cleanups in it. Please refer to the
> > patch changelogs for details.
> >
> > All of the changes in the series are straightforward in my view. The
> > first two patches are fixes, the rest is optimizations and cleanups.
>
> I'm inclined to take this stuff in for 4.20 if nobody has problems
> with it, so please have a look if you care (and you should, because
> the code in question is run on all tickless systems out there).
Looks ok to me,
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-08 5:53 Doug Smythies
@ 2018-10-08 7:51 ` Rafael J. Wysocki
2018-10-08 22:14 ` Doug Smythies
1 sibling, 0 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-08 7:51 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Peter Zijlstra, Linux Kernel Mailing List,
Daniel Lezcano, Linux PM
Hi Doug,
On Mon, Oct 8, 2018 at 8:02 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2018.10.03 23:56 Rafael J. Wysocki wrote:
> > On Tue, Oct 2, 2018 at 11:51 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >>
> >> Hi All,
> >>
> >> This series fixes a couple of issues with the menu governor, optimizes it
> >> somewhat and makes a couple of cleanups in it. Please refer to the
> >> patch changelogs for details.
> >>
> >> All of the changes in the series are straightforward in my view. The
> >> first two patches are fixes, the rest is optimizations and cleanups.
> >
> > I'm inclined to take this stuff in for 4.20 if nobody has problems
> > with it, so please have a look if you care (and you should, because
> > the code in question is run on all tickless systems out there).
>
> Hi Rafael,
>
> I did tests with kernel 4.19-rc6 as a baseline reference and then
> with 8 of your patches (&8patches in the graphs legend):
>
> cpuidle: menu: Replace data->predicted_us with local variable
> . as required to get this set of 6 to then apply.
> This set of 6 patches.
> cpuidle: poll_state: Revise loop termination condition
>
> Recall I also did some testing in late August [1], with
> a kernel that was just a few hundred commits before 4.19-rc1.
> The baseline is now way different. While I don't know why,
> I bisected the kernel and either made a mistake, or it was:
>
> first bad commit: [06e386a1db54ab6a671e103e929b590f7a88f0e3]
> Merge tag 'fbdev-v4.19' of https://github.com/bzolnier/linux
>
> Anyway, and for reference, included on some of the graphs
> is the old data from late August (legend name "4.18+3rjw
> (Aug test)")
>
> Test 1: A Thomas Ilsche type "powernightmare" test:
> (forever ((10 times - variable usec sleep) 0.999 seconds sleep) X 40 staggered threads.
> Where the "variable" was from 0.05 to 5 in steps of 0.05, for the first ~200 minutes of the test.
> (note: overheads mean that actual loop times are quite different.)
> And then from 5 to 50 in steps of 1, for the remaining 100 minutes of the test.
> (Shortened by 900 minutes from the way the test was done in August.)
> Each step ran for 2 minutes. The system was idle for 1 minute at the start, and a few
> minutes at the end of the graphs.
>
> The power and idle statistics graphs are here:
> http://fast.smythies.com/linux-pm/k419/k419-pn-sweep-rjw.htm
>
> Observations:
>
> While the graphs are pretty and such, the only significant
> difference is the idle state 0 percentages go down a lot
> with the 8 patches. However the number of idle state 0
> entries per minute goes up. To present the same information
> in a different way a trace was done (at 9 Gigabytes in
> 2 minutes):
The difference in the idle state 0 usage is a consequence of the "poll
idle" patch and is expected.
> &8patches
> Idle State 0: Total Entries: 10091412 : time (seconds): 49.447025
> Idle State 1: Total Entries: 49332297 : time (seconds): 375.943064
> Idle State 2: Total Entries: 311810 : time (seconds): 2.626403
>
> k4.19-rc6
> Idle State 0: Total Entries: 9162465 : time (seconds): 70.650566
> Idle State 1: Total Entries: 47592671 : time (seconds): 373.625083
> Idle State 2: Total Entries: 266212 : time (seconds): 2.278159
>
> Conclusions: Behaves as expected.
Right. :-)
> Test 2: pipe test 2 CPUs, one core. CPU test:
>
> The average loop times graph is here:
> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.png
>
> The power and idle statistics graphs are here:
> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.htm
>
> Conclusions:
>
> Better performance at the cost of more power with
> the patch set, but late August had both better performance
> and less power.
>
> Overall idle entries and exits are about the same, but way
> way more idle state 0 entries and exits with the patch set.
Same as above (and expected too).
> Supporting: trace summary (note: such a heavy load on the trace
> system (~6 gigabytes in 2 minutes) costs about 25% in performance):
>
> k4.16-rc6 pipe
> Idle State 0: Total Entries: 76638 : time (seconds): 0.193166
> Idle State 1: Total Entries: 37825999 : time (seconds): 23.886772
> Idle State 2: Total Entries: 49 : time (seconds): 0.007908
>
> &8patches
> Idle State 0: Total Entries: 37632104 : time (seconds): 26.097220
> Idle State 1: Total Entries: 397 : time (seconds): 0.020021
> Idle State 2: Total Entries: 208 : time (seconds): 0.031052
>
> With rjw 8 patch set (1st col is usecs duration, 2nd col
> is number of occurrences in 2 minutes):
>
> Idle State: 0 Summary:
> 0 24401500
> 1 13153259
> 2 19807
> 3 32731
> 4 802
> 5 346
> 6 1554
> 7 20087
> 8 1849
> 9 150
> 10 9
> 11 10
>
> Idle State: 1 Summary:
> 0 29
> 1 44
> 2 15
> 3 45
> 4 5
> 5 26
> 6 2
> 7 24
> 8 4
> 9 21
> 10 6
> 11 39
> 12 15
> 13 38
> 14 14
> 15 27
> 16 10
> 17 12
> 18 1
> 35 1
> 89 1
> 135 1
> 678 1
> 991 2
> 995 3
> 996 1
> 997 8
> 998 1
> 999 1
>
> Kernel 4.19-rc6 reference:
>
> Idle State: 0 Summary:
> 0 17212
> 1 7516
> 2 34737
> 3 14763
> 4 2312
> 5 74
> 6 3
> 7 3
> 8 3
> 9 4
> 10 5
> 11 5
> 40 1
>
> Idle State: 1 Summary:
> 0 36073601
> 1 1662728
> 2 67985
> 3 106
> 4 22
> 5 8
> 6 2214
> 7 11037
> 8 7110
> 9 1156
> 10 1
> 11 1
> 13 2
> 23 1
> 29 1
> 99 1
> 554 1
> 620 1
> 846 1
> 870 1
> 936 1
> 944 1
> 963 1
> 972 1
> 989 1
> 991 1
> 993 1
> 994 1
> 995 2
> 996 2
> 997 6
> 998 3
>
> Test 3: iperf test:
>
> Method: Be an iperf client to 3 servers at once.
> Packets are small on purpose, we want the highest
> frequency of packets, not fastest payload delivery.
>
> Performance:
>
> Kernel 4.19: 79.9 + 23.5 + 32.8 = 136.2 Mbits/Sec.
> &8patches: 78.6 + 23.2 + 33.0 = 134.8 Mbits/Sec.
>
> Kernel 4.19 average processor package power: 12.73 watts.
> &8patches average processor package power: 12.99 watts.
>
> The power and idle statistics graphs are here:
> http://fast.smythies.com/linux-pm/k419/k419-iperf.htm
>
> Conclusion:
>
> Marginally less performance and marginally more power
> used with the 8 patch set.
>
> Test 4: long idle test
>
> Just under 8 hours of at idle.
> (no pretty graphs)
>
> Averages (per minute):
>
> Kernel 4.19:
> % time in idle state 0: 1.76811E-05
> % time in idle state 1: 0.001501241
> % time in idle state 2: 0.002349672
> % time in idle state 3: 0.000432757
> % time in idle state 4: 100.0047484
> Idle state 0 entries: 2.470715835
> Idle state 1 entries: 27.84164859
> Idle state 2 entries: 26.02169197
> Idle state 3 entries: 4.600867679
> Idle state 4 entries: 1487.260304
> Processor package power: 3.668
>
> &8patches:
> % time in idle state 0: 4.76854E-06
> % time in idle state 1: 0.000752083
> % time in idle state 2: 0.001242119
> % time in idle state 3: 0.000408944
> % time in idle state 4: 100.0065453
> Idle state 0 entries: 4.213483146
> Idle state 1 entries: 16.42696629
> Idle state 2 entries: 16.75730337
> Idle state 3 entries: 4.541573034
> Idle state 4 entries: 1464.083146
> Processor package power: 3.667
>
> Conclusion: O.K.
>
> Test 5: intel-cpufreq schedutil specific test:
>
> Recall previously there were some significant
> improvements with this governor and the idle changes
> earlier this year.
> (no pretty graphs)
>
> Conclusion: No detectable differences.
>
> (sorry for the lack of detail here.)
>
> [1] https://marc.info/?l=linux-pm&m=153531591826719&w=2
Thanks a lot for the data and analysis, much appreciated!
Cheers,
Rafael
^ permalink raw reply [flat|nested] 28+ messages in thread
* RE: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-08 5:53 Doug Smythies
2018-10-08 7:51 ` Rafael J. Wysocki
@ 2018-10-08 22:14 ` Doug Smythies
2018-10-08 22:26 ` Rafael J. Wysocki
1 sibling, 1 reply; 28+ messages in thread
From: Doug Smythies @ 2018-10-08 22:14 UTC (permalink / raw)
To: 'Rafael J. Wysocki'
Cc: 'Rafael J. Wysocki', 'Peter Zijlstra',
'Linux Kernel Mailing List', 'Daniel Lezcano',
'Linux PM',
Doug Smythies
On 2018.10.08 00:51 Rafael J. Wysocki wrote:
> On Mon, Oct 8, 2018 at 8:02 AM Doug Smythies <dsmythies@telus.net> wrote:
>>
>> On 2018.10.03 23:56 Rafael J. Wysocki wrote:
>>> On Tue, Oct 2, 2018 at 11:51 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>>>>
>>>> Hi All,
>>>>
>>>> This series fixes a couple of issues with the menu governor, optimizes it
>>>> somewhat and makes a couple of cleanups in it. Please refer to the
>>>> patch changelogs for details.
>>>>
>>>> All of the changes in the series are straightforward in my view. The
>>>> first two patches are fixes, the rest is optimizations and cleanups.
>>>
>>> I'm inclined to take this stuff in for 4.20 if nobody has problems
>>> with it, so please have a look if you care (and you should, because
>>> the code in question is run on all tickless systems out there).
>>
>> Hi Rafael,
>>
>> I did tests with kernel 4.19-rc6 as a baseline reference and then
>> with 8 of your patches (&8patches in the graphs legend):
>>
>> cpuidle: menu: Replace data->predicted_us with local variable
>> . as required to get this set of 6 to then apply.
>> This set of 6 patches.
>> cpuidle: poll_state: Revise loop termination condition
>>
>> Recall I also did some testing in late August [1], with
>> a kernel that was just a few hundred commits before 4.19-rc1.
>> The baseline is now way different. While I don't know why,
>> I bisected the kernel and either made a mistake, or it was:
>>
>> first bad commit: [06e386a1db54ab6a671e103e929b590f7a88f0e3]
>> Merge tag 'fbdev-v4.19' of https://github.com/bzolnier/linux
>>
>> Anyway, and for reference, included on some of the graphs
>> is the old data from late August (legend name "4.18+3rjw
>> (Aug test)")
>>
>> Test 1: A Thomas Ilsche type "powernightmare" test:
>> (forever ((10 times - variable usec sleep) 0.999 seconds sleep) X 40 staggered threads.
>> Where the "variable" was from 0.05 to 5 in steps of 0.05, for the first ~200 minutes of the test.
>> (note: overheads mean that actual loop times are quite different.)
>> And then from 5 to 50 in steps of 1, for the remaining 100 minutes of the test.
>> (Shortened by 900 minutes from the way the test was done in August.)
>> Each step ran for 2 minutes. The system was idle for 1 minute at the start, and a few
>> minutes at the end of the graphs.
>>
>> The power and idle statistics graphs are here:
>> http://fast.smythies.com/linux-pm/k419/k419-pn-sweep-rjw.htm
>>
>> Observations:
>>
>> While the graphs are pretty and such, the only significant
>> difference is the idle state 0 percentages go down a lot
>> with the 8 patches. However the number of idle state 0
>> entries per minute goes up. To present the same information
>> in a different way a trace was done (at 9 Gigabytes in
>> 2 minutes):
>
> The difference in the idle state 0 usage is a consequence of the "poll
> idle" patch and is expected.
>
>> &8patches
>> Idle State 0: Total Entries: 10091412 : time (seconds): 49.447025
>> Idle State 1: Total Entries: 49332297 : time (seconds): 375.943064
>> Idle State 2: Total Entries: 311810 : time (seconds): 2.626403
>>
>> k4.19-rc6
>> Idle State 0: Total Entries: 9162465 : time (seconds): 70.650566
>> Idle State 1: Total Entries: 47592671 : time (seconds): 373.625083
>> Idle State 2: Total Entries: 266212 : time (seconds): 2.278159
>>
>> Conclusions: Behaves as expected.
>
> Right. :-)
>> Test 2: pipe test 2 CPUs, one core. CPU test:
>>
>> The average loop times graph is here:
>> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.png
>>
>> The power and idle statistics graphs are here:
>> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.htm
>>
>> Conclusions:
>>
>> Better performance at the cost of more power with
>> the patch set, but late August had both better performance
>> and less power.
>>
>> Overall idle entries and exits are about the same, but way
>> way more idle state 0 entries and exits with the patch set.
>
>Same as above (and expected too).
I Disagree. The significant transfer of idle entries from
idle state 1 with kernel 4.19-rc6 to idle state 0 with the
additional 8 patch set is virtually entirely due to this patch:
"[PATCH 2/6] cpuidle: menu: Compute first_idx when latency_req is known"
As far as I can determine from all of this data, in particular the
histogram data below, it seems to me that it now is selecting
idle state 0 whereas before it was selecting idle state 1
is the correct decision for those very short duration idle states
(well, for my processor (older i7-2600K) at least).
Note: I did test my above assertion with kernels compiled with only
the first 2 and then 3 of the 8 patch set.
>
>> Supporting: trace summary (note: such a heavy load on the trace
>> system (~6 gigabytes in 2 minutes) costs about 25% in performance):
>>
>> k4.16-rc6 pipe
>> Idle State 0: Total Entries: 76638 : time (seconds): 0.193166
>> Idle State 1: Total Entries: 37825999 : time (seconds): 23.886772
>> Idle State 2: Total Entries: 49 : time (seconds): 0.007908
>>
>> &8patches
>> Idle State 0: Total Entries: 37632104 : time (seconds): 26.097220
>> Idle State 1: Total Entries: 397 : time (seconds): 0.020021
>> Idle State 2: Total Entries: 208 : time (seconds): 0.031052
>>
>> With rjw 8 patch set (1st col is usecs duration, 2nd col
>> is number of occurrences in 2 minutes):
>>
>> Idle State: 0 Summary:
>> 0 24401500
>> 1 13153259
>> 2 19807
>> 3 32731
>> 4 802
>> 5 346
>> 6 1554
>> 7 20087
>> 8 1849
>> 9 150
>> 10 9
>> 11 10
>>
>> Idle State: 1 Summary:
>> 0 29
>> 1 44
>> 2 15
>> 3 45
>> 4 5
>> 5 26
>> 6 2
>> 7 24
...[snip]...
>>
>> Kernel 4.19-rc6 reference:
>>
>> Idle State: 0 Summary:
>> 0 17212
>> 1 7516
>> 2 34737
>> 3 14763
>> 4 2312
>> 5 74
>> 6 3
>> 7 3
>> 8 3
>> 9 4
>> 10 5
>> 11 5
>> 40 1
>>
>> Idle State: 1 Summary:
>> 0 36073601
>> 1 1662728
>> 2 67985
>> 3 106
>> 4 22
>> 5 8
>> 6 2214
>> 7 11037
>> 8 7110
...[snip]...
... Doug
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-08 22:14 ` Doug Smythies
@ 2018-10-08 22:26 ` Rafael J. Wysocki
2018-10-09 10:42 ` Rafael J. Wysocki
2018-10-10 0:02 ` Doug Smythies
0 siblings, 2 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-08 22:26 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Rafael J. Wysocki, Peter Zijlstra,
Linux Kernel Mailing List, Daniel Lezcano, Linux PM
On Tue, Oct 9, 2018 at 12:14 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2018.10.08 00:51 Rafael J. Wysocki wrote:
> > On Mon, Oct 8, 2018 at 8:02 AM Doug Smythies <dsmythies@telus.net> wrote:
> >>
> >> On 2018.10.03 23:56 Rafael J. Wysocki wrote:
> >>> On Tue, Oct 2, 2018 at 11:51 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
[cut]
> >> Test 2: pipe test 2 CPUs, one core. CPU test:
> >>
> >> The average loop times graph is here:
> >> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.png
> >>
> >> The power and idle statistics graphs are here:
> >> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.htm
> >>
> >> Conclusions:
> >>
> >> Better performance at the cost of more power with
> >> the patch set, but late August had both better performance
> >> and less power.
> >>
> >> Overall idle entries and exits are about the same, but way
> >> way more idle state 0 entries and exits with the patch set.
> >
> >Same as above (and expected too).
>
> I Disagree. The significant transfer of idle entries from
> idle state 1 with kernel 4.19-rc6 to idle state 0 with the
> additional 8 patch set is virtually entirely due to this patch:
>
> "[PATCH 2/6] cpuidle: menu: Compute first_idx when latency_req is known"
OK
> As far as I can determine from all of this data, in particular the
> histogram data below, it seems to me that it now is selecting
> idle state 0 whereas before it was selecting idle state 1
> is the correct decision for those very short duration idle states
> (well, for my processor (older i7-2600K) at least).
At least, that's a matter of consistency IMO.
State 1 should not be selected if the final latency limit is below its
exit latency and that's what happens in that situation.
> Note: I did test my above assertion with kernels compiled with only
> the first 2 and then 3 of the 8 patch set.
I see.
Thanks,
Rafael
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-08 22:26 ` Rafael J. Wysocki
@ 2018-10-09 10:42 ` Rafael J. Wysocki
2018-10-10 0:02 ` Doug Smythies
1 sibling, 0 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-09 10:42 UTC (permalink / raw)
To: Doug Smythies
Cc: Peter Zijlstra, Linux Kernel Mailing List, Daniel Lezcano, Linux PM
On Tuesday, October 9, 2018 12:26:48 AM CEST Rafael J. Wysocki wrote:
> On Tue, Oct 9, 2018 at 12:14 AM Doug Smythies <dsmythies@telus.net> wrote:
> >
> > On 2018.10.08 00:51 Rafael J. Wysocki wrote:
> > > On Mon, Oct 8, 2018 at 8:02 AM Doug Smythies <dsmythies@telus.net> wrote:
> > >>
> > >> On 2018.10.03 23:56 Rafael J. Wysocki wrote:
> > >>> On Tue, Oct 2, 2018 at 11:51 PM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> [cut]
>
> > >> Test 2: pipe test 2 CPUs, one core. CPU test:
> > >>
> > >> The average loop times graph is here:
> > >> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.png
> > >>
> > >> The power and idle statistics graphs are here:
> > >> http://fast.smythies.com/linux-pm/k419/k419-rjw-pipe-1core.htm
> > >>
> > >> Conclusions:
> > >>
> > >> Better performance at the cost of more power with
> > >> the patch set, but late August had both better performance
> > >> and less power.
> > >>
> > >> Overall idle entries and exits are about the same, but way
> > >> way more idle state 0 entries and exits with the patch set.
> > >
> > >Same as above (and expected too).
> >
> > I Disagree. The significant transfer of idle entries from
> > idle state 1 with kernel 4.19-rc6 to idle state 0 with the
> > additional 8 patch set is virtually entirely due to this patch:
> >
> > "[PATCH 2/6] cpuidle: menu: Compute first_idx when latency_req is known"
>
> OK
>
> > As far as I can determine from all of this data, in particular the
> > histogram data below, it seems to me that it now is selecting
> > idle state 0 whereas before it was selecting idle state 1
> > is the correct decision for those very short duration idle states
> > (well, for my processor (older i7-2600K) at least).
>
> At least, that's a matter of consistency IMO.
>
> State 1 should not be selected if the final latency limit is below its
> exit latency and that's what happens in that situation.
>
> > Note: I did test my above assertion with kernels compiled with only
> > the first 2 and then 3 of the 8 patch set.
>
> I see.
While at it, could you test the appended patch (on top of the previous 8)
for me please?
I think that this code can be simplified now.
---
drivers/cpuidle/governors/menu.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -371,12 +371,12 @@ static int menu_select(struct cpuidle_dr
if (s->target_residency > predicted_us) {
/*
* Use a physical idle state, not busy polling, unless
- * a timer is going to trigger really really soon.
+ * a timer is going to trigger soon enough.
*/
if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
- i == idx + 1 && latency_req > s->exit_latency &&
- data->next_timer_us > max_t(unsigned int, 20,
- s->target_residency)) {
+ s->exit_latency <= latency_req &&
+ s->target_residency <= data->next_timer_us) {
+ predicted_us = s->target_residency;
idx = i;
break;
}
^ permalink raw reply [flat|nested] 28+ messages in thread
* RE: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-08 22:26 ` Rafael J. Wysocki
2018-10-09 10:42 ` Rafael J. Wysocki
@ 2018-10-10 0:02 ` Doug Smythies
2018-10-10 7:14 ` Rafael J. Wysocki
1 sibling, 1 reply; 28+ messages in thread
From: Doug Smythies @ 2018-10-10 0:02 UTC (permalink / raw)
To: 'Rafael J. Wysocki'
Cc: 'Peter Zijlstra', 'Linux Kernel Mailing List',
'Daniel Lezcano', 'Linux PM',
Doug Smythies
On 2018.10.09 03:43 Rafael J. Wysocki wrote:
...[snip]...
> While at it, could you test the appended patch
> (on top of the previous 8) for me please?
>
> I think that this code can be simplified now.
>
> ---
> drivers/cpuidle/governors/menu.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -371,12 +371,12 @@ static int menu_select(struct cpuidle_dr
> if (s->target_residency > predicted_us) {
> /*
> * Use a physical idle state, not busy polling, unless
> - * a timer is going to trigger really really soon.
> + * a timer is going to trigger soon enough.
> */
> if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
> - i == idx + 1 && latency_req > s->exit_latency &&
> - data->next_timer_us > max_t(unsigned int, 20,
> - s->target_residency)) {
> + s->exit_latency <= latency_req &&
> + s->target_residency <= data->next_timer_us) {
> + predicted_us = s->target_residency;
> idx = i;
> break;
> }
It seems to work fine.
I was unable to detect any difference between the 8 patch set and with
this additional patch for any of the tests that I ran. (at least beyond
noise and/or experimental error.)
Note: I didn't publish any of the pretty graphs.
... Doug
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 0/6] cpuidle: menu: Fixes, optimizations and cleanups
2018-10-10 0:02 ` Doug Smythies
@ 2018-10-10 7:14 ` Rafael J. Wysocki
0 siblings, 0 replies; 28+ messages in thread
From: Rafael J. Wysocki @ 2018-10-10 7:14 UTC (permalink / raw)
To: Doug Smythies
Cc: Rafael J. Wysocki, Peter Zijlstra, Linux Kernel Mailing List,
Daniel Lezcano, Linux PM
On Wed, Oct 10, 2018 at 2:02 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On 2018.10.09 03:43 Rafael J. Wysocki wrote:
>
> ...[snip]...
>
> > While at it, could you test the appended patch
> > (on top of the previous 8) for me please?
> >
> > I think that this code can be simplified now.
> >
> > ---
> > drivers/cpuidle/governors/menu.c | 8 ++++----
> > 1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > Index: linux-pm/drivers/cpuidle/governors/menu.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> > +++ linux-pm/drivers/cpuidle/governors/menu.c
> > @@ -371,12 +371,12 @@ static int menu_select(struct cpuidle_dr
> > if (s->target_residency > predicted_us) {
> > /*
> > * Use a physical idle state, not busy polling, unless
> > - * a timer is going to trigger really really soon.
> > + * a timer is going to trigger soon enough.
> > */
> > if ((drv->states[idx].flags & CPUIDLE_FLAG_POLLING) &&
> > - i == idx + 1 && latency_req > s->exit_latency &&
> > - data->next_timer_us > max_t(unsigned int, 20,
> > - s->target_residency)) {
> > + s->exit_latency <= latency_req &&
> > + s->target_residency <= data->next_timer_us) {
> > + predicted_us = s->target_residency;
> > idx = i;
> > break;
> > }
>
> It seems to work fine.
> I was unable to detect any difference between the 8 patch set and with
> this additional patch for any of the tests that I ran. (at least beyond
> noise and/or experimental error.)
Great, thank you!
Cheers,
Rafael
^ permalink raw reply [flat|nested] 28+ messages in thread