On Thu, Sep 18, 2014 at 10:37:33AM -0700, Paul E. McKenney wrote:

On Thu, Sep 04, 2014 at 11:32:09AM -0400, Nicolas Pitre wrote:

...

From: Daniel Lezcano daniel.lezcano@linaro.org

When the cpu enters idle, it stores the cpuidle state pointer in its struct rq instance which in turn could be used to make a better decision when balancing tasks.

As soon as the cpu exits its idle state, the struct rq reference is cleared.

There are a couple of situations where the idle state pointer could be changed while it is being consulted:

1. For x86/acpi with dynamic c-states, when a laptop switches from battery to AC that could result on removing the deeper idle state. The acpi driver triggers: 'acpi_processor_cst_has_changed' 'cpuidle_pause_and_lock' 'cpuidle_uninstall_idle_handler' 'kick_all_cpus_sync'.

All cpus will exit their idle state and the pointed object will be set to NULL.

2. The cpuidle driver is unloaded. Logically that could happen but not

in practice because the drivers are always compiled in and 95% of them are not coded to unregister themselves. In any case, the unloading code must call 'cpuidle_unregister_device', that calls 'cpuidle_pause_and_lock' leading to 'kick_all_cpus_sync' as mentioned above.

A race can happen if we use the pointer and then one of these two scenarios occurs at the same moment.

In order to be safe, the idle state pointer stored in the rq must be used inside a rcu_read_lock section where we are protected with the 'rcu_barrier' in the 'cpuidle_uninstall_idle_handler' function. The idle_get_state() and idle_put_state() accessors should be used to that effect.

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

---

drivers/cpuidle/cpuidle.c | 6 ++++++ kernel/sched/idle.c | 6 ++++++ kernel/sched/sched.h | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index ee9df5e3f5..530e3055a2 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -225,6 +225,12 @@ void cpuidle_uninstall_idle_handler(void) initialized = 0; kick_all_cpus_sync(); }

• /*

• * Make sure external observers (such as the scheduler)
  

• * are done looking at pointed idle states.
  

• */
  

• rcu_barrier();

Actually, all rcu_barrier() does is to make sure that all previously queued RCU callbacks have been invoked. And given the current implementation, if there are no callbacks queued anywhere in the system, rcu_barrier() is an extended no-op. "Has CPU 0 any callbacks?" "Nope!" "Has CPU 1 any callbacks?" "Nope!" ... "Has CPU nr_cpu_ids-1 any callbacks?" "Nope!" "OK, done!"

This is all done with the current task looking at per-CPU data structures, with no interaction with the scheduler and with no need to actually make those other CPUs do anything.

So what is it that you really need to do here?

A synchronize_sched() will wait for all non-idle online CPUs to pass through the scheduler, where "idle" includes usermode execution in CONFIG_NO_HZ_FULL=y kernels. But it won't wait for CPUs executing in the idle loop.

A synchronize_rcu_tasks() will wait for all non-idle tasks that are currently on a runqueue to do a voluntary context switch. There has been some discussion about extending this to idle tasks, but the current prospective users can live without this. But if you need it, I can push on getting it set up. (Current plans are that synchronize_rcu_tasks() goes into the v3.18 merge window.) And one caveat: There is long latency associated with synchronize_rcu_tasks() by design. Grace periods are measured in seconds.

A stop_cpus() will force a context switch on all CPUs, though it is a rather big hammer.

And I was reminded by the very next email that kick_all_cpus_sync() is another possibility -- it forces an interrupt on all online CPUs, idle or not.

Thanx, Paul

So again, what do you really need?

					Thanx, Paul

...

}

/** diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c index 11e7bc434f..c47fce75e6 100644 --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -147,6 +147,9 @@ use_default: clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu)) goto use_default;

• /* Take note of the planned idle state. */
• idle_set_state(this_rq(), &drv->states[next_state]);
• /*
  • Enter the idle state previously returned by the governor decision.
  • This function will block until an interrupt occurs and will take

@@ -154,6 +157,9 @@ use_default: */ entered_state = cpuidle_enter(drv, dev, next_state);

