[2/3] cpuidle ladder: Better idle duration measurement without using CPUIDLE_FLAG_TIME_INVALID
diff mbox

Message ID 83e27d4a9f387aa9781de927ea7f436f388de7bd.1418712225.git.len.brown@intel.com
State Accepted
Headers show

Commit Message

Len Brown Dec. 16, 2014, 6:52 a.m. UTC
From: Len Brown <len.brown@intel.com>

When the ladder governor sees the CPUIDLE_FLAG_TIME_INVALID flag,
it unconditionally causes a state promotion by setting last_residency
to a number higher than the state's promotion_time:

last_residency = last_state->threshold.promotion_time + 1

It does this for fear that cpuidle_get_last_residency()
will be in-accurate, because cpuidle_enter_state() invoked
a state with CPUIDLE_FLAG_TIME_INVALID.

But the only state with CPUIDLE_FLAG_TIME_INVALID is
acpi_safe_halt(), which may return well after its actual
idle duration because it enables interrupts, so cpuidle_enter_state()
also measures interrupt service time.

So what?  In ladder, a huge invalid last_residency has exactly
the same effect as the current code -- it unconditionally
causes a state promotion.

In the case where the idle residency plus measured interrupt
handling time is less than the state's demotion_time -- we should
use that timestamp to give ladder a chance to demote, rather than
unconditionally promoting.

This can be done by simply ignoring the CPUIDLE_FLAG_TIME_INVALID,
and using the "invalid" time, as it is either equal to what we are
doing today, or better.

Signed-off-by: Len Brown <len.brown@intel.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
---
 drivers/cpuidle/governors/ladder.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

Comments

Daniel Lezcano Dec. 16, 2014, 11:07 a.m. UTC | #1
On 12/16/2014 07:52 AM, Len Brown wrote:
> From: Len Brown <len.brown@intel.com>
>
> When the ladder governor sees the CPUIDLE_FLAG_TIME_INVALID flag,
> it unconditionally causes a state promotion by setting last_residency
> to a number higher than the state's promotion_time:
>
> last_residency = last_state->threshold.promotion_time + 1
>
> It does this for fear that cpuidle_get_last_residency()
> will be in-accurate, because cpuidle_enter_state() invoked
> a state with CPUIDLE_FLAG_TIME_INVALID.
>
> But the only state with CPUIDLE_FLAG_TIME_INVALID is
> acpi_safe_halt(), which may return well after its actual
> idle duration because it enables interrupts, so cpuidle_enter_state()
> also measures interrupt service time.
>
> So what?  In ladder, a huge invalid last_residency has exactly
> the same effect as the current code -- it unconditionally
> causes a state promotion.
>
> In the case where the idle residency plus measured interrupt
> handling time is less than the state's demotion_time -- we should
> use that timestamp to give ladder a chance to demote, rather than
> unconditionally promoting.
>
> This can be done by simply ignoring the CPUIDLE_FLAG_TIME_INVALID,
> and using the "invalid" time, as it is either equal to what we are
> doing today, or better.
>
> Signed-off-by: Len Brown <len.brown@intel.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>

Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>

a dumb question: is someone still using the ladder governor nowadays ?

> ---
>   drivers/cpuidle/governors/ladder.c | 7 +------
>   1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
> index 37263d9..401c010 100644
> --- a/drivers/cpuidle/governors/ladder.c
> +++ b/drivers/cpuidle/governors/ladder.c
> @@ -79,12 +79,7 @@ static int ladder_select_state(struct cpuidle_driver *drv,
>
>   	last_state = &ldev->states[last_idx];
>
> -	if (!(drv->states[last_idx].flags & CPUIDLE_FLAG_TIME_INVALID)) {
> -		last_residency = cpuidle_get_last_residency(dev) - \
> -					 drv->states[last_idx].exit_latency;
> -	}
> -	else
> -		last_residency = last_state->threshold.promotion_time + 1;
> +	last_residency = cpuidle_get_last_residency(dev) - drv->states[last_idx].exit_latency;
>
>   	/* consider promotion */
>   	if (last_idx < drv->state_count - 1 &&
>

Patch
diff mbox

diff --git a/drivers/cpuidle/governors/ladder.c b/drivers/cpuidle/governors/ladder.c
index 37263d9..401c010 100644
--- a/drivers/cpuidle/governors/ladder.c
+++ b/drivers/cpuidle/governors/ladder.c
@@ -79,12 +79,7 @@  static int ladder_select_state(struct cpuidle_driver *drv,
 
 	last_state = &ldev->states[last_idx];
 
-	if (!(drv->states[last_idx].flags & CPUIDLE_FLAG_TIME_INVALID)) {
-		last_residency = cpuidle_get_last_residency(dev) - \
-					 drv->states[last_idx].exit_latency;
-	}
-	else
-		last_residency = last_state->threshold.promotion_time + 1;
+	last_residency = cpuidle_get_last_residency(dev) - drv->states[last_idx].exit_latency;
 
 	/* consider promotion */
 	if (last_idx < drv->state_count - 1 &&