Re: [PATCH V2] cpuidle/governors: Fix logic in selection of idlestates

[Daniel Lezcano]

On 01/24/2014 11:21 AM, Preeti U Murthy wrote:
> On 01/24/2014 02:38 PM, Daniel Lezcano wrote:
> > On 01/23/2014 12:15 PM, Preeti U Murthy wrote:
> > > Hi Daniel,
> > >
> > > Thank you for the review.

[ ... ]

> > > ---
> > >    drivers/cpuidle/cpuidle.c          |   15 +++++
> > >    drivers/cpuidle/governors/ladder.c |  101
> > > ++++++++++++++++++++++++++----------
> > >    drivers/cpuidle/governors/menu.c   |    7 +-
> > >    3 files changed, 90 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> > > index a55e68f..19d17e8 100644
> > > --- a/drivers/cpuidle/cpuidle.c
> > > +++ b/drivers/cpuidle/cpuidle.c
> > > @@ -131,8 +131,9 @@ int cpuidle_idle_call(void)
> > >
> > >        /* ask the governor for the next state */
> > >        next_state = cpuidle_curr_governor->select(drv, dev);
> > > +
> > > +    dev->last_residency = 0;
> > >        if (need_resched()) {
> >
> > What about if (need_resched() || next_state < 0) ?
>
> Hmm.. I feel we need to distinguish between the need_resched() scenario
> and the scenario when no idle state was suitable through the trace
> points at-least.

Well, I don't think so as soon as we don't care about the return value 
of cpuidle_idle_call in both cases.

The traces are following a specific format. That is if the state is -1 
(PWR_EVENT_EXIT), it means exiting the current idle state.

The idlestat tool [1] is using this traces to open - close transitions.

IMO, if the cpu is not entering idle, it should just exit without any 
idle traces.

This portion of code is a bit confusing because it is introduced by the 
menu governor updates post-poned when entering the next idle state (not 
exiting the current idle state with good reasons).

  -- Daniel

[1] http://git.linaro.org/power/idlestat.git

> This could help while debugging when we could find situations where
> there are no tasks to run, yet the cpu is not entering any idle state.
> The traces could help clearly point that no idle state was thought
> suitable by the governor. Of course there are many other means to find
> this out, but this seems rather straightforward. Hence having the
> condition next_state < 0 between trace_cpu_idle*() would be apt IMHO.
>
> Regards
> Preeti U Murthy
>
> > > -        dev->last_residency = 0;
> > >            /* give the governor an opportunity to reflect on the
> > > outcome */
> > >            if (cpuidle_curr_governor->reflect)
> > >                cpuidle_curr_governor->reflect(dev, next_state);
> > > @@ -141,6 +142,16 @@ int cpuidle_idle_call(void)
> > >        }
> > >
> > >        trace_cpu_idle_rcuidle(next_state, dev->cpu);
> > > +    /*
> > > +     * NOTE: The return code should still be success, since the
> > > verdict of
> > > +     * this call is "do not enter any idle state". It is not a failed
> > > call
> > > +     * due to errors.
> > > +     */
> > > +    if (next_state < 0) {
> > > +        entered_state = next_state;
> > > +        local_irq_enable();
> > > +        goto out;
> > > +    }
> > >
> > >        broadcast = !!(drv->states[next_state].flags &
> > > CPUIDLE_FLAG_TIMER_STOP);
> > >
> > > @@ -156,7 +167,7 @@ int cpuidle_idle_call(void)
> > >        if (broadcast)
> > >            clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> > >
> > > -    trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> > > +out:    trace_cpu_idle_rcuidle(PWR_EVENT_EXIT, dev->cpu);
> > >
> > >        /* give the governor an opportunity to reflect on the outcome */
> > >        if (cpuidle_curr_governor->reflect)


--
 <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/