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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aa8baf67-a8ec-4ad8-a6a8-afdcd7036771@arm.com>
Date: Wed, 19 Mar 2025 14:32:35 +0100
From: Dietmar Eggemann <dietmar.eggemann@....com>
To: Xuewen Yan <xuewen.yan@...soc.com>, vincent.guittot@...aro.org,
 mingo@...hat.com, peterz@...radead.org, juri.lelli@...hat.com
Cc: rostedt@...dmis.org, bsegall@...gle.com, mgorman@...e.de,
 vschneid@...hat.co, linux-kernel@...r.kernel.org, hongyan.xia2@....com,
 qyousef@...alina.io, ke.wang@...soc.com, di.shen@...soc.com,
 xuewen.yan94@...il.com
Subject: Re: [RFC PATCH] sched/util_est: Do not sub the delayed-task's
 util-est

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.