Messages in this thread | | | From | Vincent Guittot <> | Date | Mon, 20 Mar 2023 14:26:35 +0100 | Subject | Re: [PATCH v2] sched/fair: sanitize vruntime of entity being migrated |
| |
On Mon, 20 Mar 2023 at 13:29, Dietmar Eggemann <dietmar.eggemann@arm.com> wrote: > > 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@huawei.com> > >> Signed-off-by: Vincent Guittot <vincent.guittot@linaro.org> > >> --- > >> > >> 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@huawei.com> > > > > 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;
mainly to keep the clear of se->exec_start = 0 in one place to ease the maintenance
> + } 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.
| |