Re: [PATCH v2] sched/uclamp: Align uclamp and util_est and call before freq update

[Dietmar Eggemann]

On 16/04/2025 14:19, Vincent Guittot wrote:
> On Wed, 16 Apr 2025 at 13:07, Xuewen Yan <xuewen.yan94@xxxxxxxxx> wrote:
>>
>> On Wed, Apr 16, 2025 at 5:42 PM Vincent Guittot
>> <vincent.guittot@xxxxxxxxxx> wrote:
>>>
>>> On Wed, 16 Apr 2025 at 04:55, Xuewen Yan <xuewen.yan94@xxxxxxxxx> wrote:
>>>>
>>>> On Wed, Apr 16, 2025 at 1:05 AM Vincent Guittot
>>>> <vincent.guittot@xxxxxxxxxx> wrote:
>>>>>
>>>>> On Tue, 25 Mar 2025 at 02:48, Xuewen Yan <xuewen.yan@xxxxxxxxxx> 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

[...]