Re: [PATCH v1] cpuidle: governors: menu: Always check timers with tick stopped

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

On 1/20/26 15:26, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> After commit 5484e31bbbff ("cpuidle: menu: Skip tick_nohz_get_sleep_length()
> call in some cases"), if the return value of get_typical_interval()
> multiplied by NSEC_PER_USEC is not greater than RESIDENCY_THRESHOLD_NS,
> the menu governor will skip computing the time till the closest timer.
> If that happens when the tick has been stopped already, the selected
> idle state may be too deep due to the subsequent check comparing
> predicted_ns with TICK_NSEC and causing its value to be replaced with
> the expected time till the closest timer, which is KTIME_MAX in that
> case.  That will cause the deepest enabled idle state to be selected,
> but the time till the closest timer very well may be shorter than the
> target residency of that state, in which case a shallower state should
> be used.
> 
> Address this by making menu_select() always compute the time till the
> closest timer when the tick has been stopped.
> 
> Also move the predicted_ns check mentioned above into the branch in
> which the time till the closest timer is determined because it only
> needs to be done in that case.
> 
> Fixes: 5484e31bbbff ("cpuidle: menu: Skip tick_nohz_get_sleep_length() call in some cases")
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpuidle/governors/menu.c |   22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -239,7 +239,7 @@ static int menu_select(struct cpuidle_dr
>  
>  	/* Find the shortest expected idle interval. */
>  	predicted_ns = get_typical_interval(data) * NSEC_PER_USEC;
> -	if (predicted_ns > RESIDENCY_THRESHOLD_NS) {
> +	if (predicted_ns > RESIDENCY_THRESHOLD_NS || tick_nohz_tick_stopped()) {
>  		unsigned int timer_us;
>  
>  		/* Determine the time till the closest timer. */
> @@ -259,6 +259,16 @@ static int menu_select(struct cpuidle_dr
>  				   RESOLUTION * DECAY * NSEC_PER_USEC);
>  		/* Use the lowest expected idle interval to pick the idle state. */
>  		predicted_ns = min((u64)timer_us * NSEC_PER_USEC, predicted_ns);
> +		/*
> +		 * If the tick is already stopped, the cost of possible short
> +		 * idle duration misprediction is much higher, because the CPU
> +		 * may be stuck in a shallow idle state for a long time as a
> +		 * result of it.  In that case, say we might mispredict and use
> +		 * the known time till the closest timer event for the idle
> +		 * state selection.
> +		 */
> +		if (tick_nohz_tick_stopped() && predicted_ns < TICK_NSEC)
> +			predicted_ns = data->next_timer_ns;
>  	} else {
>  		/*
>  		 * Because the next timer event is not going to be determined
> @@ -285,16 +295,6 @@ static int menu_select(struct cpuidle_dr
>  	}
>  
>  	/*
> -	 * If the tick is already stopped, the cost of possible short idle
> -	 * duration misprediction is much higher, because the CPU may be stuck
> -	 * in a shallow idle state for a long time as a result of it.  In that
> -	 * case, say we might mispredict and use the known time till the closest
> -	 * timer event for the idle state selection.
> -	 */
> -	if (tick_nohz_tick_stopped() && predicted_ns < TICK_NSEC)
> -		predicted_ns = data->next_timer_ns;
> -
> -	/*
>  	 * Find the idle state with the lowest power while satisfying
>  	 * our constraints.
>  	 */
> 


Seems I forgot about this, sorry!

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