linux-kernel - Re: [PATCH V2] cpuidle/governors: Fix logic in selection of idle states

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <52E78F35.2090106@linux.vnet.ibm.com>
Date:	Tue, 28 Jan 2014 16:36:29 +0530
From:	Preeti U Murthy <preeti@...ux.vnet.ibm.com>
To:	Daniel Lezcano <daniel.lezcano@...aro.org>
CC:	paulmck@...ux.vnet.ibm.com, linux-pm@...r.kernel.org,
	rjw@...ysocki.net, linux-kernel@...r.kernel.org,
	srivatsa.bhat@...ux.vnet.ibm.com, linuxppc-dev@...ts.ozlabs.org,
	tuukka.tikkanen@...aro.org
Subject: Re: [PATCH V2] cpuidle/governors: Fix logic in selection of idle
 states

Hi Daniel,

On 01/28/2014 02:16 PM, Daniel Lezcano wrote:
> 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.

Yes I see your point here.
> 
> 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).

I am sorry but I don't understand this part. Which is the portion of the
code you refer to here? Also can you please elaborate on the above
statement?

Thanks

Regards
Preeti U Murthy
> 
>   -- 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)
>>
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/