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@xxxxxxxxxx>
> > ---
> >   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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/