Re: [PATCHv2 2/3] cpuidle: teo: Remove recent intercepts metric

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

On 11/06/2024 13:24, Christian Loehle wrote:
> The logic for recent intercepts didn't work, there is an underflow
> of the 'recent' value that can be observed during boot already, which
> teo usually doesn't recover from, making the entire logic pointless.
> Furthermore the recent intercepts also were never reset, thus not
> actually being very 'recent'.
> 
> Having underflowed 'recent' values lead to teo always acting as if
> we were in a scenario were expected sleep length based on timers is
> too high and it therefore unnecessarily selecting shallower states.
> 
> Experiments show that the remaining 'intercept' logic is enough to
> quickly react to scenarios in which teo cannot rely on the timer
> expected sleep length.
> 
> See also here:
> https://lore.kernel.org/lkml/0ce2d536-1125-4df8-9a5b-0d5e389cd8af@arm.com/

So the fixes proposed in teo_update() there:

(1) add '&& cpu_data->state_bins[cpu_data->recent_idx[i]].recent' to the 
    if condition to decrement 'cpu_data->state_bins[].recent' or

(2) Set 'cpu_data->state_bins[].recent' = 0 instead of decrement

didn't work?

> Fixes: 77577558f25d ("cpuidle: teo: Rework most recent idle duration values treatment")
> Signed-off-by: Christian Loehle <christian.loehle@arm.com>
> ---
>  drivers/cpuidle/governors/teo.c | 73 ++++++---------------------------
>  1 file changed, 12 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> index d8554c20cf10..cc7df59f488d 100644
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -59,10 +59,6 @@
>   * shallower than the one whose bin is fallen into by the sleep length (these
>   * situations are referred to as "intercepts" below).
>   *
> - * In addition to the metrics described above, the governor counts recent
> - * intercepts (that is, intercepts that have occurred during the last
> - * %NR_RECENT invocations of it for the given CPU) for each bin.
> - *
>   * In order to select an idle state for a CPU, the governor takes the following
>   * steps (modulo the possible latency constraint that must be taken into account
>   * too):
> @@ -81,20 +77,15 @@

 66  * 1. Find the deepest CPU idle state whose target residency does not exceed
 67  *    the current sleep length (the candidate idle state) and compute 3 sums as

s/3/2 ?

[...]

> @@ -320,8 +288,6 @@ static int teo_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
>  	unsigned int tick_intercept_sum = 0;
>  	unsigned int idx_intercept_sum = 0;
>  	unsigned int intercept_sum = 0;
> -	unsigned int idx_recent_sum = 0;
> -	unsigned int recent_sum = 0;
>  	unsigned int idx_hit_sum = 0;
>  	unsigned int hit_sum = 0;
>  	int constraint_idx = 0;

Looks like 'bool alt_intercepts, alt_recent' have to go here already and
not in 3/3.

[...]