* [PATCH 1/4] sched / idle: Move the default idle call code to a separate function
2015-05-04 13:54 [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Rafael J. Wysocki
@ 2015-05-04 13:56 ` Rafael J. Wysocki
2015-05-04 14:00 ` Daniel Lezcano
2015-05-04 14:22 ` Peter Zijlstra
2015-05-04 13:57 ` [PATCH 2/4] cpuidle: Check the sign of index in cpuidle_reflect() Rafael J. Wysocki
` (3 subsequent siblings)
4 siblings, 2 replies; 14+ messages in thread
From: Rafael J. Wysocki @ 2015-05-04 13:56 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Move the code under the "use_default" label in cpuidle_idle_call()
into a separate (new) function.
This just allows the subsequent changes to be more stratightforward.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
kernel/sched/idle.c | 42 +++++++++++++++++++++++-------------------
1 file changed, 23 insertions(+), 19 deletions(-)
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -67,6 +67,17 @@ void __weak arch_cpu_idle(void)
local_irq_enable();
}
+static void default_idle_call(void) {
+ /*
+ * We can't use the cpuidle framework, let's use the default idle
+ * routine.
+ */
+ if (current_clr_polling_and_test())
+ local_irq_enable();
+ else
+ arch_cpu_idle();
+}
+
/**
* cpuidle_idle_call - the main idle function
*
@@ -105,8 +116,10 @@ static void cpuidle_idle_call(void)
*/
rcu_idle_enter();
- if (cpuidle_not_available(drv, dev))
- goto use_default;
+ if (cpuidle_not_available(drv, dev)) {
+ default_idle_call();
+ goto exit_idle;
+ }
/*
* Suspend-to-idle ("freeze") is a system state in which all user space
@@ -134,8 +147,10 @@ static void cpuidle_idle_call(void)
next_state = cpuidle_select(drv, dev);
}
/* Fall back to the default arch idle method on errors. */
- if (next_state < 0)
- goto use_default;
+ if (next_state < 0) {
+ default_idle_call();
+ goto exit_idle;
+ }
/*
* The idle task must be scheduled, it is pointless to
@@ -162,8 +177,10 @@ static void cpuidle_idle_call(void)
/* The cpu is no longer idle or about to enter idle. */
idle_set_state(this_rq(), NULL);
- if (entered_state == -EBUSY)
- goto use_default;
+ if (entered_state == -EBUSY) {
+ default_idle_call();
+ goto exit_idle;
+ }
/*
* Give the governor an opportunity to reflect on the outcome
@@ -182,19 +199,6 @@ exit_idle:
rcu_idle_exit();
start_critical_timings();
- return;
-
-use_default:
- /*
- * We can't use the cpuidle framework, let's use the default
- * idle routine.
- */
- if (current_clr_polling_and_test())
- local_irq_enable();
- else
- arch_cpu_idle();
-
- goto exit_idle;
}
DEFINE_PER_CPU(bool, cpu_dead_idle);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] sched / idle: Move the default idle call code to a separate function
2015-05-04 13:56 ` [PATCH 1/4] sched / idle: Move the default idle call code to a separate function Rafael J. Wysocki
@ 2015-05-04 14:00 ` Daniel Lezcano
2015-05-04 14:22 ` Peter Zijlstra
1 sibling, 0 replies; 14+ messages in thread
From: Daniel Lezcano @ 2015-05-04 14:00 UTC (permalink / raw)
To: Rafael J. Wysocki, Peter Zijlstra
Cc: Linux PM list, Linux Kernel Mailing List
On 05/04/2015 03:56 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Move the code under the "use_default" label in cpuidle_idle_call()
> into a separate (new) function.
>
> This just allows the subsequent changes to be more stratightforward.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
--
<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] 14+ messages in thread
* Re: [PATCH 1/4] sched / idle: Move the default idle call code to a separate function
2015-05-04 13:56 ` [PATCH 1/4] sched / idle: Move the default idle call code to a separate function Rafael J. Wysocki
2015-05-04 14:00 ` Daniel Lezcano
@ 2015-05-04 14:22 ` Peter Zijlstra
2015-05-04 21:08 ` Rafael J. Wysocki
1 sibling, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2015-05-04 14:22 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
On Mon, May 04, 2015 at 03:56:24PM +0200, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Move the code under the "use_default" label in cpuidle_idle_call()
> into a separate (new) function.
>
> This just allows the subsequent changes to be more stratightforward.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> kernel/sched/idle.c | 42 +++++++++++++++++++++++-------------------
> 1 file changed, 23 insertions(+), 19 deletions(-)
>
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -67,6 +67,17 @@ void __weak arch_cpu_idle(void)
> local_irq_enable();
> }
>
> +static void default_idle_call(void) {
Please put opening brace of function on a new line.
> + /*
> + * We can't use the cpuidle framework, let's use the default idle
> + * routine.
> + */
> + if (current_clr_polling_and_test())
> + local_irq_enable();
> + else
> + arch_cpu_idle();
> +}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 1/4] sched / idle: Move the default idle call code to a separate function
2015-05-04 14:22 ` Peter Zijlstra
@ 2015-05-04 21:08 ` Rafael J. Wysocki
0 siblings, 0 replies; 14+ messages in thread
From: Rafael J. Wysocki @ 2015-05-04 21:08 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
On Monday, May 04, 2015 04:22:45 PM Peter Zijlstra wrote:
> On Mon, May 04, 2015 at 03:56:24PM +0200, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Move the code under the "use_default" label in cpuidle_idle_call()
> > into a separate (new) function.
> >
> > This just allows the subsequent changes to be more stratightforward.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > ---
> > kernel/sched/idle.c | 42 +++++++++++++++++++++++-------------------
> > 1 file changed, 23 insertions(+), 19 deletions(-)
> >
> > Index: linux-pm/kernel/sched/idle.c
> > ===================================================================
> > --- linux-pm.orig/kernel/sched/idle.c
> > +++ linux-pm/kernel/sched/idle.c
> > @@ -67,6 +67,17 @@ void __weak arch_cpu_idle(void)
> > local_irq_enable();
> > }
> >
> > +static void default_idle_call(void) {
>
> Please put opening brace of function on a new line.
Ah that's weird. I wonder why I haven't noticed this myself.
Rafael
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH 2/4] cpuidle: Check the sign of index in cpuidle_reflect()
2015-05-04 13:54 [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Rafael J. Wysocki
2015-05-04 13:56 ` [PATCH 1/4] sched / idle: Move the default idle call code to a separate function Rafael J. Wysocki
@ 2015-05-04 13:57 ` Rafael J. Wysocki
2015-05-04 14:02 ` Daniel Lezcano
2015-05-04 13:57 ` [PATCH 3/4] sched / idle: Eliminate the "reflect" check from cpuidle_idle_call() Rafael J. Wysocki
` (2 subsequent siblings)
4 siblings, 1 reply; 14+ messages in thread
From: Rafael J. Wysocki @ 2015-05-04 13:57 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Avoid calling the governor's ->reflect method if the state index
passed to cpuidle_reflect() is negative.
This allows the analogous check to be dropped from menu_reflect(),
so do that too, and ensures that arbitrary error codes can be
passed to cpuidle_reflect() as the index with no adverse
consequences.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/cpuidle.c | 2 +-
drivers/cpuidle/governors/menu.c | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -249,7 +249,7 @@ int cpuidle_enter(struct cpuidle_driver
*/
void cpuidle_reflect(struct cpuidle_device *dev, int index)
{
- if (cpuidle_curr_governor->reflect)
+ if (cpuidle_curr_governor->reflect && index >= 0)
cpuidle_curr_governor->reflect(dev, index);
}
Index: linux-pm/drivers/cpuidle/governors/menu.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/menu.c
+++ linux-pm/drivers/cpuidle/governors/menu.c
@@ -367,9 +367,9 @@ static int menu_select(struct cpuidle_dr
static void menu_reflect(struct cpuidle_device *dev, int index)
{
struct menu_device *data = this_cpu_ptr(&menu_devices);
+
data->last_state_idx = index;
- if (index >= 0)
- data->needs_update = 1;
+ data->needs_update = 1;
}
/**
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/4] cpuidle: Check the sign of index in cpuidle_reflect()
2015-05-04 13:57 ` [PATCH 2/4] cpuidle: Check the sign of index in cpuidle_reflect() Rafael J. Wysocki
@ 2015-05-04 14:02 ` Daniel Lezcano
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Lezcano @ 2015-05-04 14:02 UTC (permalink / raw)
To: Rafael J. Wysocki, Peter Zijlstra
Cc: Linux PM list, Linux Kernel Mailing List
On 05/04/2015 03:57 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Avoid calling the governor's ->reflect method if the state index
> passed to cpuidle_reflect() is negative.
>
> This allows the analogous check to be dropped from menu_reflect(),
> so do that too, and ensures that arbitrary error codes can be
> passed to cpuidle_reflect() as the index with no adverse
> consequences.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> drivers/cpuidle/cpuidle.c | 2 +-
> drivers/cpuidle/governors/menu.c | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -249,7 +249,7 @@ int cpuidle_enter(struct cpuidle_driver
> */
> void cpuidle_reflect(struct cpuidle_device *dev, int index)
> {
> - if (cpuidle_curr_governor->reflect)
> + if (cpuidle_curr_governor->reflect && index >= 0)
> cpuidle_curr_governor->reflect(dev, index);
> }
>
> Index: linux-pm/drivers/cpuidle/governors/menu.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/governors/menu.c
> +++ linux-pm/drivers/cpuidle/governors/menu.c
> @@ -367,9 +367,9 @@ static int menu_select(struct cpuidle_dr
> static void menu_reflect(struct cpuidle_device *dev, int index)
> {
> struct menu_device *data = this_cpu_ptr(&menu_devices);
> +
> data->last_state_idx = index;
> - if (index >= 0)
> - data->needs_update = 1;
> + data->needs_update = 1;
> }
>
> /**
>
--
<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] 14+ messages in thread
* [PATCH 3/4] sched / idle: Eliminate the "reflect" check from cpuidle_idle_call()
2015-05-04 13:54 [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Rafael J. Wysocki
2015-05-04 13:56 ` [PATCH 1/4] sched / idle: Move the default idle call code to a separate function Rafael J. Wysocki
2015-05-04 13:57 ` [PATCH 2/4] cpuidle: Check the sign of index in cpuidle_reflect() Rafael J. Wysocki
@ 2015-05-04 13:57 ` Rafael J. Wysocki
2015-05-04 14:49 ` Daniel Lezcano
2015-05-04 13:58 ` [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state() Rafael J. Wysocki
2015-05-04 14:25 ` [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Peter Zijlstra
4 siblings, 1 reply; 14+ messages in thread
From: Rafael J. Wysocki @ 2015-05-04 13:57 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Since cpuidle_reflect() should only be called if the idle state
to enter was selected by cpuidle_select(), there is the "reflect"
variable in cpuidle_idle_call() whose value is used to determine
whether or not that is the case.
However, if the entire code run between the conditional setting
"reflect" and the call to cpuidle_reflect() is moved to a separate
function, it will be possible to call that new function in both
branches of the conditional, in which case cpuidle_reflect() will
only need to be called from one of them too and the "reflect"
variable won't be necessary any more.
This eliminates one check made by cpuidle_idle_call() on the majority
of its invocations, so change the code as described.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
kernel/sched/idle.c | 90 ++++++++++++++++++++++++++--------------------------
1 file changed, 46 insertions(+), 44 deletions(-)
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -78,6 +78,46 @@ static void default_idle_call(void) {
arch_cpu_idle();
}
+static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
+ int next_state)
+{
+ int entered_state;
+
+ /* Fall back to the default arch idle method on errors. */
+ if (next_state < 0) {
+ default_idle_call();
+ return next_state;
+ }
+
+ /*
+ * The idle task must be scheduled, it is pointless to go to idle, just
+ * update no idle residency and return.
+ */
+ if (current_clr_polling_and_test()) {
+ dev->last_residency = 0;
+ local_irq_enable();
+ return -EBUSY;
+ }
+
+ /* Take note of the planned idle state. */
+ idle_set_state(this_rq(), &drv->states[next_state]);
+
+ /*
+ * Enter the idle state previously returned by the governor decision.
+ * This function will block until an interrupt occurs and will take
+ * care of re-enabling the local interrupts
+ */
+ entered_state = cpuidle_enter(drv, dev, next_state);
+
+ /* The cpu is no longer idle or about to enter idle. */
+ idle_set_state(this_rq(), NULL);
+
+ if (entered_state == -EBUSY)
+ default_idle_call();
+
+ return entered_state;
+}
+
/**
* cpuidle_idle_call - the main idle function
*
@@ -92,7 +132,6 @@ static void cpuidle_idle_call(void)
struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
int next_state, entered_state;
- bool reflect;
/*
* Check if the idle task must be rescheduled. If it is the
@@ -137,56 +176,19 @@ static void cpuidle_idle_call(void)
goto exit_idle;
}
- reflect = false;
next_state = cpuidle_find_deepest_state(drv, dev);
+ call_cpuidle(drv, dev, next_state);
} else {
- reflect = true;
/*
* Ask the cpuidle framework to choose a convenient idle state.
*/
next_state = cpuidle_select(drv, dev);
- }
- /* Fall back to the default arch idle method on errors. */
- if (next_state < 0) {
- default_idle_call();
- goto exit_idle;
- }
-
- /*
- * The idle task must be scheduled, it is pointless to
- * go to idle, just update no idle residency and get
- * out of this function
- */
- if (current_clr_polling_and_test()) {
- dev->last_residency = 0;
- entered_state = next_state;
- local_irq_enable();
- goto exit_idle;
- }
-
- /* Take note of the planned idle state. */
- idle_set_state(this_rq(), &drv->states[next_state]);
-
- /*
- * Enter the idle state previously returned by the governor decision.
- * This function will block until an interrupt occurs and will take
- * care of re-enabling the local interrupts
- */
- entered_state = cpuidle_enter(drv, dev, next_state);
-
- /* The cpu is no longer idle or about to enter idle. */
- idle_set_state(this_rq(), NULL);
-
- if (entered_state == -EBUSY) {
- default_idle_call();
- goto exit_idle;
- }
-
- /*
- * Give the governor an opportunity to reflect on the outcome
- */
- if (reflect)
+ entered_state = call_cpuidle(drv, dev, next_state);
+ /*
+ * Give the governor an opportunity to reflect on the outcome
+ */
cpuidle_reflect(dev, entered_state);
+ }
exit_idle:
__current_set_polling();
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 3/4] sched / idle: Eliminate the "reflect" check from cpuidle_idle_call()
2015-05-04 13:57 ` [PATCH 3/4] sched / idle: Eliminate the "reflect" check from cpuidle_idle_call() Rafael J. Wysocki
@ 2015-05-04 14:49 ` Daniel Lezcano
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Lezcano @ 2015-05-04 14:49 UTC (permalink / raw)
To: Rafael J. Wysocki, Peter Zijlstra
Cc: Linux PM list, Linux Kernel Mailing List
On 05/04/2015 03:57 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Since cpuidle_reflect() should only be called if the idle state
> to enter was selected by cpuidle_select(), there is the "reflect"
> variable in cpuidle_idle_call() whose value is used to determine
> whether or not that is the case.
>
> However, if the entire code run between the conditional setting
> "reflect" and the call to cpuidle_reflect() is moved to a separate
> function, it will be possible to call that new function in both
> branches of the conditional, in which case cpuidle_reflect() will
> only need to be called from one of them too and the "reflect"
> variable won't be necessary any more.
>
> This eliminates one check made by cpuidle_idle_call() on the majority
> of its invocations, so change the code as described.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Reviewed-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
> kernel/sched/idle.c | 90 ++++++++++++++++++++++++++--------------------------
> 1 file changed, 46 insertions(+), 44 deletions(-)
>
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -78,6 +78,46 @@ static void default_idle_call(void) {
> arch_cpu_idle();
> }
>
> +static int call_cpuidle(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> + int next_state)
> +{
> + int entered_state;
> +
> + /* Fall back to the default arch idle method on errors. */
> + if (next_state < 0) {
> + default_idle_call();
> + return next_state;
> + }
> +
> + /*
> + * The idle task must be scheduled, it is pointless to go to idle, just
> + * update no idle residency and return.
> + */
> + if (current_clr_polling_and_test()) {
> + dev->last_residency = 0;
> + local_irq_enable();
> + return -EBUSY;
> + }
> +
> + /* Take note of the planned idle state. */
> + idle_set_state(this_rq(), &drv->states[next_state]);
> +
> + /*
> + * Enter the idle state previously returned by the governor decision.
> + * This function will block until an interrupt occurs and will take
> + * care of re-enabling the local interrupts
> + */
> + entered_state = cpuidle_enter(drv, dev, next_state);
> +
> + /* The cpu is no longer idle or about to enter idle. */
> + idle_set_state(this_rq(), NULL);
> +
> + if (entered_state == -EBUSY)
> + default_idle_call();
> +
> + return entered_state;
> +}
> +
> /**
> * cpuidle_idle_call - the main idle function
> *
> @@ -92,7 +132,6 @@ static void cpuidle_idle_call(void)
> struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
> struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
> int next_state, entered_state;
> - bool reflect;
>
> /*
> * Check if the idle task must be rescheduled. If it is the
> @@ -137,56 +176,19 @@ static void cpuidle_idle_call(void)
> goto exit_idle;
> }
>
> - reflect = false;
> next_state = cpuidle_find_deepest_state(drv, dev);
> + call_cpuidle(drv, dev, next_state);
> } else {
> - reflect = true;
> /*
> * Ask the cpuidle framework to choose a convenient idle state.
> */
> next_state = cpuidle_select(drv, dev);
> - }
> - /* Fall back to the default arch idle method on errors. */
> - if (next_state < 0) {
> - default_idle_call();
> - goto exit_idle;
> - }
> -
> - /*
> - * The idle task must be scheduled, it is pointless to
> - * go to idle, just update no idle residency and get
> - * out of this function
> - */
> - if (current_clr_polling_and_test()) {
> - dev->last_residency = 0;
> - entered_state = next_state;
> - local_irq_enable();
> - goto exit_idle;
> - }
> -
> - /* Take note of the planned idle state. */
> - idle_set_state(this_rq(), &drv->states[next_state]);
> -
> - /*
> - * Enter the idle state previously returned by the governor decision.
> - * This function will block until an interrupt occurs and will take
> - * care of re-enabling the local interrupts
> - */
> - entered_state = cpuidle_enter(drv, dev, next_state);
> -
> - /* The cpu is no longer idle or about to enter idle. */
> - idle_set_state(this_rq(), NULL);
> -
> - if (entered_state == -EBUSY) {
> - default_idle_call();
> - goto exit_idle;
> - }
> -
> - /*
> - * Give the governor an opportunity to reflect on the outcome
> - */
> - if (reflect)
> + entered_state = call_cpuidle(drv, dev, next_state);
> + /*
> + * Give the governor an opportunity to reflect on the outcome
> + */
> cpuidle_reflect(dev, entered_state);
> + }
>
> exit_idle:
> __current_set_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] 14+ messages in thread
* [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state()
2015-05-04 13:54 [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Rafael J. Wysocki
` (2 preceding siblings ...)
2015-05-04 13:57 ` [PATCH 3/4] sched / idle: Eliminate the "reflect" check from cpuidle_idle_call() Rafael J. Wysocki
@ 2015-05-04 13:58 ` Rafael J. Wysocki
2015-05-04 15:04 ` Daniel Lezcano
2015-05-04 14:25 ` [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Peter Zijlstra
4 siblings, 1 reply; 14+ messages in thread
From: Rafael J. Wysocki @ 2015-05-04 13:58 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
The check of the cpuidle_enter() return value against -EBUSY
made in call_cpuidle() will not be necessary any more if
cpuidle_enter_state() calls default_idle_call() directly when it
is about to return -EBUSY, so make that happen and eliminate the
check.
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
drivers/cpuidle/cpuidle.c | 4 +++-
drivers/cpuidle/cpuidle.h | 2 ++
kernel/sched/idle.c | 14 ++++++--------
3 files changed, 11 insertions(+), 9 deletions(-)
Index: linux-pm/drivers/cpuidle/cpuidle.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.c
+++ linux-pm/drivers/cpuidle/cpuidle.c
@@ -167,8 +167,10 @@ int cpuidle_enter_state(struct cpuidle_d
* local timer will be shut down. If a local timer is used from another
* CPU as a broadcast timer, this call may fail if it is not available.
*/
- if (broadcast && tick_broadcast_enter())
+ if (broadcast && tick_broadcast_enter()) {
+ default_idle_call();
return -EBUSY;
+ }
trace_cpu_idle_rcuidle(index, dev->cpu);
time_start = ktime_get();
Index: linux-pm/drivers/cpuidle/cpuidle.h
===================================================================
--- linux-pm.orig/drivers/cpuidle/cpuidle.h
+++ linux-pm/drivers/cpuidle/cpuidle.h
@@ -18,6 +18,8 @@ extern int cpuidle_enter_state(struct cp
/* idle loop */
extern void cpuidle_install_idle_handler(void);
extern void cpuidle_uninstall_idle_handler(void);
+/* kernel/sched/idle.c */
+extern void default_idle_call(void);
/* governors */
extern int cpuidle_switch_governor(struct cpuidle_governor *gov);
Index: linux-pm/kernel/sched/idle.c
===================================================================
--- linux-pm.orig/kernel/sched/idle.c
+++ linux-pm/kernel/sched/idle.c
@@ -67,11 +67,12 @@ void __weak arch_cpu_idle(void)
local_irq_enable();
}
-static void default_idle_call(void) {
- /*
- * We can't use the cpuidle framework, let's use the default idle
- * routine.
- */
+/**
+ * default_idle_call - Default CPU idle routine.
+ *
+ * To use when the cpuidle framework cannot be used.
+ */
+void default_idle_call(void) {
if (current_clr_polling_and_test())
local_irq_enable();
else
@@ -112,9 +113,6 @@ static int call_cpuidle(struct cpuidle_d
/* The cpu is no longer idle or about to enter idle. */
idle_set_state(this_rq(), NULL);
- if (entered_state == -EBUSY)
- default_idle_call();
-
return entered_state;
}
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state()
2015-05-04 13:58 ` [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state() Rafael J. Wysocki
@ 2015-05-04 15:04 ` Daniel Lezcano
2015-05-04 21:11 ` Rafael J. Wysocki
0 siblings, 1 reply; 14+ messages in thread
From: Daniel Lezcano @ 2015-05-04 15:04 UTC (permalink / raw)
To: Rafael J. Wysocki, Peter Zijlstra
Cc: Linux PM list, Linux Kernel Mailing List
On 05/04/2015 03:58 PM, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> The check of the cpuidle_enter() return value against -EBUSY
> made in call_cpuidle() will not be necessary any more if
> cpuidle_enter_state() calls default_idle_call() directly when it
> is about to return -EBUSY, so make that happen and eliminate the
> check.
>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> ---
> drivers/cpuidle/cpuidle.c | 4 +++-
> drivers/cpuidle/cpuidle.h | 2 ++
> kernel/sched/idle.c | 14 ++++++--------
> 3 files changed, 11 insertions(+), 9 deletions(-)
>
> Index: linux-pm/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> +++ linux-pm/drivers/cpuidle/cpuidle.c
> @@ -167,8 +167,10 @@ int cpuidle_enter_state(struct cpuidle_d
> * local timer will be shut down. If a local timer is used from another
> * CPU as a broadcast timer, this call may fail if it is not available.
> */
> - if (broadcast && tick_broadcast_enter())
> + if (broadcast && tick_broadcast_enter()) {
> + default_idle_call();
> return -EBUSY;
> + }
>
> trace_cpu_idle_rcuidle(index, dev->cpu);
> time_start = ktime_get();
> Index: linux-pm/drivers/cpuidle/cpuidle.h
> ===================================================================
> --- linux-pm.orig/drivers/cpuidle/cpuidle.h
> +++ linux-pm/drivers/cpuidle/cpuidle.h
> @@ -18,6 +18,8 @@ extern int cpuidle_enter_state(struct cp
> /* idle loop */
> extern void cpuidle_install_idle_handler(void);
> extern void cpuidle_uninstall_idle_handler(void);
> +/* kernel/sched/idle.c */
> +extern void default_idle_call(void);
There is a cyclic dependency introduced with this function.
idle.c <=> cpuidle.c
Are we sure we want them to be mutually dependent ?
> /* governors */
> extern int cpuidle_switch_governor(struct cpuidle_governor *gov);
> Index: linux-pm/kernel/sched/idle.c
> ===================================================================
> --- linux-pm.orig/kernel/sched/idle.c
> +++ linux-pm/kernel/sched/idle.c
> @@ -67,11 +67,12 @@ void __weak arch_cpu_idle(void)
> local_irq_enable();
> }
>
> -static void default_idle_call(void) {
> - /*
> - * We can't use the cpuidle framework, let's use the default idle
> - * routine.
> - */
> +/**
> + * default_idle_call - Default CPU idle routine.
> + *
> + * To use when the cpuidle framework cannot be used.
> + */
> +void default_idle_call(void) {
Same comment as Peter on the patch 1/4.
> if (current_clr_polling_and_test())
> local_irq_enable();
> else
> @@ -112,9 +113,6 @@ static int call_cpuidle(struct cpuidle_d
> /* The cpu is no longer idle or about to enter idle. */
> idle_set_state(this_rq(), NULL);
>
> - if (entered_state == -EBUSY)
> - default_idle_call();
> -
> return entered_state;
> }
>
>
--
<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] 14+ messages in thread
* Re: [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state()
2015-05-04 15:04 ` Daniel Lezcano
@ 2015-05-04 21:11 ` Rafael J. Wysocki
0 siblings, 0 replies; 14+ messages in thread
From: Rafael J. Wysocki @ 2015-05-04 21:11 UTC (permalink / raw)
To: Daniel Lezcano; +Cc: Peter Zijlstra, Linux PM list, Linux Kernel Mailing List
On Monday, May 04, 2015 05:04:08 PM Daniel Lezcano wrote:
> On 05/04/2015 03:58 PM, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > The check of the cpuidle_enter() return value against -EBUSY
> > made in call_cpuidle() will not be necessary any more if
> > cpuidle_enter_state() calls default_idle_call() directly when it
> > is about to return -EBUSY, so make that happen and eliminate the
> > check.
> >
> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > ---
> > drivers/cpuidle/cpuidle.c | 4 +++-
> > drivers/cpuidle/cpuidle.h | 2 ++
> > kernel/sched/idle.c | 14 ++++++--------
> > 3 files changed, 11 insertions(+), 9 deletions(-)
> >
> > Index: linux-pm/drivers/cpuidle/cpuidle.c
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/cpuidle.c
> > +++ linux-pm/drivers/cpuidle/cpuidle.c
> > @@ -167,8 +167,10 @@ int cpuidle_enter_state(struct cpuidle_d
> > * local timer will be shut down. If a local timer is used from another
> > * CPU as a broadcast timer, this call may fail if it is not available.
> > */
> > - if (broadcast && tick_broadcast_enter())
> > + if (broadcast && tick_broadcast_enter()) {
> > + default_idle_call();
> > return -EBUSY;
> > + }
> >
> > trace_cpu_idle_rcuidle(index, dev->cpu);
> > time_start = ktime_get();
> > Index: linux-pm/drivers/cpuidle/cpuidle.h
> > ===================================================================
> > --- linux-pm.orig/drivers/cpuidle/cpuidle.h
> > +++ linux-pm/drivers/cpuidle/cpuidle.h
> > @@ -18,6 +18,8 @@ extern int cpuidle_enter_state(struct cp
> > /* idle loop */
> > extern void cpuidle_install_idle_handler(void);
> > extern void cpuidle_uninstall_idle_handler(void);
> > +/* kernel/sched/idle.c */
> > +extern void default_idle_call(void);
>
> There is a cyclic dependency introduced with this function.
>
> idle.c <=> cpuidle.c
>
> Are we sure we want them to be mutually dependent ?
Well, hadn't I think so, I wouldn't have posted the patch in the first place. :-)
Aesthetics is one thing and wasted cycles is another. A redundant check
in the idle loop means a whole lot of wasted cycles throughout the life time
of a kernel.
Rafael
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop
2015-05-04 13:54 [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Rafael J. Wysocki
` (3 preceding siblings ...)
2015-05-04 13:58 ` [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state() Rafael J. Wysocki
@ 2015-05-04 14:25 ` Peter Zijlstra
2015-05-04 21:23 ` Rafael J. Wysocki
4 siblings, 1 reply; 14+ messages in thread
From: Peter Zijlstra @ 2015-05-04 14:25 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
On Mon, May 04, 2015 at 03:54:43PM +0200, Rafael J. Wysocki wrote:
> Hi,
>
> The "reflect" variable that had to be added to cpuidle_idle_call() to fix a
> regression during the 4.0 cycle has bothered me a bit since then and guess
> what? It is not necessary.
>
> After the last regression fix related to tick_broadcast_exit() I realized
> that it should be possible to eliminate this variable by splitting
> cpuidle_idle_call() into smaller routines and reordering the code in
> question which is done by this patch series.
>
> It also gets rid of one more redundant check while at it.
Ooh nice! Yes that thing bothered me too.
Once you fix that one weird opening bracket:
Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop
2015-05-04 14:25 ` [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop Peter Zijlstra
@ 2015-05-04 21:23 ` Rafael J. Wysocki
0 siblings, 0 replies; 14+ messages in thread
From: Rafael J. Wysocki @ 2015-05-04 21:23 UTC (permalink / raw)
To: Peter Zijlstra; +Cc: Linux PM list, Linux Kernel Mailing List, Daniel Lezcano
On Monday, May 04, 2015 04:25:05 PM Peter Zijlstra wrote:
> On Mon, May 04, 2015 at 03:54:43PM +0200, Rafael J. Wysocki wrote:
> > Hi,
> >
> > The "reflect" variable that had to be added to cpuidle_idle_call() to fix a
> > regression during the 4.0 cycle has bothered me a bit since then and guess
> > what? It is not necessary.
> >
> > After the last regression fix related to tick_broadcast_exit() I realized
> > that it should be possible to eliminate this variable by splitting
> > cpuidle_idle_call() into smaller routines and reordering the code in
> > question which is done by this patch series.
> >
> > It also gets rid of one more redundant check while at it.
>
>
> Ooh nice! Yes that thing bothered me too.
>
> Once you fix that one weird opening bracket:
>
> Acked-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Fixed and queued up as 4.2 material, thanks!
^ permalink raw reply [flat|nested] 14+ messages in thread