[PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Daniel Lezcano]

In order to allow better integration between the cpuidle framework and the
scheduler, reducing the distance between these two sub-components will
facilitate this integration by moving part of the cpuidle code in the idle
task file and, because idle.c is in the sched directory, we have access to
the scheduler's private structures.

This patch splits the cpuidle_idle_call main entry function into 3 calls
to a newly added API:
 1. select the idle state
 2. enter the idle state
 3. reflect the idle state

The cpuidle_idle_call calls these three functions to implement the main
idle entry function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Nicolas Pitre <nico@linaro.org>
---

ChangeLog:

V2: 
    * splitted cpuidle_select error check into 'cpuidle_enabled' function

---
 drivers/cpuidle/cpuidle.c |  114 ++++++++++++++++++++++++++++++++++------------
 include/linux/cpuidle.h   |   19 +++++++
 2 files changed, 105 insertions(+), 28 deletions(-)

Index: cpuidle-next/drivers/cpuidle/cpuidle.c
===================================================================
--- cpuidle-next.orig/drivers/cpuidle/cpuidle.c
+++ cpuidle-next/drivers/cpuidle/cpuidle.c
@@ -65,6 +65,26 @@ int cpuidle_play_dead(void)
 }
 
 /**
+ * cpuidle_enabled - check if the cpuidle framework is ready
+ * @dev: cpuidle device for this cpu
+ * @drv: cpuidle driver for this cpu
+ *
+ * Return 0 on success, otherwise:
+ * -NODEV : the cpuidle framework is not available
+ * -EBUSY : the cpuidle framework is not initialized
+ */
+int cpuidle_enabled(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+{
+	if (off || !initialized)
+		return -ENODEV;
+
+	if (!drv || !dev || !dev->enabled)
+		return -EBUSY;
+
+	return 0;
+}
+
+/**
  * cpuidle_enter_state - enter the state and update stats
  * @dev: cpuidle device for this cpu
  * @drv: cpuidle driver for this cpu
@@ -108,6 +128,63 @@ int cpuidle_enter_state(struct cpuidle_d
 }
 
 /**
+ * cpuidle_select - ask the cpuidle framework to choose an idle state
+ *
+ * @drv: the cpuidle driver
+ * @dev: the cpuidle device
+ *
+ * Returns the index of the idle state.
+ */
+int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+{
+	return cpuidle_curr_governor->select(drv, dev);
+}
+
+/**
+ * cpuidle_enter - enter into the specified idle state
+ *
+ * @drv:   the cpuidle driver tied with the cpu
+ * @dev:   the cpuidle device
+ * @index: the index in the idle state table
+ *
+ * Returns the index in the idle state, < 0 in case of error.
+ * The error code depends on the backend driver
+ */
+int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
+		  int index)
+{
+	int entered_state;
+	bool broadcast = !!(drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP);
+
+	if (broadcast)
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
+
+	if (cpuidle_state_is_coupled(dev, drv, index))
+		entered_state = cpuidle_enter_state_coupled(dev, drv, index);
+	else
+		entered_state = cpuidle_enter_state(dev, drv, index);
+
+	if (broadcast)
+		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+
+	return entered_state;
+}
+
+/**
+ * cpuidle_reflect - tell the underlying governor what was the state
+ * we were in
+ *
+ * @dev  : the cpuidle device
+ * @index: the index in the idle state table
+ *
+ */
+void cpuidle_reflect(struct cpuidle_device *dev, int index)
+{
+	if (cpuidle_curr_governor->reflect)
+		cpuidle_curr_governor->reflect(dev, index);
+}
+
+/**
  * cpuidle_idle_call - the main idle loop
  *
  * NOTE: no locks or semaphores should be used here
@@ -116,51 +193,32 @@ int cpuidle_enter_state(struct cpuidle_d
 int cpuidle_idle_call(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
-	struct cpuidle_driver *drv;
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
-	bool broadcast;
 
-	if (off || !initialized)
-		return -ENODEV;
-
-	/* check if the device is ready */
-	if (!dev || !dev->enabled)
-		return -EBUSY;
-
-	drv = cpuidle_get_cpu_driver(dev);
+	next_state = cpuidle_enabled(drv, dev);
+	if (next_state < 0)
+		return next_state;
 
 	/* ask the governor for the next state */
-	next_state = cpuidle_curr_governor->select(drv, dev);
+	next_state = cpuidle_select(drv, dev);
+
 	if (need_resched()) {
 		dev->last_residency = 0;
 		/* give the governor an opportunity to reflect on the outcome */
-		if (cpuidle_curr_governor->reflect)
-			cpuidle_curr_governor->reflect(dev, next_state);
+		cpuidle_reflect(dev, next_state);
 		local_irq_enable();
 		return 0;
 	}
 
 	trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
-	broadcast = !!(drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP);
-
-	if (broadcast)
-		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
-
-	if (cpuidle_state_is_coupled(dev, drv, next_state))
-		entered_state = cpuidle_enter_state_coupled(dev, drv,
-							    next_state);
-	else
-		entered_state = cpuidle_enter_state(dev, drv, next_state);
-
-	if (broadcast)
-		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+	entered_state = cpuidle_enter(drv, dev, next_state);
 
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
 	/* give the governor an opportunity to reflect on the outcome */
-	if (cpuidle_curr_governor->reflect)
-		cpuidle_curr_governor->reflect(dev, entered_state);
+	cpuidle_reflect(dev, entered_state);
 
 	return 0;
 }
Index: cpuidle-next/include/linux/cpuidle.h
===================================================================
--- cpuidle-next.orig/include/linux/cpuidle.h
+++ cpuidle-next/include/linux/cpuidle.h
@@ -119,6 +119,15 @@ struct cpuidle_driver {
 
 #ifdef CONFIG_CPU_IDLE
 extern void disable_cpuidle(void);
+
+extern int cpuidle_enabled(struct cpuidle_driver *drv,
+			  struct cpuidle_device *dev);
+extern int cpuidle_select(struct cpuidle_driver *drv,
+			  struct cpuidle_device *dev);
+extern int cpuidle_enter(struct cpuidle_driver *drv,
+			 struct cpuidle_device *dev, int index);
+extern void cpuidle_reflect(struct cpuidle_device *dev, int index);
+
 extern int cpuidle_idle_call(void);
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
 extern struct cpuidle_driver *cpuidle_get_driver(void);
@@ -141,6 +150,16 @@ extern int cpuidle_play_dead(void);
 extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
 #else
 static inline void disable_cpuidle(void) { }
+static inline int cpuidle_enabled(struct cpuidle_driver *drv,
+				  struct cpuidle_device *dev)
+{return -ENODEV; }
+static inline int cpuidle_select(struct cpuidle_driver *drv,
+				 struct cpuidle_device *dev)
+{return -ENODEV; }
+static inline int cpuidle_enter(struct cpuidle_driver *drv,
+				struct cpuidle_device *dev, int index)
+{return -ENODEV; }
+static inline void cpuidle_reflect(struct cpuidle_device *dev, int index) { }
 static inline int cpuidle_idle_call(void) { return -ENODEV; }
 static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
 {return -ENODEV; }

[PATCH V2 2/5] cpuidle/idle: Move the cpuidle_idle_call function to idle.c

[Daniel Lezcano]

The cpuidle_idle_call does nothing more than calling the three individuals
function and is no longer used by any arch specific code but only in the
cpuidle framework code. 

We can move this function into the idle task code to ensure better
proximity to the scheduler code.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
---
 drivers/cpuidle/cpuidle.c |   39 ---------------------------------------
 include/linux/cpuidle.h   |    2 --
 kernel/sched/idle.c       |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 46 insertions(+), 41 deletions(-)

Index: cpuidle-next/drivers/cpuidle/cpuidle.c
===================================================================
--- cpuidle-next.orig/drivers/cpuidle/cpuidle.c
+++ cpuidle-next/drivers/cpuidle/cpuidle.c
@@ -185,45 +185,6 @@ void cpuidle_reflect(struct cpuidle_devi
 }
 
 /**
- * cpuidle_idle_call - the main idle loop
- *
- * NOTE: no locks or semaphores should be used here
- * return non-zero on failure
- */
-int 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;
-
-	next_state = cpuidle_enabled(drv, dev);
-	if (next_state < 0)
-		return next_state;
-
-	/* ask the governor for the next state */
-	next_state = cpuidle_select(drv, dev);
-
-	if (need_resched()) {
-		dev->last_residency = 0;
-		/* give the governor an opportunity to reflect on the outcome */
-		cpuidle_reflect(dev, next_state);
-		local_irq_enable();
-		return 0;
-	}
-
-	trace_cpu_idle_rcuidle(next_state, dev->cpu);
-
-	entered_state = cpuidle_enter(drv, dev, next_state);
-
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
-
-	/* give the governor an opportunity to reflect on the outcome */
-	cpuidle_reflect(dev, entered_state);
-
-	return 0;
-}
-
-/**
  * cpuidle_install_idle_handler - installs the cpuidle idle loop handler
  */
 void cpuidle_install_idle_handler(void)
Index: cpuidle-next/include/linux/cpuidle.h
===================================================================
--- cpuidle-next.orig/include/linux/cpuidle.h
+++ cpuidle-next/include/linux/cpuidle.h
@@ -128,7 +128,6 @@ extern int cpuidle_enter(struct cpuidle_
 			 struct cpuidle_device *dev, int index);
 extern void cpuidle_reflect(struct cpuidle_device *dev, int index);
 
-extern int cpuidle_idle_call(void);
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
 extern struct cpuidle_driver *cpuidle_get_driver(void);
 extern struct cpuidle_driver *cpuidle_driver_ref(void);
