[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <06231a07-c470-4156-bada-41b346b280cb@arm.com>
Date: Wed, 26 Jun 2024 12:44:01 +0200
From: Dietmar Eggemann <dietmar.eggemann@....com>
To: Christian Loehle <christian.loehle@....com>, linux-pm@...r.kernel.org,
linux-kernel@...r.kernel.org, rafael@...nel.org
Cc: vincent.guittot@...aro.org, qyousef@...alina.io, peterz@...radead.org,
daniel.lezcano@...aro.org, ulf.hansson@...aro.org, anna-maria@...utronix.de,
kajetan.puchalski@....com, lukasz.luba@....com
Subject: Re: [PATCHv2 2/3] cpuidle: teo: Remove recent intercepts metric
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@....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.
[...]
Powered by blists - more mailing lists