linux-kernel - Re: [PATCH] cpuidle: teo: fix underflow of recent intercepts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <4716b8aa-dfc8-4784-9f5d-6de27c612f47@arm.com>
Date: Mon, 20 May 2024 15:30:02 +0100
From: Christian Loehle <christian.loehle@....com>
To: "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
 linux-pm@...r.kernel.org, "Rafael J. Wysocki" <rafael@...nel.org>,
 Daniel Lezcano <daniel.lezcano@...aro.org>
Cc: Qais Yousef <qyousef@...alina.io>, kajetan.puchalski@....com,
 Anna-Maria Behnsen <anna-maria@...utronix.de>
Subject: Re: [PATCH] cpuidle: teo: fix underflow of recent intercepts

On 5/19/24 22:03, Christian Loehle wrote:
> The recent counter of each cpuidle state bin reflects the number of
> recent intercepts. It's decremented and incremented accordingly.
> The decrement was never checked for 0, therefore underflowing into a
> value teo cannot easily recover from.
> 
> The underflow lead to deeper idle states being skipped because teo
> assumed interception was likely and it preferring shallower states.
> 
> Fixes: 77577558f25d ("cpuidle: teo: Rework most recent idle duration values treatment")
> Signed-off-by: Christian Loehle <christian.loehle@....com>
> ---
>  drivers/cpuidle/governors/teo.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
> index 7244f71c59c5..42fb2771e35d 100644
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -290,7 +290,8 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
>  	if (cpu_data->next_recent_idx >= NR_RECENT)
>  		cpu_data->next_recent_idx = 0;
>  
> -	if (cpu_data->recent_idx[i] >= 0)
> +	if (cpu_data->recent_idx[i] >= 0 &&
> +			cpu_data->state_bins[cpu_data->recent_idx[i]].recent)
>  		cpu_data->state_bins[cpu_data->recent_idx[i]].recent--;
>  
>  	/*

Anyway, since I'm not sure when I will get around to finishing up a
fully-detailed report.
Rafael, unless I'm missing something, recent is only ever cycled through
the bins and never actually reset to 0, is that missing somewhere?

Maybe like this if my suspicion is correct?
Haven't tested it yet.

[PATCH] cpuidle: teo: Reset recent when cycling bins

recent intercepts value was never actually reset, only
decremented. This patch resets the value when cycling
to a new recent intercept bin.

Fixes: 77577558f25d ("cpuidle: teo: Rework most recent idle duration values treatment")
Signed-off-by: Christian Loehle <christian.loehle@....com>
---
 drivers/cpuidle/governors/teo.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpuidle/governors/teo.c b/drivers/cpuidle/governors/teo.c
index 7244f71c59c5..a39bef603408 100644
--- a/drivers/cpuidle/governors/teo.c
+++ b/drivers/cpuidle/governors/teo.c
@@ -291,7 +291,7 @@ static void teo_update(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		cpu_data->next_recent_idx = 0;

 	if (cpu_data->recent_idx[i] >= 0)
-		cpu_data->state_bins[cpu_data->recent_idx[i]].recent--;
+		cpu_data->state_bins[cpu_data->recent_idx[i]].recent = 0;

 	/*
 	 * If the deepest state's target residency is below the tick length,
-- 
2.34.1