• /* The cpu is no longer idle or about to enter idle. */
• idle_set_state(this_rq(), NULL);
• if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);

diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index 579712f4e9..aea8baa7a5 100644 --- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -14,6 +14,7 @@ #include "cpuacct.h"

struct rq; +struct cpuidle_state;

extern __read_mostly int scheduler_running;

@@ -636,6 +637,11 @@ struct rq { #ifdef CONFIG_SMP struct llist_head wake_list; #endif

+#ifdef CONFIG_CPU_IDLE

• /* Must be inspected within a rcu lock section */
• struct cpuidle_state *idle_state;

+#endif };

static inline int cpu_of(struct rq *rq) @@ -1180,6 +1186,39 @@ static inline void idle_exit_fair(struct rq *rq) { }

#endif

+#ifdef CONFIG_CPU_IDLE +static inline void idle_set_state(struct rq *rq,

• 		  struct cpuidle_state *idle_state)
  

+{

• rq->idle_state = idle_state;

+}

+static inline struct cpuidle_state *idle_get_state(struct rq *rq) +{

• rcu_read_lock();
• return rq->idle_state;

+}

+static inline void cpuidle_put_state(struct rq *rq) +{

• rcu_read_unlock();

+} +#else +static inline void idle_set_state(struct rq *rq,

• 		  struct cpuidle_state *idle_state)
  

+{ +}

+static inline struct cpuidle_state *idle_get_state(struct rq *rq) +{

• return NULL;

+}

+static inline void cpuidle_put_state(struct rq *rq) +{ +} +#endif

extern void sysrq_sched_debug_show(void); extern void sched_init_granularity(void); extern void update_max_interval(void); -- 1.8.4.108.g55ea5f6

-- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/

On Thu, 18 Sep 2014, Paul E. McKenney wrote:

On Thu, Sep 04, 2014 at 11:32:09AM -0400, Nicolas Pitre wrote:

...

From: Daniel Lezcano daniel.lezcano@linaro.org

When the cpu enters idle, it stores the cpuidle state pointer in its struct rq instance which in turn could be used to make a better decision when balancing tasks.

As soon as the cpu exits its idle state, the struct rq reference is cleared.

There are a couple of situations where the idle state pointer could be changed while it is being consulted:

1. For x86/acpi with dynamic c-states, when a laptop switches from battery to AC that could result on removing the deeper idle state. The acpi driver triggers: 'acpi_processor_cst_has_changed' 'cpuidle_pause_and_lock' 'cpuidle_uninstall_idle_handler' 'kick_all_cpus_sync'.

All cpus will exit their idle state and the pointed object will be set to NULL.

2. The cpuidle driver is unloaded. Logically that could happen but not

in practice because the drivers are always compiled in and 95% of them are not coded to unregister themselves. In any case, the unloading code must call 'cpuidle_unregister_device', that calls 'cpuidle_pause_and_lock' leading to 'kick_all_cpus_sync' as mentioned above.

A race can happen if we use the pointer and then one of these two scenarios occurs at the same moment.

In order to be safe, the idle state pointer stored in the rq must be used inside a rcu_read_lock section where we are protected with the 'rcu_barrier' in the 'cpuidle_uninstall_idle_handler' function. The idle_get_state() and idle_put_state() accessors should be used to that effect.

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

---

drivers/cpuidle/cpuidle.c | 6 ++++++ kernel/sched/idle.c | 6 ++++++ kernel/sched/sched.h | 39 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 51 insertions(+)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index ee9df5e3f5..530e3055a2 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -225,6 +225,12 @@ void cpuidle_uninstall_idle_handler(void) initialized = 0; kick_all_cpus_sync(); }

• /*

• * Make sure external observers (such as the scheduler)
  

• * are done looking at pointed idle states.
  

• */
  

• rcu_barrier();