@@ -160,7 +159,6 @@ static inline int cpuidle_enter(struct c
 				struct cpuidle_device *dev, int index)
 {return -ENODEV; }
 static inline void cpuidle_reflect(struct cpuidle_device *dev, int index) { }
-static inline int cpuidle_idle_call(void) { return -ENODEV; }
 static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
 {return -ENODEV; }
 static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
Index: cpuidle-next/kernel/sched/idle.c
===================================================================
--- cpuidle-next.orig/kernel/sched/idle.c
+++ cpuidle-next/kernel/sched/idle.c
@@ -63,6 +63,52 @@ void __weak arch_cpu_idle(void)
 	local_irq_enable();
 }
 
+#ifdef CONFIG_CPU_IDLE
+/**
+ * cpuidle_idle_call - the main idle function
+ *
+ * NOTE: no locks or semaphores should be used here
+ * return non-zero on failure
+ */
+static int 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;
+
+	next_state = cpuidle_enabled(drv, dev);
+	if (next_state < 0)
+		return next_state;
+
+	/* ask the governor for the next state */
+	next_state = cpuidle_select(drv, dev);
+
+	if (need_resched()) {
+		dev->last_residency = 0;
+		/* give the governor an opportunity to reflect on the outcome */
+		cpuidle_reflect(dev, next_state);
+		local_irq_enable();
+		return 0;
+	}
+
+	trace_cpu_idle_rcuidle(next_state, dev->cpu);
+
+	entered_state = cpuidle_enter(drv, dev, next_state);
+
+	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+
+	/* give the governor an opportunity to reflect on the outcome */
+	cpuidle_reflect(dev, entered_state);
+
+	return 0;
+}
+#else
+static inline int cpuidle_idle_call(void)
+{
+	return -ENODEV;
+}
+#endif
+
 /*
  * Generic idle loop implementation
  */

[PATCH V2 3/5] idle: Reorganize the idle loop

[Daniel Lezcano]

Now that we have the main cpuidle function in idle.c, move some code from
the idle mainloop to this function for the sake of clarity.

That removes if then else indentation difficult to follow when looking at the
code. This patch does not change the current behavior.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Nicolas Pitre <nico@linaro.org>
---

Changelog:

V2:
   * fixed type in patch's description

---
 include/linux/cpuidle.h |    2 ++
 kernel/sched/idle.c     |   40 +++++++++++++++++++++-------------------
 2 files changed, 23 insertions(+), 19 deletions(-)

Index: cpuidle-next/include/linux/cpuidle.h
===================================================================
--- cpuidle-next.orig/include/linux/cpuidle.h
+++ cpuidle-next/include/linux/cpuidle.h
@@ -180,6 +180,8 @@ static inline int cpuidle_enable_device(
 {return -ENODEV; }
 static inline void cpuidle_disable_device(struct cpuidle_device *dev) { }
 static inline int cpuidle_play_dead(void) {return -ENODEV; }
+static inline struct cpuidle_driver *cpuidle_get_cpu_driver(
+	struct cpuidle_device *dev) {return NULL; }
 #endif
 
 #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
Index: cpuidle-next/kernel/sched/idle.c
===================================================================
--- cpuidle-next.orig/kernel/sched/idle.c
+++ cpuidle-next/kernel/sched/idle.c
@@ -63,7 +63,6 @@ void __weak arch_cpu_idle(void)
 	local_irq_enable();
 }
 
-#ifdef CONFIG_CPU_IDLE
 /**
  * cpuidle_idle_call - the main idle function
  *
@@ -76,11 +75,21 @@ static int cpuidle_idle_call(void)
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
 
+	stop_critical_timings();
+	rcu_idle_enter();
+
 	next_state = cpuidle_enabled(drv, dev);
-	if (next_state < 0)
-		return next_state;
+	if (next_state < 0) {
+		arch_cpu_idle();
+		goto out;
+	}
 
-	/* ask the governor for the next state */
+	/*
+	 * Ask the governor for the next state, this call can fail for
+	 * different reasons: cpuidle is not enabled or an idle state
+	 * fulfilling the constraints was not found. In this case, we
+	 * fall back to the default idle function
+	 */
 	next_state = cpuidle_select(drv, dev);
 
 	if (need_resched()) {
@@ -88,7 +97,7 @@ static int cpuidle_idle_call(void)
 		/* give the governor an opportunity to reflect on the outcome */
 		cpuidle_reflect(dev, next_state);
 		local_irq_enable();
-		return 0;
+		goto out;
 	}
 
 	trace_cpu_idle_rcuidle(next_state, dev->cpu);
@@ -99,15 +108,15 @@ static int cpuidle_idle_call(void)
 
 	/* give the governor an opportunity to reflect on the outcome */
 	cpuidle_reflect(dev, entered_state);
+out:
+	if (WARN_ON_ONCE(irqs_disabled()))
+		local_irq_enable();
+
+	rcu_idle_exit();
+	start_critical_timings();
 
 	return 0;
 }
-#else
-static inline int cpuidle_idle_call(void)
-{
-	return -ENODEV;
-}
-#endif
 
 /*
  * Generic idle loop implementation
@@ -140,14 +149,7 @@ static void cpu_idle_loop(void)
 				cpu_idle_poll();
 			} else {
 				if (!current_clr_polling_and_test()) {
-					stop_critical_timings();
-					rcu_idle_enter();
-					if (cpuidle_idle_call())
-						arch_cpu_idle();
-					if (WARN_ON_ONCE(irqs_disabled()))
-						local_irq_enable();
-					rcu_idle_exit();
-					start_critical_timings();
+					cpuidle_idle_call();
 				} else {
 					local_irq_enable();
 				}

[PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Daniel Lezcano]

This patch moves the condition before entering idle into the cpuidle main
function located in idle.c. That simplify the idle mainloop functions and
increase the readibility of the conditions to enter truly idle.

This patch is code reorganization and does not change the behavior of the
function.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 kernel/sched/idle.c |   40 +++++++++++++++++++++-------------------
 1 file changed, 21 insertions(+), 19 deletions(-)

Index: cpuidle-next/kernel/sched/idle.c
===================================================================
--- cpuidle-next.orig/kernel/sched/idle.c
+++ cpuidle-next/kernel/sched/idle.c
@@ -75,6 +75,23 @@ static int cpuidle_idle_call(void)
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
 
+	/*
+	 * In poll mode we reenable interrupts and spin.
+	 *
+	 * Also if we detected in the wakeup from idle path that the
+	 * tick broadcast device expired for us, we don't want to go
+	 * deep idle as we know that the IPI is going to arrive right
+	 * away
+	 */
+	if (cpu_idle_force_poll || tick_check_broadcast_expired())
+		return cpu_idle_poll();
+
+	if (current_clr_polling_and_test()) {
+		local_irq_enable();
+		__current_set_polling();
+		return 0;
+	}
+
 	stop_critical_timings();
 	rcu_idle_enter();
 
@@ -115,6 +132,8 @@ out:
 	rcu_idle_exit();
 	start_critical_timings();
 
+	__current_set_polling();
+
 	return 0;
 }
 
@@ -136,25 +155,8 @@ static void cpu_idle_loop(void)
 			local_irq_disable();
 			arch_cpu_idle_enter();
 
-			/*
-			 * In poll mode we reenable interrupts and spin.
-			 *
-			 * Also if we detected in the wakeup from idle
-			 * path that the tick broadcast device expired
-			 * for us, we don't want to go deep idle as we
-			 * know that the IPI is going to arrive right
-			 * away
-			 */
-			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
-				cpu_idle_poll();
-			} else {
-				if (!current_clr_polling_and_test()) {
-					cpuidle_idle_call();
-				} else {
-					local_irq_enable();
-				}
-				__current_set_polling();
-			}
+			cpuidle_idle_call();
+
 			arch_cpu_idle_exit();
 			/*
 			 * We need to test and propagate the TIF_NEED_RESCHED

[PATCH V2 5/5] idle: Add more comments to the code

[Daniel Lezcano]

The idle main function is a complex and a critical function. Added more
comments to the code.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Acked-by: Nicolas Pitre <nico@linaro.org>
---
 Changelog:

 V2:
     * fixed typo in comment
---
 kernel/sched/idle.c |   40 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 36 insertions(+), 4 deletions(-)

Index: cpuidle-next/kernel/sched/idle.c
===================================================================
--- cpuidle-next.orig/kernel/sched/idle.c
+++ cpuidle-next/kernel/sched/idle.c
@@ -86,15 +86,34 @@ static int cpuidle_idle_call(void)
 	if (cpu_idle_force_poll || tick_check_broadcast_expired())
 		return cpu_idle_poll();
 
+	/*
+	 * Check if the idle task must be rescheduled. If it is the case,
+	 * exit the function after re-enabling the local irq and set again
+	 * the polling flag
+	 */
 	if (current_clr_polling_and_test()) {
 		local_irq_enable();
 		__current_set_polling();
 		return 0;
 	}
 
+	/*
+	 * During the idle period, stop measuring the disabled irqs
+	 * critical sections latencies
+	 */
 	stop_critical_timings();
+
+	/*
+	 * Tell the RCU framework we are entering an idle section,
+	 * so no more rcu read side critical sections and one more
+	 * step to the grace period
+	 */
 	rcu_idle_enter();
 
