Re: [patch 1/2] cpuidle: use last_state which can reflect the actual state entered

[Kevin Hilman <khilman@deeprootsystems.com>]

"Pallipadi, Venkatesh" <venkatesh.pallipadi@intel.com> writes:

>>Venkatesh Pallipadi <venkatesh.pallipadi@intel.com> writes:
>>
>>> cpuidle accounts the idle time for the C-state it was trying
>>to enter and
>>> not to the actual state that the driver eventually entered.
>>The driver may
>>> select a different state than the one chosen by cpuidle due to
>>> constraints like bus-mastering, etc.
>>>
>>> Change the time acounting code to look at the dev->last_state after
>>> returning from target_state->enter(). Driver can modify
>>dev->last_state
>>> internally, inside the enter routine to reflect the actual C-state
>>> entered.
>>>
>>> Signed-off-by: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
>>>
>>> ---
>>>  drivers/cpuidle/cpuidle.c |    5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> Index: tip/drivers/cpuidle/cpuidle.c
>>> ===================================================================
>>> --- tip.orig/drivers/cpuidle/cpuidle.c        2008-09-23
>>13:52:59.000000000 -0700
>>> +++ tip/drivers/cpuidle/cpuidle.c     2008-09-23
>>14:22:43.000000000 -0700
>>> @@ -71,8 +71,11 @@ static void cpuidle_idle_call(void)
>>>       target_state = &dev->states[next_state];
>>>
>>>       /* enter the state and update stats */
>>> -     dev->last_residency = target_state->enter(dev, target_state);
>>>       dev->last_state = target_state;
>>> +     dev->last_residency = target_state->enter(dev, target_state);
>>> +     if (dev->last_state)
>>> +             target_state = dev->last_state;
>>> +
>>>       target_state->time += (unsigned long long)dev->last_residency;
>>>       target_state->usage++;
>>
>>Under what conditions would the enter hook set dev->last_sate to NULL?
>>Having the check seems to indicate it's possilble.
>
> Sorry about the delayed response.
>
> Yes. I added the check after finding out that last_state can be
> possibly NULL.  That happens when the governor changes while one
> core is in idle and also during CPU offline/online.

OK, thanks for the clarification.

>>A minor nit-pick...  why not explicitly do the accounting using
>>'last_state' like below.  While functionally the same as above, this
>>makes it makes it more explicit when reading the code that the
>>accounting is done using 'last_state' and not 'target_state.'
>>
>
> Yes. It is cleaner. But, we still have to check for last_state being NULL.

Agreed.

I've switched to your version and tested this on OMAP3 and it's
working well.

Tested-by: Kevin Hilman <khilman@deeprootsystems.com>


>>
>>
>>diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>>index 5ce07b5..c1294f5 100644
>>--- a/drivers/cpuidle/cpuidle.c
>>+++ b/drivers/cpuidle/cpuidle.c
>>@@ -67,10 +67,10 @@ static void cpuidle_idle_call(void)
>>        target_state = &dev->states[next_state];
>>
>>        /* enter the state and update stats */
>>-       dev->last_residency = target_state->enter(dev, target_state);
>>        dev->last_state = target_state;
>>-       target_state->time += (unsigned long long)dev->last_residency;
>>-       target_state->usage++;
>>+       dev->last_residency = target_state->enter(dev, target_state);
>>+       dev->last_state->time += (unsigned long
>>long)dev->last_residency;
>>+       dev->last_state->usage++;
>>
>>        /* give the governor an opportunity to reflect on the
>>outcome */
>>        if (cpuidle_curr_governor->reflect)
>>
>
> Thanks,
> Venki