[PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop

[PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop

[Rafael J. Wysocki]

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.

[1/4] Move the default idle call code into a separate function.
[2/4] Make cpuidle_reflect() check the sign of its index arg.
[3/4] Eliminate the "reflect" check from cpuidle_idle_call().
[4/4] Eliminate a redundant check of the cpuidle_enter() return value.

All on top of 4.1-rc2.

Thanks!


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[PATCH 1/4] sched / idle: Move the default idle call code to a separate function

[Rafael J. Wysocki]

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);

Re: [PATCH 1/4] sched / idle: Move the default idle call code to a separate function

[Daniel Lezcano]

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

Re: [PATCH 1/4] sched / idle: Move the default idle call code to a separate function

[Peter Zijlstra]

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();
> +}

Re: [PATCH 1/4] sched / idle: Move the default idle call code to a separate function

[Rafael J. Wysocki]

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

[PATCH 2/4] cpuidle: Check the sign of index in cpuidle_reflect()

[Rafael J. Wysocki]

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;
 }
 
 /**

Re: [PATCH 2/4] cpuidle: Check the sign of index in cpuidle_reflect()

[Daniel Lezcano]

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

[PATCH 3/4] sched / idle: Eliminate the "reflect" check from cpuidle_idle_call()

[Rafael J. Wysocki]

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();

Re: [PATCH 3/4] sched / idle: Eliminate the "reflect" check from cpuidle_idle_call()

[Daniel Lezcano]

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

[PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state()

[Rafael J. Wysocki]

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;
 }
 

Re: [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state()

[Daniel Lezcano]

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

Re: [PATCH 4/4] sched / idle: Call default_idle_call() from cpuidle_enter_state()

[Rafael J. Wysocki]

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

Re: [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop

[Peter Zijlstra]

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>

Re: [PATCH 0/4] sched / idle: Reduce the number of branches in the idle loop

[Rafael J. Wysocki]

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!