Messages in this thread | | | Date | Wed, 12 Feb 2014 13:45:01 +0100 | From | Daniel Lezcano <> | Subject | Re: [PATCH 3/5] idle: Reorganize the idle loop |
| |
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
-- 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/
| |