Re: [PATCH v1] cpuidle: governors: menu: Avoid using invalid recent intervals data

[Christian Loehle <christian.loehle@arm.com>]

On 8/11/25 16:03, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Marc has reported that commit 85975daeaa4d ("cpuidle: menu: Avoid
> discarding useful information") caused the number of wakeup interrupts
> to increase on an idle system [1], which was not expected to happen
> after merely allowing shallower idle states to be selected by the
> governor in some cases.
> 
> However, on the system in question, all of the idle states deeper than
> WFI are rejected by the driver due to a firmware issue [2].  This causes
> the governor to only consider the recent interval duriation data
duration

> corresponding to attempts to enter WFI that are successful and the
> recent invervals table is filled with values lower than the scheduler
intervals

> tick period.  Consequently, the governor predicts an idle duration
> below the scheduler tick period length and avoids stopping the tick
> more often which leads to the observed symptom.
> 
> Address it by modifying the governor to update the recent intervals
> table also when entering the previously selected idle state fails, so
> it knows that the short idle intervals might have been the minority
> had the selected idle states been actually entered every time.
> 
> Fixes: 85975daeaa4d ("cpuidle: menu: Avoid discarding useful information")
> Link: https://lore.kernel.org/linux-pm/86o6sv6n94.wl-maz@kernel.org/ [1]
> Link: https://lore.kernel.org/linux-pm/7ffcb716-9a1b-48c2-aaa4-469d0df7c792@arm.com/ [2]
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Tested-by: Christian Loehle <christian.loehle@arm.com>
> Tested-by: Marc Zyngier <maz@kernel.org>

Reviewed-by: Christian Loehle <christian.loehle@arm.com>

> ---
>  drivers/cpuidle/governors/menu.c |   21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -97,6 +97,14 @@
>  
>  static DEFINE_PER_CPU(struct menu_device, menu_devices);
>  
> +static void menu_update_intervals(struct menu_device *data, unsigned int interval_us)
> +{
> +	/* Update the repeating-pattern data. */
> +	data->intervals[data->interval_ptr++] = interval_us;
> +	if (data->interval_ptr >= INTERVALS)
> +		data->interval_ptr = 0;
> +}
> +
>  static void menu_update(struct cpuidle_driver *drv, struct cpuidle_device *dev);
>  
>  /*
> @@ -222,6 +230,14 @@
>  	if (data->needs_update) {
>  		menu_update(drv, dev);
>  		data->needs_update = 0;
> +	} else if (!dev->last_residency_ns) {
> +		/*
> +		 * This happens when the driver rejects the previously selected
> +		 * idle state and returns an error, so update the recent
> +		 * intervals table to prevent invalid information from being
> +		 * used going forward.
> +		 */
> +		menu_update_intervals(data, UINT_MAX);
>  	}
>  
>  	/* Find the shortest expected idle interval. */
> @@ -482,10 +498,7 @@
>  
>  	data->correction_factor[data->bucket] = new_factor;
>  
> -	/* update the repeating-pattern data */
> -	data->intervals[data->interval_ptr++] = ktime_to_us(measured_ns);
> -	if (data->interval_ptr >= INTERVALS)
> -		data->interval_ptr = 0;
> +	menu_update_intervals(data, ktime_to_us(measured_ns));
>  }
>  
>  /**
> 
> 
>