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

[PATCH 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>
---
 drivers/cpuidle/cpuidle.c |  102 ++++++++++++++++++++++++++++++++-------------
 include/linux/cpuidle.h   |   14 +++++++
 2 files changed, 87 insertions(+), 29 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index a55e68f..172ab6a 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -108,6 +108,71 @@ 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. On error it returns:
+ * -NODEV : the cpuidle framework is not available
+ * -EBUSY : the cpuidle framework is not initialized
+ */
+int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+{
+	if (off || !initialized)
+		return -ENODEV;
+
+	if (!drv || !dev || !dev->enabled)
+		return -EBUSY;
+
+	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 +181,30 @@ 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);
 
 	/* ask the governor for the next state */
-	next_state = cpuidle_curr_governor->select(drv, dev);
+	next_state = cpuidle_select(drv, dev);
+	if (next_state < 0)
+		return next_state;
+
 	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;
 }
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 50fcbb0..bf06f37 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -119,6 +119,13 @@ struct cpuidle_driver {
 
 #ifdef CONFIG_CPU_IDLE
 extern void disable_cpuidle(void);
+
+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 +148,13 @@ 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_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; }
-- 
1.7.9.5

[PATCH 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 |   37 -------------------------------------
 include/linux/cpuidle.h   |    2 --
 kernel/sched/idle.c       |   44 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 44 insertions(+), 39 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index 172ab6a..ae3078b 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -173,43 +173,6 @@ void cpuidle_reflect(struct cpuidle_device *dev, int index)
 }
 
 /**
- * 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;
-
-	/* ask the governor for the next state */
-	next_state = cpuidle_select(drv, dev);
-	if (next_state < 0)
-		return next_state;
-
-	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)
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index bf06f37..0befaff 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -126,7 +126,6 @@ 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);
 extern struct cpuidle_driver *cpuidle_driver_ref(void);
@@ -155,7 +154,6 @@ 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; }
 static inline struct cpuidle_driver *cpuidle_get_driver(void) {return NULL; }
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 14ca434..6963822 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -63,6 +63,50 @@ 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;
+
+	/* ask the governor for the next state */
+	next_state = cpuidle_select(drv, dev);
+	if (next_state < 0)
+		return next_state;
+
+	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
  */
-- 
1.7.9.5

[PATCH 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 the change the current behavior.

Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 include/linux/cpuidle.h |    2 ++
 kernel/sched/idle.c     |   39 ++++++++++++++++++++-------------------
 2 files changed, 22 insertions(+), 19 deletions(-)

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 0befaff..a8d5bd3 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -175,6 +175,8 @@ static inline int cpuidle_enable_device(struct cpuidle_device *dev)
 {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
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 6963822..8b10891 100644
--- a/kernel/sched/idle.c
+++ b/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,17 +75,26 @@ static int cpuidle_idle_call(void)
 	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
 	int next_state, entered_state;
 
-	/* ask the governor for the next state */
+	stop_critical_timings();
+	rcu_idle_enter();
+
+	/* 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 (next_state < 0)
-		return next_state;
+	if (next_state < 0) {
+		arch_cpu_idle();
+		goto out;
+	}
 
 	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;
+		goto out;
 	}
 
 	trace_cpu_idle_rcuidle(next_state, dev->cpu);
@@ -97,15 +105,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
@@ -138,14 +146,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();
 				}
-- 
1.7.9.5

[PATCH 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(-)

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 8b10891..72b5926 100644
--- a/kernel/sched/idle.c
+++ b/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();
 
@@ -112,6 +129,8 @@ out:
 	rcu_idle_exit();
 	start_critical_timings();
 
+	__current_set_polling();
+
 	return 0;
 }
 
@@ -133,25 +152,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
-- 
1.7.9.5

[PATCH 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>
---
 kernel/sched/idle.c |   37 +++++++++++++++++++++++++++++++++----
 1 file changed, 33 insertions(+), 4 deletions(-)

diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index 72b5926..36ff1a7 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -86,19 +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 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();
 
-	/* 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 but the call could fail if cpuidle is not enabled
 	 */
 	next_state = cpuidle_select(drv, dev);
 	if (next_state < 0) {
@@ -106,6 +121,10 @@ static int cpuidle_idle_call(void)
 		goto out;
 	}
 
+	/*
+	 * 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 */
@@ -116,6 +135,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);
@@ -152,6 +177,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();
-- 
1.7.9.5

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

[Nicolas Pitre]

On Tue, 11 Feb 2014, Daniel Lezcano wrote:

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

> ---
>  drivers/cpuidle/cpuidle.c |  102 ++++++++++++++++++++++++++++++++-------------
>  include/linux/cpuidle.h   |   14 +++++++
>  2 files changed, 87 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index a55e68f..172ab6a 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -108,6 +108,71 @@ 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. On error it returns:
> + * -NODEV : the cpuidle framework is not available
> + * -EBUSY : the cpuidle framework is not initialized
> + */
> +int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> +{
> +	if (off || !initialized)
> +		return -ENODEV;
> +
> +	if (!drv || !dev || !dev->enabled)
> +		return -EBUSY;
> +
> +	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 +181,30 @@ 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);
>  
>  	/* ask the governor for the next state */
> -	next_state = cpuidle_curr_governor->select(drv, dev);
> +	next_state = cpuidle_select(drv, dev);
> +	if (next_state < 0)
> +		return next_state;
> +
>  	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;
>  }
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 50fcbb0..bf06f37 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -119,6 +119,13 @@ struct cpuidle_driver {
>  
>  #ifdef CONFIG_CPU_IDLE
>  extern void disable_cpuidle(void);
> +
> +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 +148,13 @@ 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_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; }
> -- 
> 1.7.9.5
> 

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

