| Message ID | 1392131491-5265-3-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 > -- 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 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).
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(); }
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(-)