+	/*
+	 * Check if the cpuidle framework is ready, otherwise fallback
+	 * to the default arch specific idle method
+	 */
 	next_state = cpuidle_enabled(drv, dev);
 	if (next_state < 0) {
 		arch_cpu_idle();
@@ -102,13 +121,16 @@ static int cpuidle_idle_call(void)
 	}
 
 	/*
-	 * Ask the governor for the next state, this call can fail for
-	 * different reasons: cpuidle is not enabled or an idle state
-	 * fulfilling the constraints was not found. In this case, we
-	 * fall back to the default idle function
+	 * Ask the governor to choose an idle state it thinks it is
+	 * convenient to go to. There is *always* a convenient idle
+	 * state
 	 */
 	next_state = cpuidle_select(drv, dev);
 
+	/*
+	 * 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 (need_resched()) {
 		dev->last_residency = 0;
 		/* give the governor an opportunity to reflect on the outcome */
@@ -119,6 +141,12 @@ static int cpuidle_idle_call(void)
 
 	trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
+	/*
+	 * 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);
 
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
@@ -155,6 +183,10 @@ static void cpu_idle_loop(void)
 			local_irq_disable();
 			arch_cpu_idle_enter();
 
+			/*
+			 * It is up to the underlying functions to
+			 * enable the local interrupts again
+			 */
 			cpuidle_idle_call();
 
 			arch_cpu_idle_exit();

Re: [PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Peter Zijlstra]

On Mon, Feb 24, 2014 at 02:55:50PM +0100, Daniel Lezcano wrote:
> @@ -136,25 +155,8 @@ static void cpu_idle_loop(void)
>  			local_irq_disable();
>  			arch_cpu_idle_enter();
>  
> -			/*
> -			 * In poll mode we reenable interrupts and spin.
> -			 *
> -			 * Also if we detected in the wakeup from idle
> -			 * path that the tick broadcast device expired
> -			 * for us, we don't want to go deep idle as we
> -			 * know that the IPI is going to arrive right
> -			 * away
> -			 */
> -			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
> -				cpu_idle_poll();
> -			} else {
> -				if (!current_clr_polling_and_test()) {
> -					cpuidle_idle_call();
> -				} else {
> -					local_irq_enable();
> -				}
> -				__current_set_polling();
> -			}
> +			cpuidle_idle_call();
> +

Yeah, not liking that much; you can make it look like:

	if (cpu_idle_force_poll || tick_check_broadcast_expired())
		cpu_idle_poll();
	else
		cpu_idle_call();

Though. That keeps the polling case separate from the actual idle
function.


And when you do that; you can also push down the
current_clr_polling_and_test() muck so it doesn't cover the actual
cpuidle policy code.

Re: [PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Peter Zijlstra]

None of this actually applies :/ I'm having major conflicts in
driveres/cpuidle/cpuidle.c.

Re: [PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Daniel Lezcano]

On 02/24/2014 04:00 PM, Peter Zijlstra wrote:
>
>
> None of this actually applies :/ I'm having major conflicts in
> driveres/cpuidle/cpuidle.c.

Ok, except if I am missing something, the patchset is based on top of 
tip/sched/core (commit 6990566).



-- 
  <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 V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Peter Zijlstra]

On Mon, Feb 24, 2014 at 04:12:07PM +0100, Daniel Lezcano wrote:
> On 02/24/2014 04:00 PM, Peter Zijlstra wrote:
> >
> >
> >None of this actually applies :/ I'm having major conflicts in
> >driveres/cpuidle/cpuidle.c.
> 
> Ok, except if I am missing something, the patchset is based on top of
> tip/sched/core (commit 6990566).

Ah! So the below commit from the timers/core branch is in the way.

Thomas, Ingo, how do we go about solving this sched/core vs timers/core
conflict?

---

# git log 6990566..tip/master drivers/cpuidle/
commit 2f60b8ae0b2a006e4d3452e57ea98b59ce985d14
Merge: 9eaf844efc23 849401b66d30
Author: Ingo Molnar <mingo@kernel.org>
Date:   Sat Feb 22 18:52:27 2014 +0100

    Merge branch 'timers/core'

commit ba8f20c2eb4158a443e9d6a909aee5010efa0c69
Author: Preeti U Murthy <preeti@linux.vnet.ibm.com>
Date:   Fri Feb 7 13:36:52 2014 +0530

    cpuidle: Handle clockevents_notify(BROADCAST_ENTER) failure
    
    Some archs set the CPUIDLE_FLAG_TIMER_STOP flag for idle states in which the
    local timers stop. The cpuidle_idle_call() currently handles such idle states
    by calling into the broadcast framework so as to wakeup CPUs at their next
    wakeup event. With the hrtimer mode of broadcast, the BROADCAST_ENTER call
    into the broadcast frameowork can fail for archs that do not have an external
    clock device to handle wakeups and the CPU in question has thus to be made
    the stand by CPU. This patch handles such cases by failing the call into
    cpuidle so that the arch can take some default action. The arch will certainly
    not enter a similar idle state because a failed cpuidle call will also implicitly
    indicate that the broadcast framework has not registered this CPU to be woken up.
    Hence we are safe if we fail the cpuidle call.
    
    In the process move the functions that trace idle statistics just before and
    after the entry and exit into idle states respectively. In other
    scenarios where the call to cpuidle fails, we end up not tracing idle
    entry and exit since a decision on an idle state could not be taken. Similarly
    when the call to broadcast framework fails, we skip tracing idle statistics
    because we are in no further position to take a decision on an alternative
    idle state to enter into.
    
    Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
    Cc: deepthi@linux.vnet.ibm.com
    Cc: paulmck@linux.vnet.ibm.com
    Cc: fweisbec@gmail.com
    Cc: paulus@samba.org
    Cc: srivatsa.bhat@linux.vnet.ibm.com
    Cc: svaidy@linux.vnet.ibm.com
    Cc: peterz@infradead.org
    Cc: benh@kernel.crashing.org
    Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
    Cc: linuxppc-dev@lists.ozlabs.org
    Link: http://lkml.kernel.org/r/20140207080652.17187.66344.stgit@preeti.in.ibm.com
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Re: [PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Daniel Lezcano]

On 02/24/2014 03:59 PM, Peter Zijlstra wrote:
> On Mon, Feb 24, 2014 at 02:55:50PM +0100, Daniel Lezcano wrote:
>> @@ -136,25 +155,8 @@ static void cpu_idle_loop(void)
>>   			local_irq_disable();
>>   			arch_cpu_idle_enter();
>>
>> -			/*
>> -			 * In poll mode we reenable interrupts and spin.
>> -			 *
>> -			 * Also if we detected in the wakeup from idle
>> -			 * path that the tick broadcast device expired
>> -			 * for us, we don't want to go deep idle as we
>> -			 * know that the IPI is going to arrive right
>> -			 * away
>> -			 */
>> -			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
>> -				cpu_idle_poll();
>> -			} else {
>> -				if (!current_clr_polling_and_test()) {
>> -					cpuidle_idle_call();
>> -				} else {
>> -					local_irq_enable();
>> -				}
>> -				__current_set_polling();
>> -			}
>> +			cpuidle_idle_call();
>> +
>
> Yeah, not liking that much; you can make it look like:
>
> 	if (cpu_idle_force_poll || tick_check_broadcast_expired())
> 		cpu_idle_poll();
> 	else
> 		cpu_idle_call();
>
> Though. That keeps the polling case separate from the actual idle
> function.

Yes, you are right, it looks better.

> And when you do that; you can also push down the
> current_clr_polling_and_test() muck so it doesn't cover the actual
> cpuidle policy code.

I am not getting it. Where do you suggest to move it ?

-- 
  <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 V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Peter Zijlstra]

On Mon, Feb 24, 2014 at 04:39:08PM +0100, Daniel Lezcano wrote:

> >And when you do that; you can also push down the
> >current_clr_polling_and_test() muck so it doesn't cover the actual
> >cpuidle policy code.
> 
> I am not getting it. Where do you suggest to move it ?

A little something like so I suppose; only call
current_clr_polling_and_test() right before calling the actual idle
function. Although I suppose cpuidle_enter() can still do all sorts of
weird.

--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -78,37 +78,35 @@ static int cpuidle_idle_call(void)
 	stop_critical_timings();
 	rcu_idle_enter();
 