Actually, all rcu_barrier() does is to make sure that all previously queued RCU callbacks have been invoked. And given the current implementation, if there are no callbacks queued anywhere in the system, rcu_barrier() is an extended no-op. "Has CPU 0 any callbacks?" "Nope!" "Has CPU 1 any callbacks?" "Nope!" ... "Has CPU nr_cpu_ids-1 any callbacks?" "Nope!" "OK, done!"

This is all done with the current task looking at per-CPU data structures, with no interaction with the scheduler and with no need to actually make those other CPUs do anything.

So what is it that you really need to do here?

In short, we don't want the cpufreq data to go away (see the 2 scenarios above) while the scheduler is looking at it. The scheduler uses the provided accessors (see patch 2/2) so we can put any protection mechanism we want in them. A simple spinlock could do just as well which should be good enough.

Nicolas

On Fri, Sep 19, 2014 at 01:17:15AM +0200, Peter Zijlstra wrote:

On Thu, Sep 18, 2014 at 02:32:25PM -0400, Nicolas Pitre wrote:

...

On Thu, 18 Sep 2014, Paul E. McKenney wrote:

...

...

So what is it that you really need to do here?

In short, we don't want the cpufreq data to go away (see the 2 scenarios above) while the scheduler is looking at it. The scheduler uses the provided accessors (see patch 2/2) so we can put any protection mechanism we want in them. A simple spinlock could do just as well which should be good enough.

rq->lock disables interrupts so on that something like kick_all_cpus_sync() will guarantee what you need -- wake_up_all_idle_cpus() will not.

Something like so then?

--- Subject: sched: let the scheduler see CPU idle states From: Daniel Lezcano daniel.lezcano@linaro.org Date: Thu, 04 Sep 2014 11:32:09 -0400

When the cpu enters idle, it stores the cpuidle state pointer in its struct rq instance which in turn could be used to make a better decision when balancing tasks.

As soon as the cpu exits its idle state, the struct rq reference is cleared.

There are a couple of situations where the idle state pointer could be changed while it is being consulted:

1. For x86/acpi with dynamic c-states, when a laptop switches from battery to AC that could result on removing the deeper idle state. The acpi driver triggers: 'acpi_processor_cst_has_changed' 'cpuidle_pause_and_lock' 'cpuidle_uninstall_idle_handler' 'kick_all_cpus_sync'.

All cpus will exit their idle state and the pointed object will be set to NULL.

2. The cpuidle driver is unloaded. Logically that could happen but not in practice because the drivers are always compiled in and 95% of them are not coded to unregister themselves. In any case, the unloading code must call 'cpuidle_unregister_device', that calls 'cpuidle_pause_and_lock' leading to 'kick_all_cpus_sync' as mentioned above.

A race can happen if we use the pointer and then one of these two scenarios occurs at the same moment.

In order to be safe, the idle state pointer stored in the rq must be used inside a rcu_read_lock section where we are protected with the 'rcu_barrier' in the 'cpuidle_uninstall_idle_handler' function. The idle_get_state() and idle_put_state() accessors should be used to that effect.

Cc: "Rafael J. Wysocki" rjw@rjwysocki.net Cc: Ingo Molnar mingo@redhat.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Nicolas Pitre nico@linaro.org --- drivers/cpuidle/cpuidle.c | 6 ++++++ kernel/sched/idle.c | 6 ++++++ kernel/sched/sched.h | 29 +++++++++++++++++++++++++++++ 3 files changed, 41 insertions(+)

--- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -225,6 +225,12 @@ void cpuidle_uninstall_idle_handler(void initialized = 0; wake_up_all_idle_cpus(); } + + /* + * Make sure external observers (such as the scheduler) + * are done looking at pointed idle states. + */ + kick_all_cpus_sync(); }

/** --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -147,6 +147,9 @@ static void cpuidle_idle_call(void) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu)) goto use_default;

+ /* Take note of the planned idle state. */ + idle_set_state(this_rq(), &drv->states[next_state]); + /* * Enter the idle state previously returned by the governor decision. * This function will block until an interrupt occurs and will take @@ -154,6 +157,9 @@ static void cpuidle_idle_call(void) */ entered_state = cpuidle_enter(drv, dev, next_state);

