Re: [PATCH v2 2/3] cpuidle: allow setting deepest idle

Jacob Pan
2016-11-15T16:50:41Z

On Mon, 14 Nov 2016 22:42:03 +0100
"Rafael J. Wysocki" <rjw@rjwysocki.net> wrote:

> > diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> > index cb6442f..9e80f32 100644
> > --- a/kernel/sched/idle.c
> > +++ b/kernel/sched/idle.c
> > @@ -173,6 +173,9 @@ static void cpuidle_idle_call(void)
> >
> > next_state = cpuidle_find_deepest_state(drv, dev);
> > call_cpuidle(drv, dev, next_state);
> > + } else if (dev->use_deepest_state) {
> > + next_state = cpuidle_find_deepest_state(drv, dev);
> > + call_cpuidle(drv, dev, next_state);
> > } else {
> > /*
> > * Ask the cpuidle framework to choose a
> > convenient idle state.
>
> I would arrange the code slightly differently here:
>
> if (idle_should_freeze() || dev->use_deepest_state) {
> if (idle_should_freeze()) {
> entered_state = cpuidle_enter_freeze(drv,
> dev); if (entered_state > 0) {
> local_irq_enable();
> goto exit_idle;
> }
> }
>
> next_state = cpuidle_find_deepest_state(drv, dev);
> call_cpuidle(drv, dev, next_state);
> } else {
>
>
> This way you'd avoid the ugly code duplication and the extra
> dev->use_deepest_state branch in the most frequent case. I guess you
> could take the unlikely() thing away from idle_should_freeze() and
> use it directly here too.
Sounds good. Will change in the next version.