-	next_state = cpuidle_enabled(drv, dev);
-	if (next_state < 0) {
-		arch_cpu_idle();
-		goto out;
-	}
-
-	/*
-	 * Ask the governor for the next state, this call can fail for
-	 * different reasons: cpuidle is not enabled or an idle state
-	 * fulfilling the constraints was not found. In this case, we
-	 * fall back to the default idle function
-	 */
-	next_state = cpuidle_select(drv, dev);
+	if (cpuidle_enabled(drv, dev) == 0) {
+		/*
+		 * Ask the governor for the next state, this call can fail for
+		 * different reasons: cpuidle is not enabled or an idle state
+		 * fulfilling the constraints was not found. In this case, we
+		 * fall back to the default idle function
+		 */
+		next_state = cpuidle_select(drv, dev);
+
+		if (current_clr_polling_and_test()) {
+			dev->last_residency = 0;
+			entered_state = next_state;
+			local_irq_enable();
+		} else {
+			trace_cpu_idle_rcuidle(next_state, dev->cpu);
+			entered_state = cpuidle_enter(drv, dev, next_state);
+			trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
+		}
 
-	if (need_resched()) {
-		dev->last_residency = 0;
 		/* give the governor an opportunity to reflect on the outcome */
-		cpuidle_reflect(dev, next_state);
-		local_irq_enable();
-		goto out;
+		cpuidle_reflect(dev, entered_state);
+	} else {
+		if (current_clr_polling_and_test())
+			local_irq_enable();
+		else
+			arch_cpu_idle();
 	}
+	__current_set_polling();
 
-	trace_cpu_idle_rcuidle(next_state, dev->cpu);
-
-	entered_state = cpuidle_enter(drv, dev, next_state);
-
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
-
-	/* give the governor an opportunity to reflect on the outcome */
-	cpuidle_reflect(dev, entered_state);
-out:
 	if (WARN_ON_ONCE(irqs_disabled()))
 		local_irq_enable();
 
@@ -145,16 +143,11 @@ static void cpu_idle_loop(void)
 			 * know that the IPI is going to arrive right
 			 * away
 			 */
-			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
+			if (cpu_idle_force_poll || tick_check_broadcast_expired())
 				cpu_idle_poll();
-			} else {
-				if (!current_clr_polling_and_test()) {
-					cpuidle_idle_call();
-				} else {
-					local_irq_enable();
-				}
-				__current_set_polling();
-			}
+			else
+				cpu_idle_call();
+
 			arch_cpu_idle_exit();
 			/*
 			 * We need to test and propagate the TIF_NEED_RESCHED

Re: [PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Daniel Lezcano]

On 02/24/2014 05:05 PM, Peter Zijlstra wrote:
> On Mon, Feb 24, 2014 at 04:39:08PM +0100, Daniel Lezcano wrote:
>
>>> And when you do that; you can also push down the
>>> current_clr_polling_and_test() muck so it doesn't cover the actual
>>> cpuidle policy code.
>>
>> I am not getting it. Where do you suggest to move it ?
>
> A little something like so I suppose;

Thanks for taking the time to send me this patch.

> only call
> current_clr_polling_and_test() right before calling the actual idle
> function. Although I suppose cpuidle_enter() can still do all sorts of
> weird.

Well there is the polling idle state for the x86 and ppc cpuidle 
drivers. Except that, I think we have something more or less clean.

> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -78,37 +78,35 @@ static int cpuidle_idle_call(void)
>   	stop_critical_timings();
>   	rcu_idle_enter();
>
> -	next_state = cpuidle_enabled(drv, dev);
> -	if (next_state < 0) {
> -		arch_cpu_idle();
> -		goto out;
> -	}
> -
> -	/*
> -	 * Ask the governor for the next state, this call can fail for
> -	 * different reasons: cpuidle is not enabled or an idle state
> -	 * fulfilling the constraints was not found. In this case, we
> -	 * fall back to the default idle function
> -	 */
> -	next_state = cpuidle_select(drv, dev);
> +	if (cpuidle_enabled(drv, dev) == 0) {
> +		/*
> +		 * Ask the governor for the next state, this call can fail for
> +		 * different reasons: cpuidle is not enabled or an idle state
> +		 * fulfilling the constraints was not found. In this case, we
> +		 * fall back to the default idle function
> +		 */
> +		next_state = cpuidle_select(drv, dev);
> +
> +		if (current_clr_polling_and_test()) {
> +			dev->last_residency = 0;
> +			entered_state = next_state;
> +			local_irq_enable();
> +		} else {
> +			trace_cpu_idle_rcuidle(next_state, dev->cpu);
> +			entered_state = cpuidle_enter(drv, dev, next_state);
> +			trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> +		}
>
> -	if (need_resched()) {

Ok. The need_resched is now replaced by 'current_clr_polling_and_test', 
with a call to '__current_set_polling()' to set the flag back, right ?

For my personal information, what is the subtlety with:

if (tif_need_resched())
	set_preempt_need_resched();

?

> -		dev->last_residency = 0;
>   		/* give the governor an opportunity to reflect on the outcome */
> -		cpuidle_reflect(dev, next_state);
> -		local_irq_enable();
> -		goto out;
> +		cpuidle_reflect(dev, entered_state);
> +	} else {
> +		if (current_clr_polling_and_test())
> +			local_irq_enable();
> +		else
> +			arch_cpu_idle();
>   	}
> +	__current_set_polling();
>
> -	trace_cpu_idle_rcuidle(next_state, dev->cpu);
> -
> -	entered_state = cpuidle_enter(drv, dev, next_state);
> -
> -	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> -
> -	/* give the governor an opportunity to reflect on the outcome */
> -	cpuidle_reflect(dev, entered_state);
> -out:
>   	if (WARN_ON_ONCE(irqs_disabled()))
>   		local_irq_enable();
>
> @@ -145,16 +143,11 @@ static void cpu_idle_loop(void)
>   			 * know that the IPI is going to arrive right
>   			 * away
>   			 */
> -			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
> +			if (cpu_idle_force_poll || tick_check_broadcast_expired())
>   				cpu_idle_poll();
> -			} else {
> -				if (!current_clr_polling_and_test()) {
> -					cpuidle_idle_call();
> -				} else {
> -					local_irq_enable();
> -				}
> -				__current_set_polling();
> -			}
> +			else
> +				cpu_idle_call();
> +
>   			arch_cpu_idle_exit();
>   			/*
>   			 * We need to test and propagate the TIF_NEED_RESCHED
>


-- 
  <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 V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Peter Zijlstra]

On Mon, Feb 24, 2014 at 06:03:10PM +0100, Daniel Lezcano wrote:
> Well there is the polling idle state for the x86 and ppc cpuidle drivers.
> Except that, I think we have something more or less clean.

Yeah, they have to set it back to polling again :/

Ideally we'd sweep the entire tree and switch the default polling state
to polling and add a current_clr_polling_and_test() to all WFI/HLT like
ones that need the interrupt.

But lots of work that.

> >-	if (need_resched()) {
> 
> Ok. The need_resched is now replaced by 'current_clr_polling_and_test', with
> a call to '__current_set_polling()' to set the flag back, right ?

Yah.

> For my personal information, what is the subtlety with:
> 
> if (tif_need_resched())
> 	set_preempt_need_resched();
> 
> ?

Urgh, looks like something went wrong with: cf37b6b48428d 

That commit doesn't actually remove kernel/cpu/idle.c nor is the new
code an exact replica of the old one.

Ingo, any chance we can get that fixed?

Daniel; does the below change/comment clarify?


---
Subject: sched/idle: Fixup merge fail of idle.c move

Commit cf37b6b48428d ("sched/idle: Move cpu/idle.c to sched/idle.c")
said to simply move a file; somehow it got mangled and created an old
version of the file and forgot to remove the old file.

Fix this fail; add the lost change and remove the now identical old
file.

Cc: Nicolas Pitre <nico@linaro.org>
Fixes: cf37b6b48428d ("sched/idle: Move cpu/idle.c to sched/idle.c")
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
---
 kernel/cpu/idle.c   | 147 ----------------------------------------------------
 kernel/sched/idle.c |  17 +++---
 2 files changed, 10 insertions(+), 154 deletions(-)

diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c
deleted file mode 100644
index b7976a127178..000000000000
--- a/kernel/cpu/idle.c
+++ /dev/null
@@ -1,147 +0,0 @@
-/*
- * Generic entry point for the idle threads
- */
-#include <linux/sched.h>
-#include <linux/cpu.h>
-#include <linux/cpuidle.h>
-#include <linux/tick.h>
-#include <linux/mm.h>
-#include <linux/stackprotector.h>
-
-#include <asm/tlb.h>
-
-#include <trace/events/power.h>
-
-static int __read_mostly cpu_idle_force_poll;
-
-void cpu_idle_poll_ctrl(bool enable)
-{
-	if (enable) {
-		cpu_idle_force_poll++;
-	} else {
-		cpu_idle_force_poll--;
-		WARN_ON_ONCE(cpu_idle_force_poll < 0);
-	}
-}
-
-#ifdef CONFIG_GENERIC_IDLE_POLL_SETUP
-static int __init cpu_idle_poll_setup(char *__unused)
-{
-	cpu_idle_force_poll = 1;
-	return 1;
-}
-__setup("nohlt", cpu_idle_poll_setup);
-
-static int __init cpu_idle_nopoll_setup(char *__unused)
-{
-	cpu_idle_force_poll = 0;
-	return 1;
-}
-__setup("hlt", cpu_idle_nopoll_setup);
-#endif
-
-static inline int cpu_idle_poll(void)
-{
-	rcu_idle_enter();
-	trace_cpu_idle_rcuidle(0, smp_processor_id());
-	local_irq_enable();
-	while (!tif_need_resched())
-		cpu_relax();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
-	rcu_idle_exit();
-	return 1;
-}
-
-/* Weak implementations for optional arch specific functions */
-void __weak arch_cpu_idle_prepare(void) { }
-void __weak arch_cpu_idle_enter(void) { }
-void __weak arch_cpu_idle_exit(void) { }
-void __weak arch_cpu_idle_dead(void) { }
-void __weak arch_cpu_idle(void)
-{
-	cpu_idle_force_poll = 1;
-	local_irq_enable();
-}
-
-/*
- * Generic idle loop implementation
- */
-static void cpu_idle_loop(void)
-{
-	while (1) {
-		tick_nohz_idle_enter();
-
-		while (!need_resched()) {
-			check_pgt_cache();
-			rmb();
-
-			if (cpu_is_offline(smp_processor_id()))
-				arch_cpu_idle_dead();
-
-			local_irq_disable();
-			arch_cpu_idle_enter();
-
-			/*
-			 * In poll mode we reenable interrupts and spin.
-			 *
-			 * Also if we detected in the wakeup from idle
-			 * path that the tick broadcast device expired
-			 * for us, we don't want to go deep idle as we
-			 * know that the IPI is going to arrive right
-			 * away
-			 */
-			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
-				cpu_idle_poll();
-			} else {
-				if (!current_clr_polling_and_test()) {
-					stop_critical_timings();
-					rcu_idle_enter();
-					if (cpuidle_idle_call())
-						arch_cpu_idle();
-					if (WARN_ON_ONCE(irqs_disabled()))
-						local_irq_enable();
-					rcu_idle_exit();
-					start_critical_timings();
-				} else {
-					local_irq_enable();
-				}
-				__current_set_polling();
-			}
-			arch_cpu_idle_exit();
-		}
-
-		/*
-		 * Since we fell out of the loop above, we know
-		 * TIF_NEED_RESCHED must be set, propagate it into
-		 * PREEMPT_NEED_RESCHED.
-		 *
-		 * This is required because for polling idle loops we will
-		 * not have had an IPI to fold the state for us.
-		 */
-		preempt_set_need_resched();
-		tick_nohz_idle_exit();
-		schedule_preempt_disabled();
-	}
-}
-
-void cpu_startup_entry(enum cpuhp_state state)
-{
-	/*
-	 * This #ifdef needs to die, but it's too late in the cycle to
-	 * make this generic (arm and sh have never invoked the canary
-	 * init for the non boot cpus!). Will be fixed in 3.11
-	 */
-#ifdef CONFIG_X86
-	/*
-	 * If we're the non-boot CPU, nothing set the stack canary up
-	 * for us. The boot CPU already has it initialized but no harm
-	 * in doing it again. This is a good place for updating it, as
-	 * we wont ever return from this function (so the invalid
-	 * canaries already on the stack wont ever trigger).
-	 */
-	boot_init_stack_canary();
-#endif
-	__current_set_polling();
-	arch_cpu_idle_prepare();
-	cpu_idle_loop();
-}
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 14ca43430aee..b7976a127178 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -108,14 +108,17 @@ static void cpu_idle_loop(void)
 				__current_set_polling();
 			}
 			arch_cpu_idle_exit();