+ /* The cpu is no longer idle or about to enter idle. */ + idle_set_state(this_rq(), NULL); + if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);

--- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -14,6 +14,7 @@ #include "cpuacct.h"

struct rq; +struct cpuidle_state;

/* task_struct::on_rq states: */ #define TASK_ON_RQ_QUEUED 1 @@ -640,6 +641,11 @@ struct rq { #ifdef CONFIG_SMP struct llist_head wake_list; #endif + +#ifdef CONFIG_CPU_IDLE + /* Must be inspected within a rcu lock section */ + struct cpuidle_state *idle_state; +#endif };

static inline int cpu_of(struct rq *rq) @@ -1193,6 +1199,29 @@ static inline void idle_exit_fair(struct

#endif

+#ifdef CONFIG_CPU_IDLE +static inline void idle_set_state(struct rq *rq, + struct cpuidle_state *idle_state) +{ + rq->idle_state = idle_state; +} + +static inline struct cpuidle_state *idle_get_state(struct rq *rq) +{ + return rq->idle_state; +} +#else +static inline void idle_set_state(struct rq *rq, + struct cpuidle_state *idle_state) +{ +} + +static inline struct cpuidle_state *idle_get_state(struct rq *rq) +{ + return NULL; +} +#endif + extern void sysrq_sched_debug_show(void); extern void sched_init_granularity(void); extern void update_max_interval(void);

On Thu, Sep 04, 2014 at 11:32:10AM -0400, Nicolas Pitre wrote:

The code in find_idlest_cpu() looks for the CPU with the smallest load. However, if multiple CPUs are idle, the first idle CPU is selected irrespective of the depth of its idle state.

Among the idle CPUs we should pick the one with with the shallowest idle state, or the latest to have gone idle if all idle CPUs are in the same state. The later applies even when cpuidle is configured out.

This patch doesn't cover the following issues:

• The idle exit latency of a CPU might be larger than the time needed to migrate the waking task to an already running CPU with sufficient capacity, and therefore performance would benefit from task packing in such case (in most cases task packing is about power saving).

• Some idle states have a non negligible and non abortable entry latency which needs to run to completion before the exit latency can start. A concurrent patch series is making this info available to the cpuidle core. Once available, the entry latency with the idle timestamp could determine when the exit latency may be effective.

Those issues will be handled in due course. In the mean time, what is implemented here should improve things already compared to the current state of affairs.

Based on an initial patch from Daniel Lezcano.

Signed-off-by: Nicolas Pitre nico@linaro.org

kernel/sched/fair.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index bfa3c86d0d..416329e1a6 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -23,6 +23,7 @@ #include <linux/latencytop.h> #include <linux/sched.h> #include <linux/cpumask.h> +#include <linux/cpuidle.h> #include <linux/slab.h> #include <linux/profile.h> #include <linux/interrupt.h> @@ -4428,20 +4429,48 @@ static int find_idlest_cpu(struct sched_group *group, struct task_struct *p, int this_cpu)

Ah, now I see, you use it in find_idlest_cpu(), that does not indeed hold rq->lock, but it does already hold rcu_read_lock(), so in that regard sync_rcu() should be the right primitive.

I suppose we want the same kind of logic in select_idle_sibling() and that too already has rcu_read_lock().

So I'll replace the kick_all_cpus_sync() with sync_rcu() and add a WARN_ON(!rcu_read_lock_held()) to idle_get_state(), like the below.

I however do think we need a few word on why we don't need rcu_assign_pointer() and rcu_dereference() for rq->idle_state -- and I do indeed think we do not because the idle state data is static.

--- Subject: sched: let the scheduler see CPU idle states From: Daniel Lezcano daniel.lezcano@linaro.org Date: Thu, 04 Sep 2014 11:32:09 -0400

When the cpu enters idle, it stores the cpuidle state pointer in its struct rq instance which in turn could be used to make a better decision when balancing tasks.

As soon as the cpu exits its idle state, the struct rq reference is cleared.

There are a couple of situations where the idle state pointer could be changed while it is being consulted:

1. For x86/acpi with dynamic c-states, when a laptop switches from battery to AC that could result on removing the deeper idle state. The acpi driver triggers: 'acpi_processor_cst_has_changed' 'cpuidle_pause_and_lock' 'cpuidle_uninstall_idle_handler' 'kick_all_cpus_sync'.

All cpus will exit their idle state and the pointed object will be set to NULL.

2. The cpuidle driver is unloaded. Logically that could happen but not in practice because the drivers are always compiled in and 95% of them are not coded to unregister themselves. In any case, the unloading code must call 'cpuidle_unregister_device', that calls 'cpuidle_pause_and_lock' leading to 'kick_all_cpus_sync' as mentioned above.

A race can happen if we use the pointer and then one of these two scenarios occurs at the same moment.

In order to be safe, the idle state pointer stored in the rq must be used inside a rcu_read_lock section where we are protected with the 'rcu_barrier' in the 'cpuidle_uninstall_idle_handler' function. The idle_get_state() and idle_put_state() accessors should be used to that effect.

Cc: "Rafael J. Wysocki" rjw@rjwysocki.net Cc: Ingo Molnar mingo@redhat.com Signed-off-by: Daniel Lezcano daniel.lezcano@linaro.org Signed-off-by: Nicolas Pitre nico@linaro.org --- drivers/cpuidle/cpuidle.c | 6 ++++++ kernel/sched/idle.c | 6 ++++++ kernel/sched/sched.h | 30 ++++++++++++++++++++++++++++++ 3 files changed, 42 insertions(+)

--- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -225,6 +225,12 @@ void cpuidle_uninstall_idle_handler(void initialized = 0; wake_up_all_idle_cpus(); } + + /* + * Make sure external observers (such as the scheduler) + * are done looking at pointed idle states. + */ + synchronize_rcu(); }

