Re: [PATCH v2 1/2] cpuidle: governors: menu: Refine stopped tick handling

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

On 2/23/26 15:38, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> This change is based on the observation that it is not in fact necessary
> to select a deep idle state every time the scheduler tick has been
> stopped before the idle state selection takes place.  Namely, if the
> time till the closest timer (that is not the tick) is short enough,
> a shallow idle state can be selected because the timer will kick the
> CPU out of that state, so the damage from a possible overly optimistic
> selection will be limited.
> 
> Update the menu governor in accordance with the above and use twice
> the tick period length as the "safe timer range" for allowing the
> original predicted_ns value to be used even if the tick has been
> stopped.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2: No changes
> 
> ---
>  drivers/cpuidle/governors/gov.h  |    5 +++++
>  drivers/cpuidle/governors/menu.c |   15 +++++++++------
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> --- a/drivers/cpuidle/governors/gov.h
> +++ b/drivers/cpuidle/governors/gov.h
> @@ -10,5 +10,10 @@
>   * check the time till the closest expected timer event.
>   */
>  #define RESIDENCY_THRESHOLD_NS	(15 * NSEC_PER_USEC)
> +/*
> + * If the closest timer is in this range, the governor idle state selection need
> + * not be adjusted after the scheduler tick has been stopped.
> + */
> +#define SAFE_TIMER_RANGE_NS	(2 * TICK_NSEC)
>  
>  #endif /* __CPUIDLE_GOVERNOR_H */
> --- a/drivers/cpuidle/governors/menu.c
> +++ b/drivers/cpuidle/governors/menu.c
> @@ -261,13 +261,16 @@ static int menu_select(struct cpuidle_dr
>  		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.
> +		 * idle duration misprediction is higher because the CPU may get
> +		 * stuck in a shallow idle state then.  To avoid that, if
> +		 * predicted_ns is small enough, say it might be mispredicted
> +		 * and use the known time till the closest timer for idle state
> +		 * selection unless that timer is going to trigger within
> +		 * SAFE_TIMER_RANGE_NS in which case it can be regarded as a
> +		 * sufficient safety net.
>  		 */
> -		if (tick_nohz_tick_stopped() && predicted_ns < TICK_NSEC)
> +		if (tick_nohz_tick_stopped() && predicted_ns < TICK_NSEC &&
> +		    data->next_timer_ns > SAFE_TIMER_RANGE_NS)
>  			predicted_ns = data->next_timer_ns;
>  	} else {
>  		/*
> 
> 
> 

So FWIW both patches look sane to me, I'm still trying to get some test setup
to see what this looks like and should look like, but for now:
Reviewed-by: Christian Loehle <christian.loehle@arm.com>