[Nicolas Pitre]

On Tue, 11 Feb 2014, Daniel Lezcano wrote:

> 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 the change the current behavior.

s/the change/change/

> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  include/linux/cpuidle.h |    2 ++
>  kernel/sched/idle.c     |   39 ++++++++++++++++++++-------------------
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 0befaff..a8d5bd3 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -175,6 +175,8 @@ static inline int cpuidle_enable_device(struct cpuidle_device *dev)
>  {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
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 6963822..8b10891 100644
> --- a/kernel/sched/idle.c
> +++ b/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,17 +75,26 @@ static int cpuidle_idle_call(void)
>  	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>  	int next_state, entered_state;
>  
> -	/* ask the governor for the next state */
> +	stop_critical_timings();
> +	rcu_idle_enter();
> +
> +	/* 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
> +	 */

Preferred style for multi-line comment blocks is:

	/*
	 * First line here, leaving the opening line empty.
	 * Next line.
	 */

Otherwise...

Acked-by: Nicolas Pitre <nico@linaro.org>

>  	next_state = cpuidle_select(drv, dev);
> -	if (next_state < 0)
> -		return next_state;
> +	if (next_state < 0) {
> +		arch_cpu_idle();
> +		goto out;
> +	}
>  
>  	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;
> +		goto out;
>  	}
>  
>  	trace_cpu_idle_rcuidle(next_state, dev->cpu);
> @@ -97,15 +105,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
> @@ -138,14 +146,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();
>  				}
> -- 
> 1.7.9.5
> 

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

[Nicolas Pitre]

On Tue, 11 Feb 2014, Daniel Lezcano wrote:

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

Few questions below.  In any case,:

Acked-by: Nicolas Pitre <nico@linaro.org>

> ---
>  kernel/sched/idle.c |   37 +++++++++++++++++++++++++++++++++----
>  1 file changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 72b5926..36ff1a7 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -86,19 +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 rescheduled. If it is the case,

s/must/must be/

> +	 * 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();
>  
> -	/* 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 but the call could fail if cpuidle is not enabled
>  	 */
>  	next_state = cpuidle_select(drv, dev);
>  	if (next_state < 0) {
> @@ -106,6 +121,10 @@ static int cpuidle_idle_call(void)
>  		goto out;
>  	}
>  
> +	/*
> +	 * 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 */

Is this if block really necessary?  We already have need_resched() being 
monitored in the outer loop.  Are cpuidle_select() or rcu_idle_enter() 
likely to spend a significant amount of time justifying a recheck here?


