Messages in this thread | | | Date | Wed, 20 Jul 2022 21:43:59 +0800 | Subject | Re: [External] Re: [PATCH v2 07/10] sched/fair: use update_load_avg() to attach/detach entity load_avg | From | Chengming Zhou <> |
| |
On 2022/7/19 23:02, Dietmar Eggemann wrote: > On 15/07/2022 18:21, Chengming Zhou wrote: >> On 2022/7/15 19:18, Dietmar Eggemann wrote: >>> On 13/07/2022 06:04, Chengming Zhou wrote: >>>> Since update_load_avg() support DO_ATTACH and DO_DETACH now, we can >>>> use update_load_avg() to implement attach/detach entity load_avg. >>>> >>>> Another advantage of using update_load_avg() is that it will check >>>> last_update_time before attach or detach, instead of unconditional >>>> attach/detach in the current code. >>>> >>>> This way can avoid some corner problematic cases of load tracking, >>>> like twice attach problem, detach unattached NEW task problem. >>> >>> This explanation is somewhat hard to follow for me. Since both issues >>> have been fixed already (you mention this further below) you're saying >>> that with you change you don't reintroduce them? >> >> Sorry for this not very clear explanation. >> >> Yes, both issues have been fixed already, what I want to say is that bugfix >> brings its own problem and limitation mentioned below. >> >> So I want to use another way to solve these problems better. > > [...] > >>>> These problems have been fixed in commit 7dc603c9028e >>>> ("sched/fair: Fix PELT integrity for new tasks"), which also >>>> bring its own problems. >>>> >>>> First, it add a new task state TASK_NEW and an unnessary limitation >>>> that we would fail when change the cgroup of TASK_NEW tasks. >> >> This is the limitation that bugfix has brought. >> >> We can't change cgroup or switch to fair for task with last_update_time=0 >> if we don't have conditional detach/attach. >> >> So we have to: >> >> 1. !fair task also need to set last_update_time. >> 2. cpu_cgroup_can_attach() have to wait for TASK_NEW to fully attached. > > I see. > > `cgroup_migrate_execute() -> cpu_cgroup_[can|]_attach()` has to wait for > `wake_up_new_task() -> WRITE_ONCE(p->__state, TASK_RUNNING)`. > > Just to understand this change better: IMHO, this is still the case for > fair tasks, right?
Yes, I think so. We could delete this limitation when we have conditional detach/attach, since nothing will be detached when last_update_time==0.
Thanks!
> > `wake_up_new_task() -> post_init_entity_util_avg() -> > attach_entity_cfs_rq()` has to happen before the fair task can move > between taskgroups in `cgroup_migrate_execute() -> cpu_cgroup_attach() > -> sched_move_task() -> sched_change_group() -> task_change_group_fair()`. > > [...]
| |