Re: [PATCH 07/20 v3] sched/fair: Prepare select_task_rq_fair() to be called for new cases

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] sched/fair: wrong util_est accounting in cpu_util() for
  queued tasks in EAS
--

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

sched/fair: Prepare select_task_rq_fair() to be called for new cases

This commit updates select_task_rq_fair() and its helpers so that runqueue
selection can be invoked for already-queued runnable tasks outside of
wake-up, exec, and fork, routing those cases through EAS and
select_idle_sibling().

> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index 039dce244aaaf..8f99c8bf1a80d 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
[ ... ]
> @@ -9912,7 +9924,7 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
>  
>  	target = prev_cpu;
>  
> -	sync_entity_load_avg(&p->se);
> +	sync_task_load_avg(p);
>  	if (!task_util_est(p) && p_util_min == 0)
>  		return target;
>  
> @@ -10060,46 +10072,55 @@ static int find_energy_efficient_cpu(struct task_struct *p, int prev_cpu)
[ ... ]
> -		if (!is_rd_overutilized(this_rq()->rd)) {
> -			new_cpu = find_energy_efficient_cpu(p, prev_cpu);
> -			if (new_cpu >= 0)
> -				return new_cpu;
> -			new_cpu = prev_cpu;
> -		}
> +	/*
> +	 * We don't want EAS to be called for exec or fork but it should be
> +	 * called for any other case such as wake up or push callback.
> +	 */
> +	if (!is_rd_overutilized(this_rq()->rd) && want_sibling) {
> +		new_cpu = find_energy_efficient_cpu(p, prev_cpu);
> +		if (new_cpu >= 0)
> +			return new_cpu;
> +	}

[Severity: Medium]
When find_energy_efficient_cpu() is called for a task that is already
queued on prev_cpu, does cpu_util() miscalculate util_est across the
performance domain?

find_energy_efficient_cpu(), eenv_pd_busy_time(), and eenv_pd_max_util()
evaluate each CPU in the performance domain by calling cpu_util(cpu, p,
dst_cpu, ...).  While cpu_util() guards util_avg and runnable_avg updates
with add_task and sub_task, its UTIL_EST handling assumes EAS is only
called on wake-up (when p is dequeued) and that a queued task is only
passed for its own CPU via cpu_util_without():

kernel/sched/fair.c:cpu_util() {
    ...
		if (dst_cpu == cpu)
			util_est += _task_util_est(p);
		else if (p && unlikely(task_on_rq_queued(p) || current == p))
			lsub_positive(&util_est, _task_util_est(p));
    ...
}

When p is queued on prev_cpu, cfs_rq->avg.util_est on prev_cpu already
includes _task_util_est(p).  Evaluating cpu == prev_cpu with dst_cpu ==
prev_cpu adds _task_util_est(p) a second time, and evaluating any other
CPU in the performance domain with dst_cpu != cpu subtracts
_task_util_est(p) from that unrelated CPU's util_est, skewing capacity
fitness checks and energy estimates in EAS.

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