Nicolas

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

[Daniel Lezcano]

On 02/11/2014 06:51 PM, Nicolas Pitre wrote:
> On Tue, 11 Feb 2014, Daniel Lezcano wrote:
>
>> 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>
>
> Few questions below.  In any case,:
>
> Acked-by: Nicolas Pitre <nico@linaro.org>

Thanks for the review Nico !

Answer below.

>> ---
>>   kernel/sched/idle.c |   37 +++++++++++++++++++++++++++++++++----
>>   1 file changed, 33 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
>> index 72b5926..36ff1a7 100644
>> --- a/kernel/sched/idle.c
>> +++ b/kernel/sched/idle.c
>> @@ -86,19 +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 rescheduled. If it is the case,
>
> s/must/must be/
>
>> +	 * 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();
>>
>> -	/* 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 but the call could fail if cpuidle is not enabled
>>   	 */
>>   	next_state = cpuidle_select(drv, dev);
>>   	if (next_state < 0) {
>> @@ -106,6 +121,10 @@ static int cpuidle_idle_call(void)
>>   		goto out;
>>   	}
>>
>> +	/*
>> +	 * 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 */
>
> Is this if block really necessary?  We already have need_resched() being
> monitored in the outer loop.  Are cpuidle_select() or rcu_idle_enter()
> likely to spend a significant amount of time justifying a recheck here?

That's a question I have been always asking myself.

The cpuidle_select function could spend some time for:

1. reflecting the idle time for the statistics of the previous idle 
period. This processing is post-poned when exiting an idle state via the 
'need_update' field in the cpuidle structure. I guess, this is because 
it can take a while and we want to exit asap to reduce the wakeup latency.

2. there are some processing to choose the idle state.

I don't know what is the rational here to use need_resched at this place 
except to 'abort' an idle state arbitrarily after some experimentation 
for better reactivity. I am wondering if the multiple need_resched() we 
find in the call stack for some idle states makes really sense and 
doesn't denote a lack of control of what is happening in the idle path 
vs system activity or a lack of confidence in the idle duration prediction.


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

[Preeti U Murthy]

Hi Daniel,

On 02/11/2014 08:41 PM, Daniel Lezcano wrote:
> 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>
> ---
>  drivers/cpuidle/cpuidle.c |  102 ++++++++++++++++++++++++++++++++-------------
>  include/linux/cpuidle.h   |   14 +++++++
>  2 files changed, 87 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index a55e68f..172ab6a 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -108,6 +108,71 @@ 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. On error it returns:
> + * -NODEV : the cpuidle framework is not available
> + * -EBUSY : the cpuidle framework is not initialized
> + */
> +int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
> +{
> +	if (off || !initialized)
> +		return -ENODEV;
> +
> +	if (!drv || !dev || !dev->enabled)
> +		return -EBUSY;

I would suggest moving the above two conditions under another function,
cpuidle_enabled() maybe? The reason is, cpuidle_select() indicates that,
it is invoked to select an idle state. While you are expecting this
function to return an idle state, it seems counter-intuitive to return a
ENODEV/EBUSY. This function is expected to be a call into the governor
specific code and the same function should not be used to verify if
cpuidle is enabled/not IMHO.

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

The tip tree, timers/core branch has the patch,
tick: Introduce hrtimer based broadcast. In the problem scenario that
this patchset is addressing, the call to broadcast framework may return
an error indicating that the idle state in question cannot be entered
into. I wanted to bring it to your notice, so that early on you can take
care of this. You will need to add code below in the invocation of
cpuidle_enter() to verify if the idle state was entered into or not. If
it was not, then you will need to skip tracing and reflecting of the
idle state and directly exit the cpuidle loop with a failed status.

> +
> +	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 +181,30 @@ 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);
> 
>  	/* ask the governor for the next state */
> -	next_state = cpuidle_curr_governor->select(drv, dev);
> +	next_state = cpuidle_select(drv, dev);
> +	if (next_state < 0)
> +		return next_state;
> +
>  	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;
>  }

Thanks

Regards
Preeti U Murthy

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

[Preeti U Murthy]

Hi Daniel,