-			/*
-			 * We need to test and propagate the TIF_NEED_RESCHED
-			 * bit here because we might not have send the
-			 * reschedule IPI to idle tasks.
-			 */
-			if (tif_need_resched())
-				set_preempt_need_resched();
 		}
+
+		/*
+		 * Since we fell out of the loop above, we know
+		 * TIF_NEED_RESCHED must be set, propagate it into
+		 * PREEMPT_NEED_RESCHED.
+		 *
+		 * This is required because for polling idle loops we will
+		 * not have had an IPI to fold the state for us.
+		 */
+		preempt_set_need_resched();
 		tick_nohz_idle_exit();
 		schedule_preempt_disabled();
 	}

Re: [PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Nicolas Pitre]

On Mon, 24 Feb 2014, Peter Zijlstra wrote:

> Urgh, looks like something went wrong with: cf37b6b48428d 
> 
> That commit doesn't actually remove kernel/cpu/idle.c nor is the new
> code an exact replica of the old one.
> 
> Ingo, any chance we can get that fixed?

The patch I posted was based on v3.13.  It doesn't apply on later 
kernels without a conflict.  I suspect the conflict resolution was 
goofed somehow.


Nicolas

Re: [PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Peter Zijlstra]

On Mon, Feb 24, 2014 at 12:54:52PM -0500, Nicolas Pitre wrote:
> On Mon, 24 Feb 2014, Peter Zijlstra wrote:
> 
> > Urgh, looks like something went wrong with: cf37b6b48428d 
> > 
> > That commit doesn't actually remove kernel/cpu/idle.c nor is the new
> > code an exact replica of the old one.
> > 
> > Ingo, any chance we can get that fixed?
> 
> The patch I posted was based on v3.13.  It doesn't apply on later 
> kernels without a conflict.  I suspect the conflict resolution was 
> goofed somehow.

Jah, something went bad. I actually remember fixing that patch, but
clearly I must've made a massive fail someplace.

Re: [PATCH V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Daniel Lezcano]

On 02/24/2014 06:22 PM, Peter Zijlstra wrote:
> On Mon, Feb 24, 2014 at 06:03:10PM +0100, Daniel Lezcano wrote:
>> Well there is the polling idle state for the x86 and ppc cpuidle drivers.
>> Except that, I think we have something more or less clean.
>
> Yeah, they have to set it back to polling again :/
>
> Ideally we'd sweep the entire tree and switch the default polling state
> to polling and add a current_clr_polling_and_test() to all WFI/HLT like
> ones that need the interrupt.
>
> But lots of work that.
>
>>> -	if (need_resched()) {
>>
>> Ok. The need_resched is now replaced by 'current_clr_polling_and_test', with
>> a call to '__current_set_polling()' to set the flag back, right ?
>
> Yah.
>
>> For my personal information, what is the subtlety with:
>>
>> if (tif_need_resched())
>> 	set_preempt_need_resched();
>>
>> ?
>
> Urgh, looks like something went wrong with: cf37b6b48428d
>
> That commit doesn't actually remove kernel/cpu/idle.c nor is the new
> code an exact replica of the old one.
>
> Ingo, any chance we can get that fixed?
>
> Daniel; does the below change/comment clarify?

Yes, I think so.

[ ... ]

> +
> +		/*
> +		 * Since we fell out of the loop above, we know
> +		 * TIF_NEED_RESCHED must be set, propagate it into
> +		 * PREEMPT_NEED_RESCHED.
> +		 *
> +		 * This is required because for polling idle loops we will
> +		 * not have had an IPI to fold the state for us.
> +		 */
> +		preempt_set_need_resched();
>   		tick_nohz_idle_exit();
>   		schedule_preempt_disabled();

So IIUC, the mainloop has two states: one where it is blocked on a 
HLT/WFI instruction (or about to enter/ exit this state) and another one 
outside of this blocking section.

When the idle task is blocked on HLT/WFI, it needs the IPI-reschedule in 
order to be woken up and rescheduled. But if it is outside this section, 
the idle task is not waiting for an interrupt and an expensive IPI can 
be saved by just setting the TS_POLLING flag, the scheduler will check 
this flag and won't send the IPI.

But 'set_preempt_need_resched' is called from the IPI handler. So if no 
IPI is sent because the idle task is in polling state, we have to set it 
ourself.

Now, the difference between the old code with 'tif_need_resched()' is 
because we don't need to check it because it is always true.

Am I right ?




-- 
  <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 V2 4/5] idle: Move idle conditions in cpuidle_idle main function

[Peter Zijlstra]

On Mon, Feb 24, 2014 at 08:04:25PM +0100, Daniel Lezcano wrote:
> >+		/*
> >+		 * Since we fell out of the loop above, we know
> >+		 * TIF_NEED_RESCHED must be set, propagate it into
> >+		 * PREEMPT_NEED_RESCHED.
> >+		 *
> >+		 * This is required because for polling idle loops we will
> >+		 * not have had an IPI to fold the state for us.
> >+		 */
> >+		preempt_set_need_resched();
> >  		tick_nohz_idle_exit();
> >  		schedule_preempt_disabled();
> 
> So IIUC, the mainloop has two states: one where it is blocked on a HLT/WFI
> instruction (or about to enter/ exit this state) and another one outside of
> this blocking section.

Almost; on x86 we have an monitor/mwait construct that blocks waiting
for a cacheline write. We point it at the thread_info->flags line. So
the TIF_NEED_RESCHED write from the other CPU wakes us up.

So no need to send an IPI after we write that flag.

> When the idle task is blocked on HLT/WFI, it needs the IPI-reschedule in
> order to be woken up and rescheduled. But if it is outside this section, the
> idle task is not waiting for an interrupt and an expensive IPI can be saved
> by just setting the TS_POLLING flag, the scheduler will check this flag and
> won't send the IPI.
> 
> But 'set_preempt_need_resched' is called from the IPI handler. So if no IPI
> is sent because the idle task is in polling state, we have to set it
> ourself.
> 
> Now, the difference between the old code with 'tif_need_resched()' is
> because we don't need to check it because it is always true.
> 
> Am I right ?

Yah, also it closes a very narrow window where TIF_NEED_RESCHED wasn't
set at the end of the while (!need_resched()) but is at the top.

Re: [PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Preeti U Murthy]

Hi Peter, Daniel,

On 02/24/2014 08:46 PM, Peter Zijlstra wrote:
> On Mon, Feb 24, 2014 at 04:12:07PM +0100, Daniel Lezcano wrote:
>> On 02/24/2014 04:00 PM, Peter Zijlstra wrote:
>>>
>>>
>>> None of this actually applies :/ I'm having major conflicts in
>>> driveres/cpuidle/cpuidle.c.
>>
>> Ok, except if I am missing something, the patchset is based on top of
>> tip/sched/core (commit 6990566).
> 
> Ah! So the below commit from the timers/core branch is in the way.

I had informed Daniel earlier of a possible conflict here. Anyway, I
think this patch should not move the BROADCAST_ENTER notification
out of the cpuidle_idle_call(). The reasons are:

1. Even if the patch handles the failed call to BROADCAST_ENTER in
cpuidle_enter(), you would have traced an entry into an idle state earlier
to this. If the entry into broadcast fails we should not be tracing either.

2. Moving the trace after the cpuidle_enter() call is wrong.

So I would suggest the patch at the end of this mail as the alternative
to this one so as to get around the patching conflict.

Thanks

Regards
Preeti U Murthy

