>-----Original Message----- >From: Kevin Hilman [mailto:khilman@xxxxxxxxxxxxxxxxxxx] >Sent: Wednesday, October 01, 2008 4:09 AM >To: Pallipadi, Venkatesh >Cc: lenb@xxxxxxxxxx; linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx >Subject: Re: [patch 1/2] cpuidle: use last_state >which can reflect the actual state entered > >Venkatesh Pallipadi <venkatesh.pallipadi@xxxxxxxxx> 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@xxxxxxxxx> >> >> --- >> 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. >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. > > >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 _______________________________________________ linux-pm mailing list linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/linux-pm