On 02/11/2014 08:41 PM, Daniel Lezcano wrote:
> 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.

So my understanding of this patchset is that by moving
cpuidle_idle_call() under kernel/sched, we now have a way of calling
into the cpuidle governor and the cpuidle driver with additional
parameters like cpu_load(), idle_stamp etc.. so that we can expect the
governor and driver to take better decisions about entry and exit into
idle states. Is this the advantage we hope to begin with?

Thanks

Regards
Preeti U Murthy
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> Acked-by: Nicolas Pitre <nicolas.pitre@linaro.org>
> ---

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

[Preeti U Murthy]

Hi Daniel,

Find below a couple of comments.

On 02/11/2014 08:41 PM, Daniel Lezcano wrote:
> 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 the change the current behavior.
> 
> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
> ---
>  include/linux/cpuidle.h |    2 ++
>  kernel/sched/idle.c     |   39 ++++++++++++++++++++-------------------
>  2 files changed, 22 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 0befaff..a8d5bd3 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -175,6 +175,8 @@ static inline int cpuidle_enable_device(struct cpuidle_device *dev)
>  {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
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index 6963822..8b10891 100644
> --- a/kernel/sched/idle.c
> +++ b/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,17 +75,26 @@ static int cpuidle_idle_call(void)
>  	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>  	int next_state, entered_state;
> 
> -	/* ask the governor for the next state */
> +	stop_critical_timings();
> +	rcu_idle_enter();
> +
> +	/* 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 (next_state < 0)
> -		return next_state;
> +	if (next_state < 0) {
> +		arch_cpu_idle();
> +		goto out;
> +	}
> 
>  	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;
> +		goto out;
>  	}
> 
>  	trace_cpu_idle_rcuidle(next_state, dev->cpu);
> @@ -97,15 +105,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
> @@ -138,14 +146,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();
>  				}

Is this really necessary? It seems better to let the cpuidle_idle_loop()
to handle the cpuidle specific tasks and the generic idle loop to handle
the peripheral functions like stop_critical_timings(), rcu_idle_enter()
and their counterparts. I am unable to see what we are gaining by doing
this.

Besides, cpuidle_idle_call() is expected to just call into the cpuidle
governor and driver. What I would have expected this patchset to do is
simply move this call under kernel/sched like you have done in your
first two patches so as to gain the benefit of using the parameters that
the scheduler keeps track of to make better cpuidle state entry decisions.

But going one step too far by moving the other idle handling functions
into it would result in some confusion and add more code than it is
meant to handle. This will avoid having to add  comments in the
cpuidle_idle_call() function as currently being done in Patch[5/5], to
clarify what each function is meant to do.

So IMO, Patches[1/5] and [2/5] by themselves are sufficient to increase
the proximity between scheduler and cpuidle.


Thanks

Regards
Preeti U Murthy
> 

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

[Daniel Lezcano]

On 02/12/2014 11:43 AM, Preeti U Murthy wrote:
> Hi Daniel,
>
> On 02/11/2014 08:41 PM, Daniel Lezcano wrote:
>> 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.
>
> So my understanding of this patchset is that by moving
> cpuidle_idle_call() under kernel/sched, we now have a way of calling
> into the cpuidle governor and the cpuidle driver with additional
> parameters like cpu_load(), idle_stamp etc.. so that we can expect the
> governor and driver to take better decisions about entry and exit into
> idle states. Is this the advantage we hope to begin with?

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

[Daniel Lezcano]

On 02/12/2014 11:38 AM, Preeti U Murthy wrote:
> Hi Daniel,
>
> On 02/11/2014 08:41 PM, Daniel Lezcano wrote:
>> 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>
>> ---
>>   drivers/cpuidle/cpuidle.c |  102 ++++++++++++++++++++++++++++++++-------------
>>   include/linux/cpuidle.h   |   14 +++++++
>>   2 files changed, 87 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index a55e68f..172ab6a 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -108,6 +108,71 @@ 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. On error it returns:
>> + * -NODEV : the cpuidle framework is not available
>> + * -EBUSY : the cpuidle framework is not initialized
>> + */
>> +int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>> +{
>> +	if (off || !initialized)
>> +		return -ENODEV;
>> +
>> +	if (!drv || !dev || !dev->enabled)
>> +		return -EBUSY;
>
> I would suggest moving the above two conditions under another function,
> cpuidle_enabled() maybe? The reason is, cpuidle_select() indicates that,
> it is invoked to select an idle state. While you are expecting this
> function to return an idle state, it seems counter-intuitive to return a
> ENODEV/EBUSY. This function is expected to be a call into the governor
> specific code and the same function should not be used to verify if
> cpuidle is enabled/not IMHO.

Yes, I fully agree. I will fix that.

>> +
>> +	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);
>
> The tip tree, timers/core branch has the patch,
> tick: Introduce hrtimer based broadcast. In the problem scenario that
> this patchset is addressing, the call to broadcast framework may return
> an error indicating that the idle state in question cannot be entered
> into. I wanted to bring it to your notice, so that early on you can take
> care of this. You will need to add code below in the invocation of
> cpuidle_enter() to verify if the idle state was entered into or not. If
> it was not, then you will need to skip tracing and reflecting of the
> idle state and directly exit the cpuidle loop with a failed status.

Ok.

Thanks !


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Daniel Lezcano]

