Re: [RFC PATCH] sched/util_est: Do not sub the delayed-task's util-est

[Dietmar Eggemann <dietmar.eggemann@arm.com>]

On 14/03/2025 10:09, Xuewen Yan wrote:
> In cpu_util_without, When the task is in rq, we should
> sub the task's util_est, however, the delayed_task->on_rq
> is true, however, the delayed_task's util had been sub
> when sleep, so there is no need to sub the delayed task's
> util-est. So add the checking of delayed-task.
> 
> On the other hand, as said in [1], the logic of util_est's
> enqueue/dequeue could be simplified.
> So simplify it by aligning with the conditions of uclamp.

This flag simplification looks good to me.

IMHO, you should submit this with the uclamp change so that we can call
uclamp_rq_inc() before p->sched_class->enqueue_task(). To make sure the
task which is enqueued with 'flags & ENQUEUE_DELAYED' is considered for
cpufreq_update_util() in enqueue_task_fair() (Hongyan's finding in
https://lkml.kernel.org/r/84441660bef0a5e67fd09dc3787178d0276dad31.1740664400.git.hongyan.xia2@arm.com)

I would prefer the less invasive fix you presented here:

https://lkml.kernel.org/r/CAB8ipk9LpbiUDnbcV6+59+Sa=Ai7tFzO===mpLD3obNdV4=J-A@mail.gmail.com

since uclamp is already a core thing (fair + rt), it works for current
max aggregation and it's less invasive.

[...]

> @@ -8037,7 +8037,8 @@ cpu_util(int cpu, struct task_struct *p, int dst_cpu, int boost)
>  		 */
>  		if (dst_cpu == cpu)
>  			util_est += _task_util_est(p);
> -		else if (p && unlikely(task_on_rq_queued(p) || current == p))
> +		else if (p && unlikely(current == p ||
> +			 (task_on_rq_queued(p) && !p->se.sched_delayed)))
>  			lsub_positive(&util_est, _task_util_est(p));

cpu_util(..., p != NULL, ...) is only used for select_task_rq_fair().
IMHO p->se.sched_delayed is not set there.