> 
> Thomas, Ingo, how do we go about solving this sched/core vs timers/core
> conflict?
> 
> ---
> 
> # git log 6990566..tip/master drivers/cpuidle/
> commit 2f60b8ae0b2a006e4d3452e57ea98b59ce985d14
> Merge: 9eaf844efc23 849401b66d30
> Author: Ingo Molnar <mingo@kernel.org>
> Date:   Sat Feb 22 18:52:27 2014 +0100
> 
>     Merge branch 'timers/core'
> 
> commit ba8f20c2eb4158a443e9d6a909aee5010efa0c69
> Author: Preeti U Murthy <preeti@linux.vnet.ibm.com>
> Date:   Fri Feb 7 13:36:52 2014 +0530
> 
>     cpuidle: Handle clockevents_notify(BROADCAST_ENTER) failure
>     
>     Some archs set the CPUIDLE_FLAG_TIMER_STOP flag for idle states in which the
>     local timers stop. The cpuidle_idle_call() currently handles such idle states
>     by calling into the broadcast framework so as to wakeup CPUs at their next
>     wakeup event. With the hrtimer mode of broadcast, the BROADCAST_ENTER call
>     into the broadcast frameowork can fail for archs that do not have an external
>     clock device to handle wakeups and the CPU in question has thus to be made
>     the stand by CPU. This patch handles such cases by failing the call into
>     cpuidle so that the arch can take some default action. The arch will certainly
>     not enter a similar idle state because a failed cpuidle call will also implicitly
>     indicate that the broadcast framework has not registered this CPU to be woken up.
>     Hence we are safe if we fail the cpuidle call.
>     
>     In the process move the functions that trace idle statistics just before and
>     after the entry and exit into idle states respectively. In other
>     scenarios where the call to cpuidle fails, we end up not tracing idle
>     entry and exit since a decision on an idle state could not be taken. Similarly
>     when the call to broadcast framework fails, we skip tracing idle statistics
>     because we are in no further position to take a decision on an alternative
>     idle state to enter into.
>     
>     Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>     Cc: deepthi@linux.vnet.ibm.com
>     Cc: paulmck@linux.vnet.ibm.com
>     Cc: fweisbec@gmail.com
>     Cc: paulus@samba.org
>     Cc: srivatsa.bhat@linux.vnet.ibm.com
>     Cc: svaidy@linux.vnet.ibm.com
>     Cc: peterz@infradead.org
>     Cc: benh@kernel.crashing.org
>     Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>     Cc: linuxppc-dev@lists.ozlabs.org
>     Link: http://lkml.kernel.org/r/20140207080652.17187.66344.stgit@preeti.in.ibm.com
>     Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 

Signed-off-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
---
 drivers/cpuidle/cpuidle.c |   98 ++++++++++++++++++++++++++++++++++++---------
 include/linux/cpuidle.h   |   19 +++++++++
 2 files changed, 98 insertions(+), 19 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 09d05ab..a58c15b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -65,6 +65,26 @@ int cpuidle_play_dead(void)
 }
 
 /**
+ * cpuidle_enabled - check if the cpuidle framework is ready
+ * @dev: cpuidle device for this cpu
+ * @drv: cpuidle driver for this cpu
+ *
+ * Return 0 on success, otherwise:
+ * -NODEV : the cpuidle framework is not available
+ * -EBUSY : the cpuidle framework is not initialized
+ */
+int cpuidle_enabled(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+{
+	if (off || !initialized)
+		return -ENODEV;
+
+	if (!drv || !dev || !dev->enabled)
+		return -EBUSY;
+
+	return 0;
+}
+
+/**
  * cpuidle_enter_state - enter the state and update stats
  * @dev: cpuidle device for this cpu
  * @drv: cpuidle driver for this cpu
@@ -108,6 +128,56 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 }
 
 /**
+ * cpuidle_select - ask the cpuidle framework to choose an idle state
+ *
+ * @drv: the cpuidle driver
+ * @dev: the cpuidle device
+ *
+ * Returns the index of the idle state.
+ */
+int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+{
+	return cpuidle_curr_governor->select(drv, dev);
+}
+
+/**
+ * cpuidle_enter - enter into the specified idle state
+ *
+ * @drv:   the cpuidle driver tied with the cpu
+ * @dev:   the cpuidle device
+ * @index: the index in the idle state table
+ *
+ * Returns the index in the idle state, < 0 in case of error.
+ * The error code depends on the backend driver
+ */
+int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
+		  int index)
+{
+	int entered_state;
+
+	if (cpuidle_state_is_coupled(dev, drv, index))
+		entered_state = cpuidle_enter_state_coupled(dev, drv, index);
+	else
+		entered_state = cpuidle_enter_state(dev, drv, index);
+
+	return entered_state;
+}
+
+/**
+ * cpuidle_reflect - tell the underlying governor what was the state
+ * we were in
+ *
+ * @dev  : the cpuidle device
+ * @index: the index in the idle state table
+ *
+ */
+void cpuidle_reflect(struct cpuidle_device *dev, int index)
+{
+	if (cpuidle_curr_governor->reflect)
+		cpuidle_curr_governor->reflect(dev, index);
+}
+
+/**
  * cpuidle_idle_call - the main idle loop
  *
  * NOTE: no locks or semaphores should be used here
@@ -116,26 +186,21 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct cpuidle_driver *drv,
 int cpuidle_idle_call(void)
 {
 	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
-	struct cpuidle_driver *drv;
+	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
 	bool broadcast;
 
-	if (off || !initialized)
-		return -ENODEV;
-
-	/* check if the device is ready */
-	if (!dev || !dev->enabled)
-		return -EBUSY;
-
-	drv = cpuidle_get_cpu_driver(dev);
+	next_state = cpuidle_enabled(drv, dev);
+	if (next_state < 0)
+		return next_state;
 
 	/* ask the governor for the next state */
-	next_state = cpuidle_curr_governor->select(drv, dev);
+	next_state = cpuidle_select(drv, dev);
+
 	if (need_resched()) {
 		dev->last_residency = 0;
 		/* give the governor an opportunity to reflect on the outcome */
-		if (cpuidle_curr_governor->reflect)
-			cpuidle_curr_governor->reflect(dev, next_state);
+		cpuidle_reflect(dev, next_state);
 		local_irq_enable();
 		return 0;
 	}
@@ -149,11 +214,7 @@ int cpuidle_idle_call(void)
 
 	trace_cpu_idle_rcuidle(next_state, dev->cpu);
 
-	if (cpuidle_state_is_coupled(dev, drv, next_state))
-		entered_state = cpuidle_enter_state_coupled(dev, drv,
-							    next_state);
-	else
-		entered_state = cpuidle_enter_state(dev, drv, next_state);
+	entered_state = cpuidle_enter(drv, dev, next_state);
 
 	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
 
@@ -161,8 +222,7 @@ int cpuidle_idle_call(void)
 		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
 
 	/* give the governor an opportunity to reflect on the outcome */
-	if (cpuidle_curr_governor->reflect)
-		cpuidle_curr_governor->reflect(dev, entered_state);
+	cpuidle_reflect(dev, entered_state);
 
 	return 0;
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 50fcbb0..accc2dd 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -119,6 +119,15 @@ struct cpuidle_driver {
 
 #ifdef CONFIG_CPU_IDLE
 extern void disable_cpuidle(void);
+
+extern int cpuidle_enabled(struct cpuidle_driver *drv,
+			  struct cpuidle_device *dev);
+extern int cpuidle_select(struct cpuidle_driver *drv,
+			  struct cpuidle_device *dev);
+extern int cpuidle_enter(struct cpuidle_driver *drv,
+			 struct cpuidle_device *dev, int index);
+extern void cpuidle_reflect(struct cpuidle_device *dev, int index);
+
 extern int cpuidle_idle_call(void);
 extern int cpuidle_register_driver(struct cpuidle_driver *drv);
 extern struct cpuidle_driver *cpuidle_get_driver(void);
@@ -141,6 +150,16 @@ extern int cpuidle_play_dead(void);
 extern struct cpuidle_driver *cpuidle_get_cpu_driver(struct cpuidle_device *dev);
 #else
 static inline void disable_cpuidle(void) { }
+static inline int cpuidle_enabled(struct cpuidle_driver *drv,
+				  struct cpuidle_device *dev)
+{return -ENODEV; }
+static inline int cpuidle_select(struct cpuidle_driver *drv,
+				 struct cpuidle_device *dev)
+{return -ENODEV; }
+static inline int cpuidle_enter(struct cpuidle_driver *drv,
+				struct cpuidle_device *dev, int index)
+{return -ENODEV; }
+static inline void cpuidle_reflect(struct cpuidle_device *dev, int index) { }
 static inline int cpuidle_idle_call(void) { return -ENODEV; }
 static inline int cpuidle_register_driver(struct cpuidle_driver *drv)
 {return -ENODEV; }

Re: [PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Preeti U Murthy]

Hi Daniel,

On 02/24/2014 07:25 PM, Daniel Lezcano wrote:
> ---
>  drivers/cpuidle/cpuidle.c |  114 ++++++++++++++++++++++++++++++++++------------
>  include/linux/cpuidle.h   |   19 +++++++
>  2 files changed, 105 insertions(+), 28 deletions(-)
> 
> Index: cpuidle-next/drivers/cpuidle/cpuidle.c
> ===================================================================
> --- cpuidle-next.orig/drivers/cpuidle/cpuidle.c
> +++ cpuidle-next/drivers/cpuidle/cpuidle.c
> @@ -65,6 +65,26 @@ int cpuidle_play_dead(void)
>  }
> 
>  /**
> + * cpuidle_enabled - check if the cpuidle framework is ready
> + * @dev: cpuidle device for this cpu
> + * @drv: cpuidle driver for this cpu
> + *
> + * Return 0 on success, otherwise:
> + * -NODEV : the cpuidle framework is not available
> + * -EBUSY : the cpuidle framework is not initialized
> + */
> +int cpuidle_enabled(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> +{
> +	if (off || !initialized)
> +		return -ENODEV;
> +
> +	if (!drv || !dev || !dev->enabled)
> +		return -EBUSY;
> +
> +	return 0;
> +}
> +
> +/**
>   * cpuidle_enter_state - enter the state and update stats
>   * @dev: cpuidle device for this cpu
>   * @drv: cpuidle driver for this cpu
> @@ -108,6 +128,63 @@ int cpuidle_enter_state(struct cpuidle_d
>  }
> 
>  /**
> + * cpuidle_select - ask the cpuidle framework to choose an idle state
> + *
> + * @drv: the cpuidle driver
> + * @dev: the cpuidle device
> + *
> + * Returns the index of the idle state.
> + */
> +int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> +{
> +	return cpuidle_curr_governor->select(drv, dev);
> +}
> +
> +/**
> + * cpuidle_enter - enter into the specified idle state
> + *
> + * @drv:   the cpuidle driver tied with the cpu
> + * @dev:   the cpuidle device
> + * @index: the index in the idle state table
> + *
> + * Returns the index in the idle state, < 0 in case of error.
> + * The error code depends on the backend driver
> + */
> +int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
> +		  int index)
> +{
> +	int entered_state;
> +	bool broadcast = !!(drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP);
> +
> +	if (broadcast)
> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> +
> +	if (cpuidle_state_is_coupled(dev, drv, index))
> +		entered_state = cpuidle_enter_state_coupled(dev, drv, index);
> +	else
> +		entered_state = cpuidle_enter_state(dev, drv, index);
> +
> +	if (broadcast)
> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +
> +	return entered_state;
> +}
> +
> +/**
> + * cpuidle_reflect - tell the underlying governor what was the state
> + * we were in
> + *
> + * @dev  : the cpuidle device
> + * @index: the index in the idle state table
> + *
> + */
> +void cpuidle_reflect(struct cpuidle_device *dev, int index)
> +{
> +	if (cpuidle_curr_governor->reflect)
> +		cpuidle_curr_governor->reflect(dev, index);
> +}
> +
> +/**
>   * cpuidle_idle_call - the main idle loop
>   *
>   * NOTE: no locks or semaphores should be used here
> @@ -116,51 +193,32 @@ int cpuidle_enter_state(struct cpuidle_d
>  int cpuidle_idle_call(void)
>  {
>  	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
> -	struct cpuidle_driver *drv;
> +	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>  	int next_state, entered_state;
> -	bool broadcast;
> 
> -	if (off || !initialized)
> -		return -ENODEV;
> -
> -	/* check if the device is ready */
> -	if (!dev || !dev->enabled)
> -		return -EBUSY;
> -
> -	drv = cpuidle_get_cpu_driver(dev);
> +	next_state = cpuidle_enabled(drv, dev);