On 02/12/2014 12:00 PM, Preeti U Murthy wrote:
> Hi Daniel,
>
> Find below a couple of comments.
>
> On 02/11/2014 08:41 PM, Daniel Lezcano wrote:
>> 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 the change the current behavior.
>>
>> Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
>> ---
>>   include/linux/cpuidle.h |    2 ++
>>   kernel/sched/idle.c     |   39 ++++++++++++++++++++-------------------
>>   2 files changed, 22 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 0befaff..a8d5bd3 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -175,6 +175,8 @@ static inline int cpuidle_enable_device(struct cpuidle_device *dev)
>>   {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
>> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
>> index 6963822..8b10891 100644
>> --- a/kernel/sched/idle.c
>> +++ b/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,17 +75,26 @@ static int cpuidle_idle_call(void)
>>   	struct cpuidle_driver *drv = cpuidle_get_cpu_driver(dev);
>>   	int next_state, entered_state;
>>
>> -	/* ask the governor for the next state */
>> +	stop_critical_timings();
>> +	rcu_idle_enter();
>> +
>> +	/* 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 (next_state < 0)
>> -		return next_state;
>> +	if (next_state < 0) {
>> +		arch_cpu_idle();
>> +		goto out;
>> +	}
>>
>>   	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;
>> +		goto out;
>>   	}
>>
>>   	trace_cpu_idle_rcuidle(next_state, dev->cpu);
>> @@ -97,15 +105,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
>> @@ -138,14 +146,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();
>>   				}
>
> Is this really necessary? It seems better to let the cpuidle_idle_loop()
> to handle the cpuidle specific tasks and the generic idle loop to handle
> the peripheral functions like stop_critical_timings(), rcu_idle_enter()
> and their counterparts. I am unable to see what we are gaining by doing
> this.
>
> Besides, cpuidle_idle_call() is expected to just call into the cpuidle
> governor and driver. What I would have expected this patchset to do is
> simply move this call under kernel/sched like you have done in your
> first two patches so as to gain the benefit of using the parameters that
> the scheduler keeps track of to make better cpuidle state entry decisions.
>
> But going one step too far by moving the other idle handling functions
> into it would result in some confusion and add more code than it is
> meant to handle. This will avoid having to add  comments in the
> cpuidle_idle_call() function as currently being done in Patch[5/5], to
> clarify what each function is meant to do.
>
> So IMO, Patches[1/5] and [2/5] by themselves are sufficient to increase
> the proximity between scheduler and cpuidle.

May be patches[1/5] and [2/5] are enough to increase the proximity but 
the next 3 patches do clearly simplify the code readability.

 From my POV, the conditions in the idle loop are much more confusing 
than making a linear code in cpuidle_idle_call.

What I agree on is the confusion around the cpuidle_idle_call function 
name which does not reflect what it does after these patches and maybe 
we should rename 'cpuidle_idle_call' to 'idle()' (or whatever).


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