Re: [PATCH v2 2/2] cpuidle: governors: teo: Rearrange stopped tick handling

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

On 2/23/26 15:40, 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 teo governor in accordance with the above in analogy with
> the previous analogous menu governor update.
> 
> Among other things, this will cause the teo governor to call
> tick_nohz_get_sleep_length() every time when the tick has been
> stopped already and only change the original idle state selection
> if the time till the closest timer is beyond SAFE_TIMER_RANGE_NS
> which is way more straightforward than the current code flow.
> 
> Of course, this effectively throws away some of the recent teo governor
> changes made recently, but the resulting simplification is worth it in
> my view.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> 
> v1 -> v2: Take constraint_idx into account when looking for a deeper idle
>           state (Christian)
> 
> ---
>  drivers/cpuidle/governors/teo.c |   81 ++++++++++++++++------------------------
>  1 file changed, 34 insertions(+), 47 deletions(-)
> 
> --- a/drivers/cpuidle/governors/teo.c
> +++ b/drivers/cpuidle/governors/teo.c
> @@ -407,50 +407,13 @@ static int teo_select(struct cpuidle_dri
>  	 * better choice.
>  	 */
>  	if (2 * idx_intercept_sum > cpu_data->total - idx_hit_sum) {
> -		int min_idx = idx0;
> -
> -		if (tick_nohz_tick_stopped()) {
> -			/*
> -			 * Look for the shallowest idle state below the current
> -			 * candidate one whose target residency is at least
> -			 * equal to the tick period length.
> -			 */
> -			while (min_idx < idx &&
> -			       drv->states[min_idx].target_residency_ns < TICK_NSEC)
> -				min_idx++;
> -
> -			/*
> -			 * Avoid selecting a state with a lower index, but with
> -			 * the same target residency as the current candidate
> -			 * one.
> -			 */
> -			if (drv->states[min_idx].target_residency_ns ==
> -					drv->states[idx].target_residency_ns)
> -				goto constraint;
> -		}
> -
> -		/*
> -		 * If the minimum state index is greater than or equal to the
> -		 * index of the state with the maximum intercepts metric and
> -		 * the corresponding state is enabled, there is no need to look
> -		 * at the deeper states.
> -		 */
> -		if (min_idx >= intercept_max_idx &&
> -		    !dev->states_usage[min_idx].disable) {
> -			idx = min_idx;
> -			goto constraint;
> -		}
> -
>  		/*
>  		 * Look for the deepest enabled idle state, at most as deep as
>  		 * the one with the maximum intercepts metric, whose target
>  		 * residency had not been greater than the idle duration in over
>  		 * a half of the relevant cases in the past.
> -		 *
> -		 * Take the possible duration limitation present if the tick
> -		 * has been stopped already into account.
>  		 */
> -		for (i = idx - 1, intercept_sum = 0; i >= min_idx; i--) {
> +		for (i = idx - 1, intercept_sum = 0; i >= idx0; i--) {
>  			intercept_sum += cpu_data->state_bins[i].intercepts;
>  
>  			if (dev->states_usage[i].disable)
> @@ -463,7 +426,6 @@ static int teo_select(struct cpuidle_dri
>  		}
>  	}
>  
> -constraint:
>  	/*
>  	 * If there is a latency constraint, it may be necessary to select an
>  	 * idle state shallower than the current candidate one.
> @@ -472,13 +434,13 @@ constraint:
>  		idx = constraint_idx;
>  
>  	/*
> -	 * If either the candidate state is state 0 or its target residency is
> -	 * low enough, there is basically nothing more to do, but if the sleep
> -	 * length is not updated, the subsequent wakeup will be counted as an
> -	 * "intercept" which may be problematic in the cases when timer wakeups
> -	 * are dominant.  Namely, it may effectively prevent deeper idle states
> -	 * from being selected at one point even if no imminent timers are
> -	 * scheduled.
> +	 * If the tick has not been stopped and either the candidate state is
> +	 * state 0 or its target residency is low enough, there is basically
> +	 * nothing more to do, but if the sleep length is not updated, the
> +	 * subsequent wakeup will be counted as an "intercept".  That may be
> +	 * problematic in the cases when timer wakeups are dominant because it
> +	 * may effectively prevent deeper idle states from being selected at one
> +	 * point even if no imminent timers are scheduled.
>  	 *
>  	 * However, frequent timers in the RESIDENCY_THRESHOLD_NS range on one
>  	 * CPU are unlikely (user space has a default 50 us slack value for
> @@ -494,7 +456,8 @@ constraint:
>  	 * shallow idle states regardless of the wakeup type, so the sleep
>  	 * length need not be known in that case.
>  	 */
> -	if ((!idx || drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) &&
> +	if (!tick_nohz_tick_stopped() && (!idx ||
> +	     drv->states[idx].target_residency_ns < RESIDENCY_THRESHOLD_NS) &&
>  	    (2 * cpu_data->short_idles >= cpu_data->total ||
>  	     latency_req < LATENCY_THRESHOLD_NS))
>  		goto out_tick;
> @@ -502,6 +465,30 @@ constraint:
>  	duration_ns = tick_nohz_get_sleep_length(&delta_tick);
>  	cpu_data->sleep_length_ns = duration_ns;
>  
> +	/*
> +	 * If the tick has been stopped and the closest timer is too far away,
> +	 * update the selection to prevent the CPU from getting stuck in a
> +	 * shallow idle state for too long.
> +	 */
> +	if (tick_nohz_tick_stopped() && duration_ns > SAFE_TIMER_RANGE_NS &&
> +	    drv->states[idx].target_residency_ns < TICK_NSEC) {
> +		/*
> +		 * Look for the deepest enabled idle state with exit latency
> +		 * within the PM QoS limit and with target residency within
> +		 * duration_ns.
> +		 */
> +		for (i = constraint_idx; i > idx; i--) {
> +			if (dev->states_usage[i].disable)
> +				continue;
> +
> +			if (drv->states[i].target_residency_ns <= duration_ns) {
> +				idx = i;
> +				break;
> +			}
> +		}
> +		return idx;
> +	}
> +
>  	if (!idx)
>  		goto out_tick;
>  
> 

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