Accepting the return of cpuidle_enabled() into a parameter named
"next_state" looks misleading. You are not returning any idle state. You
could use ret/enabled to accept the return value here perhaps?

> +	if (next_state < 0)
> +		return next_state;
> 
>  	/* ask the governor for the next state */
> -	next_state = cpuidle_curr_governor->select(drv, dev);
> +	next_state = cpuidle_select(drv, dev);
> +
>  	if (need_resched()) {
>  		dev->last_residency = 0;
>  		/* give the governor an opportunity to reflect on the outcome */
> -		if (cpuidle_curr_governor->reflect)
> -			cpuidle_curr_governor->reflect(dev, next_state);
> +		cpuidle_reflect(dev, next_state);
>  		local_irq_enable();
>  		return 0;
>  	}
> 
>  	trace_cpu_idle_rcuidle(next_state, dev->cpu);
> 
> -	broadcast = !!(drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP);
> -
> -	if (broadcast)
> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> -
> -	if (cpuidle_state_is_coupled(dev, drv, next_state))
> -		entered_state = cpuidle_enter_state_coupled(dev, drv,
> -							    next_state);
> -	else
> -		entered_state = cpuidle_enter_state(dev, drv, next_state);
> -
> -	if (broadcast)
> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +	entered_state = cpuidle_enter(drv, dev, next_state);

Shouldn't the return value be checked here, considering you are
expecting the driver to return an error code. Another reason I mention
this is that since you would have done BROADCAST_ENTRY and if this call
to the broadcast framework succeeds, you will have to do a
BROADCAST_EXIT irrespective of if the driver could put the CPU to that
idle state or not. So even if cpuidle_enter() fails, you will need to do
a clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu),
although you will have to skip over cpuidle_reflect().

So are there drivers which can return an error code when we call into
the enter function of the idle states? If not, you can probably avoid
checking the error code return value of cpuidle_enter() altogether and
make it simpler. Its not being checked in the current code too.

Thanks

Regards
Preeti U Murthy
> 
>  	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> 
>  	/* give the governor an opportunity to reflect on the outcome */
> -	if (cpuidle_curr_governor->reflect)
> -		cpuidle_curr_governor->reflect(dev, entered_state);
> +	cpuidle_reflect(dev, entered_state);
> 
>  	return 0;
>  }

Re: [PATCH V2 1/5] idle/cpuidle: Split cpuidle_idle_call main function into smaller functions

[Daniel Lezcano]

On 02/25/2014 04:47 AM, Preeti U Murthy wrote:
> Hi Daniel,
>
> On 02/24/2014 07:25 PM, Daniel Lezcano wrote:
>> ---
>>   drivers/cpuidle/cpuidle.c |  114 ++++++++++++++++++++++++++++++++++------------
>>   include/linux/cpuidle.h   |   19 +++++++
>>   2 files changed, 105 insertions(+), 28 deletions(-)
>>
>> Index: cpuidle-next/drivers/cpuidle/cpuidle.c
>> ===================================================================
>> --- cpuidle-next.orig/drivers/cpuidle/cpuidle.c
>> +++ cpuidle-next/drivers/cpuidle/cpuidle.c
>> @@ -65,6 +65,26 @@ int cpuidle_play_dead(void)
>>   }
>>
>>   /**
>> + * cpuidle_enabled - check if the cpuidle framework is ready
>> + * @dev: cpuidle device for this cpu
>> + * @drv: cpuidle driver for this cpu
>> + *
>> + * Return 0 on success, otherwise:
>> + * -NODEV : the cpuidle framework is not available
>> + * -EBUSY : the cpuidle framework is not initialized
>> + */
>> +int cpuidle_enabled(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>> +{
>> +	if (off || !initialized)
>> +		return -ENODEV;
>> +
>> +	if (!drv || !dev || !dev->enabled)
>> +		return -EBUSY;
>> +
>> +	return 0;
>> +}
>> +
>> +/**
>>    * cpuidle_enter_state - enter the state and update stats
>>    * @dev: cpuidle device for this cpu
>>    * @drv: cpuidle driver for this cpu
>> @@ -108,6 +128,63 @@ int cpuidle_enter_state(struct cpuidle_d
>>   }
>>
>>   /**
>> + * cpuidle_select - ask the cpuidle framework to choose an idle state
>> + *
>> + * @drv: the cpuidle driver
>> + * @dev: the cpuidle device
>> + *
>> + * Returns the index of the idle state.
>> + */
>> +int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>> +{
>> +	return cpuidle_curr_governor->select(drv, dev);
>> +}
>> +
>> +/**
>> + * cpuidle_enter - enter into the specified idle state
>> + *
>> + * @drv:   the cpuidle driver tied with the cpu
>> + * @dev:   the cpuidle device
>> + * @index: the index in the idle state table
>> + *
>> + * Returns the index in the idle state, < 0 in case of error.
>> + * The error code depends on the backend driver
>> + */
>> +int cpuidle_enter(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>> +		  int index)
>> +{
>> +	int entered_state;
>> +	bool broadcast = !!(drv->states[index].flags & CPUIDLE_FLAG_TIMER_STOP);
>> +
>> +	if (broadcast)
>> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> +
>> +	if (cpuidle_state_is_coupled(dev, drv, index))
>> +		entered_state = cpuidle_enter_state_coupled(dev, drv, index);
>> +	else
>> +		entered_state = cpuidle_enter_state(dev, drv, index);
>> +
>> +	if (broadcast)
>> +		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> +
>> +	return entered_state;
>> +}
>> +
>> +/**
>> + * cpuidle_reflect - tell the underlying governor what was the state
>> + * we were in
>> + *
>> + * @dev  : the cpuidle device
>> + * @index: the index in the idle state table
>> + *
>> + */
>> +void cpuidle_reflect(struct cpuidle_device *dev, int index)
>> +{
>> +	if (cpuidle_curr_governor->reflect)
>> +		cpuidle_curr_governor->reflect(dev, index);
>> +}
>> +
>> +/**
>>    * cpuidle_idle_call - the main idle loop
>>    *
>>    * NOTE: no locks or semaphores should be used here
>> @@ -116,51 +193,32 @@ int cpuidle_enter_state(struct cpuidle_d
>>   int cpuidle_idle_call(void)
>>   {
>>   	struct cpuidle_device *dev = __this_cpu_read(cpuidle_devices);
>> -	struct cpuidle_driver *drv;
>> +	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>>   	int next_state, entered_state;
>> -	bool broadcast;
>>
>> -	if (off || !initialized)
>> -		return -ENODEV;
>> -
>> -	/* check if the device is ready */
>> -	if (!dev || !dev->enabled)
>> -		return -EBUSY;
>> -
>> -	drv = cpuidle_get_cpu_driver(dev);
>> +	next_state = cpuidle_enabled(drv, dev);
>
> Accepting the return of cpuidle_enabled() into a parameter named
> "next_state" looks misleading. You are not returning any idle state. You
> could use ret/enabled to accept the return value here perhaps?

Yes, it was to save an extra variable. I can replace it by a 'ret'.

>> +	if (next_state < 0)
>> +		return next_state;
>>
>>   	/* ask the governor for the next state */
>> -	next_state = cpuidle_curr_governor->select(drv, dev);
>> +	next_state = cpuidle_select(drv, dev);
>> +
>>   	if (need_resched()) {
>>   		dev->last_residency = 0;
>>   		/* give the governor an opportunity to reflect on the outcome */
>> -		if (cpuidle_curr_governor->reflect)
>> -			cpuidle_curr_governor->reflect(dev, next_state);
>> +		cpuidle_reflect(dev, next_state);
>>   		local_irq_enable();
>>   		return 0;
>>   	}
>>
>>   	trace_cpu_idle_rcuidle(next_state, dev->cpu);
>>
>> -	broadcast = !!(drv->states[next_state].flags & CPUIDLE_FLAG_TIMER_STOP);
>> -
>> -	if (broadcast)
>> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> -
>> -	if (cpuidle_state_is_coupled(dev, drv, next_state))
>> -		entered_state = cpuidle_enter_state_coupled(dev, drv,
>> -							    next_state);
>> -	else
>> -		entered_state = cpuidle_enter_state(dev, drv, next_state);
>> -
>> -	if (broadcast)
>> -		clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> +	entered_state = cpuidle_enter(drv, dev, next_state);
>
> Shouldn't the return value be checked here, considering you are
> expecting the driver to return an error code. Another reason I mention
> this is that since you would have done BROADCAST_ENTRY and if this call
> to the broadcast framework succeeds, you will have to do a
> BROADCAST_EXIT irrespective of if the driver could put the CPU to that
> idle state or not. So even if cpuidle_enter() fails, you will need to do
> a clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu),
> although you will have to skip over cpuidle_reflect().
>
> So are there drivers which can return an error code when we call into
> the enter function of the idle states?

