Re: [PATCH 06/20 v3] sched/eevdf: Add min slice check when selecting CPU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] sched/fair: broken busy CPU ranking and SMT bias in
  select_idle_capacity()
--

commit 67b6addf19b77860d43186abca2f383d2cb45b72
Author: Vincent Guittot <vincent.guittot@linaro.org>

sched/eevdf: Add min slice check when selecting CPU

This commit adds a fallback CPU selection tier in select_idle_cpu(),
select_idle_core(), and select_idle_capacity() that picks a CPU whose
runqueue min_slice is larger than the waking task's slice when no idle CPU
is found.

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index cfc99b94d9ce9..039dce244aaaf 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
[ ... ]
> @@ -9091,24 +9123,26 @@ static int select_idle_cpu(struct task_struct *p, struct sched_domain *sd, bool
>   *   ASYM_IDLE_THREAD_MISFIT          0   thread  Busy SMT sibling; capacity
>   *                                                does not fit.
>   *
> - * ASYM_IDLE_CORE_BIAS (-3) is an offset, not a state. On an idle core,
> + * ASYM_IDLE_CORE_BIAS (-4) is an offset, not a state. On an idle core,
>   * fits += ASYM_IDLE_CORE_BIAS rebases thread-tier ranks into the core tier:
>   *
> - *   ASYM_IDLE_THREAD_UCLAMP_MISFIT (-1) + BIAS -> ASYM_IDLE_UCLAMP_MISFIT   (-4)
> - *   ASYM_IDLE_THREAD_MISFIT         (0) + BIAS -> ASYM_IDLE_COMPLETE_MISFIT (-3)
> + *   ASYM_IDLE_THREAD_UCLAMP_MISFIT (-1) + BIAS -> ASYM_IDLE_UCLAMP_MISFIT   (-5)
> + *   ASYM_IDLE_THREAD_MISFIT         (0) + BIAS -> ASYM_IDLE_COMPLETE_MISFIT (-4)
>   *
> - * ASYM_IDLE_THREAD_FITS (-2) is never rebased because a fully-fitting idle-core
> + * ASYM_IDLE_THREAD_FITS (-3) is never rebased because a fully-fitting idle-core
>   * candidate early-returns from select_idle_capacity().
>   */
>  enum asym_fits_state {
> -	ASYM_IDLE_UCLAMP_MISFIT = -4,
> +	ASYM_BUSY_FITS = -6,
> +	ASYM_IDLE_UCLAMP_MISFIT,
>  	ASYM_IDLE_COMPLETE_MISFIT,
>  	ASYM_IDLE_THREAD_FITS,
> +	ASYM_BUSY_THREAD_FITS,
>  	ASYM_IDLE_THREAD_UCLAMP_MISFIT,
>  	ASYM_IDLE_THREAD_MISFIT,
>  
>  	/* util_fits_cpu() bias for idle core */
> -	ASYM_IDLE_CORE_BIAS = -3,
> +	ASYM_IDLE_CORE_BIAS = -4,
>  };
[ ... ]
> @@ -9160,42 +9196,49 @@ select_idle_capacity(struct task_struct *p, struct sched_domain *sd, int target)
>  		if (!has_idle_core && --nr <= 0)
>  			return best_cpu;
>  
> -		if (!choose_idle_cpu(cpu, p))
> -			continue;
> -
>  		fits = util_fits_cpu(task_util, util_min, util_max, cpu);
>  
> -		/*
> -		 * Perfect fit: capacity satisfies util + uclamp and the CPU
> -		 * sits on a fully-idle SMT core, this is a !SMT system, or
> -		 * there is no idle core to find.
> -		 * Short-circuit the rank-based selection and return
> -		 * immediately.
> -		 */
> -		if (fits > 0 && preferred_core)
> -			return cpu;
> -		/*
> -		 * Only the min performance hint (i.e. uclamp_min) doesn't fit.
> -		 * Look for the CPU with best capacity.
> -		 */
> -		else if (fits < 0)
> +		if (choose_idle_cpu(cpu, p)) {
> +			/*
> +			 * Perfect fit: capacity satisfies util + uclamp and the CPU
> +			 * sits on a fully-idle SMT core, this is a !SMT system, or
> +			 * there is no idle core to find.
> +			 * Short-circuit the rank-based selection and return
> +			 * immediately.
> +			 */
> +			if (fits > 0 && preferred_core)
> +				return cpu;
> +			/*
> +			 * Only the min performance hint (i.e. uclamp_min) doesn't fit.
> +			 * Look for the CPU with best capacity.
> +			 */
> +			else if (fits < 0)
> +				cpu_cap = get_actual_cpu_capacity(cpu);
> +			/*
> +			 * fits > 0 implies we are not on a preferred core, but the util
> +			 * fits CPU capacity. Set fits to ASYM_IDLE_THREAD_FITS
> +			 * so the effective range becomes
> +			 * [ASYM_IDLE_THREAD_FITS, ASYM_IDLE_THREAD_MISFIT], where:
> +			 *    ASYM_IDLE_THREAD_MISFIT - does not fit
> +			 *    ASYM_IDLE_THREAD_UCLAMP_MISFIT - fits with the exception of UCLAMP_MIN
> +			 *    ASYM_BUSY_THREAD_FITS - fits with the exception of idleness
> +			 *			      but can preempt current
> +			 *    ASYM_IDLE_THREAD_FITS - fits with the exception of preferred_core
> +			 */
> +			else if (fits > 0)
> +				fits = ASYM_IDLE_THREAD_FITS;
> +
> +		} else if (fits > 0 && task_slice < get_rq_min_slice(cpu_rq(cpu))) {
> +			fits = ASYM_BUSY_THREAD_FITS;
>  			cpu_cap = get_actual_cpu_capacity(cpu);
> -		/*
> -		 * fits > 0 implies we are not on a preferred core, but the util
> -		 * fits CPU capacity. Set fits to ASYM_IDLE_THREAD_FITS
> -		 * so the effective range becomes
> -		 * [ASYM_IDLE_THREAD_FITS, ASYM_IDLE_THREAD_MISFIT], where:
> -		 *    ASYM_IDLE_THREAD_MISFIT - does not fit
> -		 *    ASYM_IDLE_THREAD_UCLAMP_MISFIT - fits with the exception of UCLAMP_MIN
> -		 *    ASYM_IDLE_THREAD_FITS - fits with the exception of preferred_core
> -		 */
> -		else if (fits > 0)
> -			fits = ASYM_IDLE_THREAD_FITS;
> +		} else {
> +			continue;
> +		}
>  
>  		/*
>  		 * If we are on a preferred core, translate the range of fits
> -		 * of [ASYM_IDLE_THREAD_UCLAMP_MISFIT, ASYM_IDLE_THREAD_MISFIT] to
> -		 * [ASYM_IDLE_UCLAMP_MISFIT, ASYM_IDLE_COMPLETE_MISFIT].
> +		 * of [ASYM_BUSY_THREAD_FITS, ASYM_IDLE_THREAD_MISFIT] to
> +		 * [ASYM_BUSY_FITS, ASYM_IDLE_COMPLETE_MISFIT].
>  		 * This ensures that an idle core is always given priority over
>  		 * (partially) busy core.
>  		 *
> @@ -9219,15 +9262,17 @@ select_idle_capacity(struct task_struct *p, struct sched_domain *sd, int target)
>  
>  	/*
>  	 * A value in the [ASYM_IDLE_UCLAMP_MISFIT, ASYM_IDLE_COMPLETE_MISFIT]
> -	 * range means the chosen CPU is in a fully idle SMT core. Values above
> -	 * ASYM_IDLE_COMPLETE_MISFIT mean we never ranked such a CPU best.
> +	 * range mean the chosen CPU is in a fully idle SMT core. Values
> +	 * ASYM_BUSY_FITS or above ASYM_IDLE_COMPLETE_MISFIT mean we never
> +	 * ranked such a CPU best.
>  	 *
>  	 * The asym-capacity wakeup path returns from select_idle_sibling()
>  	 * after this function and never runs select_idle_cpu(), so the usual
>  	 * select_idle_cpu() tail that clears idle cores must live here when the
>  	 * idle-core preference did not win.
>  	 */
> -	if (has_idle_core && best_fits > ASYM_IDLE_COMPLETE_MISFIT)
> +	if (has_idle_core && (best_fits > ASYM_IDLE_COMPLETE_MISFIT ||
> +			      best_fits == ASYM_BUSY_FITS))
>  		set_idle_cores(target, false);
>  
>  	return best_cpu;

[Severity: High]
Does select_idle_capacity() rank busy CPUs ahead of idle CPUs and idle SMT
cores?

In select_idle_capacity(), lower fits values are more preferred
(fits < best_fits), and preferred_core is computed as
!has_idle_core || is_core_idle(cpu).  is_core_idle() assumes its argument
was already checked for idleness and only tests SMT siblings:

kernel/sched/fair.c:is_core_idle() {
	for_each_cpu(sibling, cpu_smt_mask(cpu)) {
		if (cpu == sibling)
			continue;

		if (!idle_cpu(sibling))
			return false;
	}

	return true;
}

On a partially busy SMT core where thread A is busy (fits > 0 and
task_slice < get_rq_min_slice(cpu_rq(A))) and its sibling thread B is idle,
is_core_idle(A) returns true while is_core_idle(B) returns false.  Busy
thread A therefore gets preferred_core == true and is biased from
ASYM_BUSY_THREAD_FITS (-2) by ASYM_IDLE_CORE_BIAS (-4) to
ASYM_BUSY_FITS (-6), whereas its idle sibling B gets
ASYM_IDLE_THREAD_FITS (-3), so select_idle_capacity() stacks the waking
task onto the busy thread instead of its idle sibling on the same core.

Even on !SMT systems (or when has_idle_core is false), preferred_core is
true for every CPU, biasing any fitting busy CPU with a larger min_slice to
ASYM_BUSY_FITS (-6) so it beats completely idle cores/CPUs ranked at
ASYM_IDLE_UCLAMP_MISFIT (-5) or ASYM_IDLE_COMPLETE_MISFIT (-4).  When
has_idle_core is true and a candidate with ASYM_BUSY_FITS (-6) outranks an
idle core (-5 or -4), best_fits == ASYM_BUSY_FITS also calls
set_idle_cores(target, false) while fully idle cores still exist in the
domain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106151947.3991524-2-vincent.guittot@linaro.org?part=6