/** --- a/kernel/sched/idle.c +++ b/kernel/sched/idle.c @@ -147,6 +147,9 @@ static void cpuidle_idle_call(void) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu)) goto use_default;

+ /* Take note of the planned idle state. */ + idle_set_state(this_rq(), &drv->states[next_state]); + /* * Enter the idle state previously returned by the governor decision. * This function will block until an interrupt occurs and will take @@ -154,6 +157,9 @@ static void cpuidle_idle_call(void) */ entered_state = cpuidle_enter(drv, dev, next_state);

+ /* The cpu is no longer idle or about to enter idle. */ + idle_set_state(this_rq(), NULL); + if (broadcast) clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);

--- a/kernel/sched/sched.h +++ b/kernel/sched/sched.h @@ -14,6 +14,7 @@ #include "cpuacct.h"

struct rq; +struct cpuidle_state;

/* task_struct::on_rq states: */ #define TASK_ON_RQ_QUEUED 1 @@ -640,6 +641,11 @@ struct rq { #ifdef CONFIG_SMP struct llist_head wake_list; #endif + +#ifdef CONFIG_CPU_IDLE + /* Must be inspected within a rcu lock section */ + struct cpuidle_state *idle_state; +#endif };

static inline int cpu_of(struct rq *rq) @@ -1193,6 +1199,30 @@ static inline void idle_exit_fair(struct

#endif

+#ifdef CONFIG_CPU_IDLE +static inline void idle_set_state(struct rq *rq, + struct cpuidle_state *idle_state) +{ + rq->idle_state = idle_state; +} + +static inline struct cpuidle_state *idle_get_state(struct rq *rq) +{ + WARN_ON(!rcu_read_lock_held()); + return rq->idle_state; +} +#else +static inline void idle_set_state(struct rq *rq, + struct cpuidle_state *idle_state) +{ +} + +static inline struct cpuidle_state *idle_get_state(struct rq *rq) +{ + return NULL; +} +#endif + extern void sysrq_sched_debug_show(void); extern void sched_init_granularity(void); extern void update_max_interval(void);

On 2014/9/4 23:32, Nicolas Pitre wrote:

The code in find_idlest_cpu() looks for the CPU with the smallest load. However, if multiple CPUs are idle, the first idle CPU is selected irrespective of the depth of its idle state.

Among the idle CPUs we should pick the one with with the shallowest idle state, or the latest to have gone idle if all idle CPUs are in the same state. The later applies even when cpuidle is configured out.

This patch doesn't cover the following issues:

• The idle exit latency of a CPU might be larger than the time needed to migrate the waking task to an already running CPU with sufficient capacity, and therefore performance would benefit from task packing in such case (in most cases task packing is about power saving).

• Some idle states have a non negligible and non abortable entry latency which needs to run to completion before the exit latency can start. A concurrent patch series is making this info available to the cpuidle core. Once available, the entry latency with the idle timestamp could determine when the exit latency may be effective.

Those issues will be handled in due course. In the mean time, what is implemented here should improve things already compared to the current state of affairs.

Based on an initial patch from Daniel Lezcano.

Signed-off-by: Nicolas Pitre nico@linaro.org

kernel/sched/fair.c | 43 ++++++++++++++++++++++++++++++++++++------- 1 file changed, 36 insertions(+), 7 deletions(-)

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c index bfa3c86d0d..416329e1a6 100644 --- a/kernel/sched/fair.c +++ b/kernel/sched/fair.c @@ -23,6 +23,7 @@ #include <linux/latencytop.h> #include <linux/sched.h> #include <linux/cpumask.h> +#include <linux/cpuidle.h> #include <linux/slab.h> #include <linux/profile.h> #include <linux/interrupt.h> @@ -4428,20 +4429,48 @@ static int find_idlest_cpu(struct sched_group *group, struct task_struct *p, int this_cpu) { unsigned long load, min_load = ULONG_MAX;

• int idlest = -1;

• unsigned int min_exit_latency = UINT_MAX;
• u64 latest_idle_timestamp = 0;
• int least_loaded_cpu = this_cpu;
• int shallowest_idle_cpu = -1; int i;

/* Traverse only the allowed CPUs */ for_each_cpu_and(i, sched_group_cpus(group), tsk_cpus_allowed(p)) {

• load = weighted_cpuload(i);
  

• if (load < min_load || (load == min_load && i == this_cpu)) {
  

• 	min_load = load;
  

• 	idlest = i;
  

• if (idle_cpu(i)) {
  

• 	struct rq *rq = cpu_rq(i);
  

• 	struct cpuidle_state *idle = idle_get_state(rq);
  

• 	if (idle && idle->exit_latency < min_exit_latency) {
  

• 		/*
  

• 		 * We give priority to a CPU whose idle state
  

• 		 * has the smallest exit latency irrespective
  

• 		 * of any idle timestamp.
  

• 		 */
  

• 		min_exit_latency = idle->exit_latency;
  

• 		latest_idle_timestamp = rq->idle_stamp;
  

• 		shallowest_idle_cpu = i;
  

• 	} else if ((!idle || idle->exit_latency == min_exit_latency) &&
  

• 		   rq->idle_stamp > latest_idle_timestamp) {
  

• 		/*
  

• 		 * If equal or no active idle state, then
  

• 		 * the most recently idled CPU might have
  

• 		 * a warmer cache.
  

• 		 */
  

• 		latest_idle_timestamp = rq->idle_stamp;
  

• 		shallowest_idle_cpu = i;
  

• 	}
  

• 	cpuidle_put_state(rq);
  

• } else {
  

I think we needn't test no idle cpus after find an idle cpu. And what about this? } else if (shallowest_idle_cpu == -1) {

• 	load = weighted_cpuload(i);
  

• 	if (load < min_load ||
  

• 	    (load == min_load && i == this_cpu)) {
  

• 		min_load = load;
  

• 		least_loaded_cpu = i;
  

• 	}
  

  } }

• return idlest;

• return shallowest_idle_cpu != -1 ? shallowest_idle_cpu : least_loaded_cpu;

} /*