Except for the acpi noodle plate driver, no backend driver is returning 
an error.

> If not, you can probably avoid
> checking the error code return value of cpuidle_enter() altogether and
> make it simpler. Its not being checked in the current code too.

Yeah, that is the point. I don't want to mix the changes with this 
patchset. I agree, the code must be fixed but I prefer to do that in a 
separate patch.

Thanks for the review

   -- Daniel

>
> Thanks
>
> Regards
> Preeti U Murthy
>>
>>   	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
>>
>>   	/* give the governor an opportunity to reflect on the outcome */
>> -	if (cpuidle_curr_governor->reflect)
>> -		cpuidle_curr_governor->reflect(dev, entered_state);
>> +	cpuidle_reflect(dev, entered_state);
>>
>>   	return 0;
>>   }
>


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

[tip:sched/core] sched/idle: Remove stale old file

[tip-bot for Peter Zijlstra]

Commit-ID:  06d50c65b1043b166d102accc081093f79d8f7e5
Gitweb:     http://git.kernel.org/tip/06d50c65b1043b166d102accc081093f79d8f7e5
Author:     Peter Zijlstra <peterz@infradead.org>
AuthorDate: Mon, 24 Feb 2014 18:22:07 +0100
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Thu, 27 Feb 2014 12:41:01 +0100

sched/idle: Remove stale old file

Commit cf37b6b48428d ("sched/idle: Move cpu/idle.c to sched/idle.c")
said to simply move a file; somehow it got mangled and created an old
version of the file and forgot to remove the old file.

Fix this fail; add the lost change and remove the now identical old
file.

Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Cc: rjw@rjwysocki.net
Cc: nicolas.pitre@linaro.org
Cc: preeti@linux.vnet.ibm.com
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: http://lkml.kernel.org/r/20140224172207.GC9987@twins.programming.kicks-ass.net
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 kernel/cpu/idle.c   | 147 ----------------------------------------------------
 kernel/sched/idle.c |  17 +++---
 2 files changed, 10 insertions(+), 154 deletions(-)

diff --git a/kernel/cpu/idle.c b/kernel/cpu/idle.c
deleted file mode 100644
index b7976a1..0000000
--- a/kernel/cpu/idle.c
+++ /dev/null
@@ -1,147 +0,0 @@
-/*
- * Generic entry point for the idle threads
- */
-#include <linux/sched.h>
-#include <linux/cpu.h>
-#include <linux/cpuidle.h>
-#include <linux/tick.h>
-#include <linux/mm.h>
-#include <linux/stackprotector.h>
-
-#include <asm/tlb.h>
-
-#include <trace/events/power.h>
-
-static int __read_mostly cpu_idle_force_poll;
-
-void cpu_idle_poll_ctrl(bool enable)
-{
-	if (enable) {
-		cpu_idle_force_poll++;
-	} else {
-		cpu_idle_force_poll--;
-		WARN_ON_ONCE(cpu_idle_force_poll < 0);
-	}
-}
-
-#ifdef CONFIG_GENERIC_IDLE_POLL_SETUP
-static int __init cpu_idle_poll_setup(char *__unused)
-{
-	cpu_idle_force_poll = 1;
-	return 1;
-}
-__setup("nohlt", cpu_idle_poll_setup);
-
-static int __init cpu_idle_nopoll_setup(char *__unused)
-{
-	cpu_idle_force_poll = 0;
-	return 1;
-}
-__setup("hlt", cpu_idle_nopoll_setup);
-#endif
-
-static inline int cpu_idle_poll(void)
-{
-	rcu_idle_enter();
-	trace_cpu_idle_rcuidle(0, smp_processor_id());
-	local_irq_enable();
-	while (!tif_need_resched())
-		cpu_relax();
-	trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, smp_processor_id());
-	rcu_idle_exit();
-	return 1;
-}
-
-/* Weak implementations for optional arch specific functions */
-void __weak arch_cpu_idle_prepare(void) { }
-void __weak arch_cpu_idle_enter(void) { }
-void __weak arch_cpu_idle_exit(void) { }
-void __weak arch_cpu_idle_dead(void) { }
-void __weak arch_cpu_idle(void)
-{
-	cpu_idle_force_poll = 1;
-	local_irq_enable();
-}
-
-/*
- * Generic idle loop implementation
- */
-static void cpu_idle_loop(void)
-{
-	while (1) {
-		tick_nohz_idle_enter();
-
-		while (!need_resched()) {
-			check_pgt_cache();
-			rmb();
-
-			if (cpu_is_offline(smp_processor_id()))
-				arch_cpu_idle_dead();
-
-			local_irq_disable();
-			arch_cpu_idle_enter();
-
-			/*
-			 * In poll mode we reenable interrupts and spin.
-			 *
-			 * Also if we detected in the wakeup from idle
-			 * path that the tick broadcast device expired
-			 * for us, we don't want to go deep idle as we
-			 * know that the IPI is going to arrive right
-			 * away
-			 */
-			if (cpu_idle_force_poll || tick_check_broadcast_expired()) {
-				cpu_idle_poll();
-			} else {
-				if (!current_clr_polling_and_test()) {
-					stop_critical_timings();
-					rcu_idle_enter();
-					if (cpuidle_idle_call())
-						arch_cpu_idle();
-					if (WARN_ON_ONCE(irqs_disabled()))
-						local_irq_enable();
-					rcu_idle_exit();
-					start_critical_timings();
-				} else {
-					local_irq_enable();
-				}
-				__current_set_polling();
-			}
-			arch_cpu_idle_exit();
-		}
-
-		/*
-		 * Since we fell out of the loop above, we know
-		 * TIF_NEED_RESCHED must be set, propagate it into
-		 * PREEMPT_NEED_RESCHED.
-		 *
-		 * This is required because for polling idle loops we will
-		 * not have had an IPI to fold the state for us.
-		 */
-		preempt_set_need_resched();
-		tick_nohz_idle_exit();
-		schedule_preempt_disabled();
-	}
-}
-
-void cpu_startup_entry(enum cpuhp_state state)
-{
-	/*
-	 * This #ifdef needs to die, but it's too late in the cycle to
-	 * make this generic (arm and sh have never invoked the canary
-	 * init for the non boot cpus!). Will be fixed in 3.11
-	 */
-#ifdef CONFIG_X86
-	/*
-	 * If we're the non-boot CPU, nothing set the stack canary up
-	 * for us. The boot CPU already has it initialized but no harm
-	 * in doing it again. This is a good place for updating it, as
-	 * we wont ever return from this function (so the invalid
-	 * canaries already on the stack wont ever trigger).
-	 */
-	boot_init_stack_canary();
-#endif
-	__current_set_polling();
-	arch_cpu_idle_prepare();
-	cpu_idle_loop();
-}
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 14ca434..b7976a1 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -108,14 +108,17 @@ static void cpu_idle_loop(void)
 				__current_set_polling();
 			}
 			arch_cpu_idle_exit();
-			/*
-			 * We need to test and propagate the TIF_NEED_RESCHED
-			 * bit here because we might not have send the
-			 * reschedule IPI to idle tasks.
-			 */
-			if (tif_need_resched())
-				set_preempt_need_resched();
 		}
+
+		/*
+		 * Since we fell out of the loop above, we know
+		 * TIF_NEED_RESCHED must be set, propagate it into
+		 * PREEMPT_NEED_RESCHED.
+		 *
+		 * This is required because for polling idle loops we will
+		 * not have had an IPI to fold the state for us.
+		 */
+		preempt_set_need_resched();
 		tick_nohz_idle_exit();
 		schedule_preempt_disabled();
 	}