linux-kernel - Re: [PATCH v2] sched/uclamp: Align uclamp and util

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 for Android: free password hash cracker in your pocket

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

Message-ID: <acef778c-a359-484d-a61d-aeca81bdae1d@arm.com>
Date: Thu, 17 Apr 2025 00:12:20 +0200
From: Dietmar Eggemann <dietmar.eggemann@....com>
To: Vincent Guittot <vincent.guittot@...aro.org>,
 Xuewen Yan <xuewen.yan94@...il.com>
Cc: Xuewen Yan <xuewen.yan@...soc.com>, mingo@...hat.com,
 peterz@...radead.org, juri.lelli@...hat.com, rostedt@...dmis.org,
 bsegall@...gle.com, mgorman@...e.de, vschneid@...hat.com,
 hongyan.xia2@....com, qyousef@...alina.io, ke.wang@...soc.com,
 di.shen@...soc.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] sched/uclamp: Align uclamp and util_est and call
 before freq update

On 16/04/2025 14:19, Vincent Guittot wrote:
> On Wed, 16 Apr 2025 at 13:07, Xuewen Yan <xuewen.yan94@...il.com> wrote:
>>
>> On Wed, Apr 16, 2025 at 5:42 PM Vincent Guittot
>> <vincent.guittot@...aro.org> wrote:
>>>
>>> On Wed, 16 Apr 2025 at 04:55, Xuewen Yan <xuewen.yan94@...il.com> wrote:
>>>>
>>>> On Wed, Apr 16, 2025 at 1:05 AM Vincent Guittot
>>>> <vincent.guittot@...aro.org> wrote:
>>>>>
>>>>> On Tue, 25 Mar 2025 at 02:48, Xuewen Yan <xuewen.yan@...soc.com> wrote:

[...]

>>> Why is testing sched_delayed enough for migrating/prio_change ?
>>> With your change, we will remove then add back util_est when changing
>>> prio of the task which is useless
>>
>> I sincerely apologize for any misunderstanding my previous description
>> may have caused.
>> When changing prio without changing class, the delayed_task's
>> sched_delayed flag is not changed,
>> we would not remove then add back util_est.
>> If the class was changed：
>>
>> if (prev_class != next_class && p->se.sched_delayed)
>>                  dequeue_task(rq, p, DEQUEUE_SLEEP | DEQUEUE_DELAYED |
>> DEQUEUE_NOCLOCK);
>>
>> It will dequeue the delayed-task first, and will not enqueue it.
>>
>> As for normal tasks which are not delayed, indeed, the issue you
>> mentioned can occur, but it seems that this problem has always
>> existed. Perhaps this is a new issue that has come up.
> 
> I have been confused by the patch that added  the condition "if
> (!(p->se.sched_delayed && (task_on_rq_migrating(p) || (flags &
> ENQUEUE_RESTORE))))". I wrongly thought it was for
> dequeue_save/enqueue_restore

No, this was just for sched_delayed. I convinced myself that the
logic stays the same with the following tests:

-->8--

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index eb5a2572b4f8..65692938696f 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -6930,6 +6930,19 @@ enqueue_task_fair(struct rq *rq, struct task_struct *p, int flags)
        int rq_h_nr_queued = rq->cfs.h_nr_queued;
        u64 slice = 0;
 
+       bool D = !!p->se.sched_delayed;
+       bool M = task_on_rq_migrating(p);
+       bool Er = !!(flags & ENQUEUE_RESTORE);
+       bool Ed = !!(flags & ENQUEUE_DELAYED);
+
+       /* won't be called */
+       BUG_ON(D && M && Er);                           // [1]
+       BUG_ON(!D && M && Er);                          // [2]
+
+       BUG_ON(D && ((M || Er) == Ed));                 // [3]
+       BUG_ON(!(D && (M || Er)) != (!D || Ed));        // [4]
+       BUG_ON(!(D && (M || Er)) != (!(D && !Ed)));
+
        /*
         * The code below (indirectly) updates schedutil which looks at
         * the cfs_rq utilization to select a frequency.
@@ -7178,6 +7191,17 @@ static int dequeue_entities(struct rq *rq, struct sched_entity *se, int flags)
  */
 static bool dequeue_task_fair(struct rq *rq, struct task_struct *p, int flags)
 {
+       bool D = !!p->se.sched_delayed;
+       bool M = task_on_rq_migrating(p);
+       bool Ds = !!(flags & DEQUEUE_SAVE);
+
+       /* won't be called */
+       BUG_ON(D && M && Ds);                           // [5]
+       BUG_ON(!D && M && Ds);                          // [6]
+       BUG_ON(D && !M && !Ds);                         // [7]
+
+       BUG_ON(!(D && (M || Ds)) != !D);                // [8]
+
        if (!(p->se.sched_delayed && (task_on_rq_migrating(p) || (flags & DEQUEUE_SAVE))))
                util_est_dequeue(&rq->cfs, p);

-->8--

In enqueue, when D is true, M or Er is never set with Ed. [3], [4].
In dequeue, since [7] is never true, [8] is never true as well.

> Could you please split this in 2 patches :
> patch 1 updates condition for util_est_dequeue/enqueue  and a
> description why it's safe
> patch 2 for aligning uclamp with util_est

+1

[...]