lkml.org 
[lkml]   [2014]   [Feb]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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/

\
 
 \ /
  Last update: 2014-02-12 14:21    [W:1.042 / U:0.436 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site