Re: [PATCH v2] sched/fair: sanitize vruntime of entity being migrated

[Dietmar Eggemann]

On 18/03/2023 08:45, Zhang Qiao wrote:
> 
> 
> 在 2023/3/18 0:08, Vincent Guittot 写道:
>> Commit 829c1651e9c4 ("sched/fair: sanitize vruntime of entity being placed")
>> fixes an overflowing bug, but ignore a case that se->exec_start is reset
>> after a migration.
>>
>> For fixing this case, we delay the reset of se->exec_start after
>> placing the entity which se->exec_start to detect long sleeping task.
>>
>> In order to take into account a possible divergence between the clock_task
>> of 2 rqs, we increase the threshold to around 104 days.
>>
>>
>> Fixes: 829c1651e9c4 ("sched/fair: sanitize vruntime of entity being placed")
>> Signed-off-by: Zhang Qiao <zhangqiao22@xxxxxxxxxx>
>> Signed-off-by: Vincent Guittot <vincent.guittot@xxxxxxxxxx>
>> ---
>>
>> My last proposal was not yet correct as the exec_start was not always
>> reset after migrating a task. I finally found this solution which keeps
>> the long sleep detection to one place as well as the reset of se->exec_start.
>>
> 
> Tested-by: Zhang Qiao <zhangqiao22@xxxxxxxxxx>
> 
> I have retested it with this version, and the result is fine.
> 
> -------
> 
>  Performance counter stats for 'hackbench -g 44 -f 20 --process --pipe -l 60000 -s 100' (10 runs):
> 
>              80.10 +- 1.22 seconds time elapsed  ( +-  1.53% )

[...]

>> @@ -8701,7 +8734,7 @@ static void attach_task(struct rq *rq, struct task_struct *p)
>>  	lockdep_assert_rq_held(rq);
>>  
>>  	WARN_ON_ONCE(task_rq(p) != rq);
>> -	activate_task(rq, p, ENQUEUE_NOCLOCK);
>> +	activate_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_MIGRATED);
>>  	check_preempt_curr(rq, p, 0);

Why not:

diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index b9bc1ab67aaa..96dd3a62e683 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7674,7 +7674,10 @@ static void migrate_task_rq_fair(struct task_struct *p, int new_cpu)
 		se->vruntime -= u64_u32_load(cfs_rq->min_vruntime);
 	}
 
-	if (!task_on_rq_migrating(p)) {
+	if (task_on_rq_migrating(p)) {
+		/* We have migrated, no longer consider this task hot */
+		se->exec_start = 0;
+	} else {
 		remove_entity_load_avg(se);
 
 		/*
@@ -8726,7 +8729,7 @@ static void attach_task(struct rq *rq, struct task_struct *p)
 	lockdep_assert_rq_held(rq);
 
 	WARN_ON_ONCE(task_rq(p) != rq);
-	activate_task(rq, p, ENQUEUE_NOCLOCK | ENQUEUE_MIGRATED);
+	activate_task(rq, p, ENQUEUE_NOCLOCK);
 	check_preempt_curr(rq, p, 0);
 }


entity_is_long_sleeper() will bail early for these rq-migrating tasks
for which a long-sleep test would make little sense anyway.

Plus move_queued_task() (e.g. from sched_exex()) would be covered as well.