Received-SPF: pass (google.com: domain of linux-kernel+bounces-207978-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240606090050.327614-1-christian.loehle@arm.com>
 <20240606090050.327614-2-christian.loehle@arm.com> <20240609224701.pc6om2o5ep6btywe@airbuntu>
In-Reply-To: <20240609224701.pc6om2o5ep6btywe@airbuntu>
From: Ulf Hansson <ulf.hansson@linaro.org>
Date: Mon, 10 Jun 2024 11:57:55 +0200
Message-ID: <CAPDyKFoHc6Zcg8i-y3HxUp0=S_N2GhmKcT0w2e-NkzR498mp2Q@mail.gmail.com>
Subject: Re: [PATCH 1/6] cpuidle: teo: Increase util-threshold
To: Qais Yousef <qyousef@layalina.io>, Christian Loehle <christian.loehle@arm.com>
Cc: linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, rafael@kernel.org, 
	vincent.guittot@linaro.org, peterz@infradead.org, daniel.lezcano@linaro.org, 
	anna-maria@linutronix.de, kajetan.puchalski@arm.com, lukasz.luba@arm.com, 
	dietmar.eggemann@arm.com
Content-Type: text/plain; charset="UTF-8"

On Mon, 10 Jun 2024 at 00:47, Qais Yousef <qyousef@layalina.io> wrote:
>
> On 06/06/24 10:00, Christian Loehle wrote:
> > Increase the util-threshold by a lot as it was low enough for some
> > minor load to always be active, especially on smaller CPUs.
> >
> > For small cap CPUs (Pixel6) the util threshold is as low as 1.
> > For CPUs of capacity <64 it is 0. So ensure it is at a minimum, too.
> >
> > Fixes: 9ce0f7c4bc64 ("cpuidle: teo: Introduce util-awareness")
> > Reported-by: Qais Yousef <qyousef@layalina.io>
> > Reported-by: Vincent Guittot <vincent.guittot@linaro.org>
> > Suggested-by: Kajetan Puchalski <kajetan.puchalski@arm.com>
> > Signed-off-by: Christian Loehle <christian.loehle@arm.com>
> > ---
> >  drivers/cpuidle/governors/teo.c | 11 +++++------
> >  1 file changed, 5 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> > index 7244f71c59c5..45f43e2ee02d 100644
> > --- a/drivers/cpuidle/governors/teo.c
> > +++ b/drivers/cpuidle/governors/teo.c
> > @@ -146,13 +146,11 @@
> >   * The number of bits to shift the CPU's capacity by in order to determine
> >   * the utilized threshold.
> >   *
> > - * 6 was chosen based on testing as the number that achieved the best balance
> > - * of power and performance on average.
> > - *
> >   * The resulting threshold is high enough to not be triggered by background
> > - * noise and low enough to react quickly when activity starts to ramp up.
> > + * noise.
> >   */
> > -#define UTIL_THRESHOLD_SHIFT 6
> > +#define UTIL_THRESHOLD_SHIFT 2
> > +#define UTIL_THRESHOLD_MIN 50
> >
> >  /*
> >   * The PULSE value is added to metrics when they grow and the DECAY_SHIFT value
> > @@ -671,7 +669,8 @@ static int teo_enable_device(struct cpuidle_driver *drv,
> >       int i;
> >
> >       memset(cpu_data, 0, sizeof(*cpu_data));
> > -     cpu_data->util_threshold = max_capacity >> UTIL_THRESHOLD_SHIFT;
> > +     cpu_data->util_threshold = max(UTIL_THRESHOLD_MIN,
> > +                             max_capacity >> UTIL_THRESHOLD_SHIFT);
>
> Thanks for trying to fix this. But I am afraid this is not a solution. There's
> no magic number that can truly work here - we tried. As I tried to explain
> before, a higher util value doesn't mean long idle time is unlikely. And
> blocked load can cause problems where a decay can take too long.
>
> We are following up with the suggestions I have thrown back then and we'll
> share results if anything actually works.
>
> For now, I think a revert is more appropriate. There was some perf benefit, but
> the power regressions were bad and there's no threshold value that actually
> works. The thresholding concept itself is incorrect and flawed - it seemed the
> correct thing back then, yes. But in a hindsight now it doesn't work.
>

For the record, I fully agree with the above. A revert seems to be the
best option in my opinion too.

Besides for the above reasons; when using cpuidle-psci with PSCI OSI
mode, the approach leads to disabling *all* of cluster's idle-states
too, as those aren't even visible for the teo governor. I am sure,
that was not the intent with commit 9ce0f7c4bc64.

[...]

Kind regards
Uffe