Messages in this thread | | | Date | Thu, 30 Jun 2022 09:52:30 +0800 | Subject | Re: [External] Re: [PATCH] sched: remove redundant cpu_cgrp_subsys->fork() | From | Chengming Zhou <> |
| |
On 2022/6/29 22:05, Vincent Guittot wrote: > On Wed, 22 Jun 2022 at 11:48, Chengming Zhou > <zhouchengming@bytedance.com> wrote: >> >> We use cpu_cgrp_subsys->fork() to set task group for the new fair task >> in cgroup_post_fork(). >> >> Since commit b1e8206582f9 ("sched: Fix yet more sched_fork() races") >> has already set task group for the new fair task in sched_cgroup_fork(), >> so cpu_cgrp_subsys->fork() can be removed. >> >> cgroup_can_fork() --> pin parent's sched_task_group >> sched_cgroup_fork() >> __set_task_cpu --> set task group >> cgroup_post_fork() >> ss->fork() := cpu_cgroup_fork() --> set again >> >> After this change, task_change_group_fair() only need to care about >> task cgroup migration, make the code much simplier. >> >> This patch move the task se depth setting to set_task_rq(), which >> is called when task moves across CPUs/groups, is a better place than >> attach_entity_cfs_rq(). The latter is also used when change task's >> sched_class, in which case the group/depth doesn't change actually. >> > > The commit message needs some minor fixes but apart from this > > Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
Ok, I will try to improve and send v2. Thanks for your review.
> >> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com> >> --- >> kernel/sched/core.c | 27 ++++----------------------- >> kernel/sched/fair.c | 31 +------------------------------ >> kernel/sched/sched.h | 6 ++---- >> 3 files changed, 7 insertions(+), 57 deletions(-) >> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c >> index c16f8cc5de08..f44bc5b889ab 100644 >> --- a/kernel/sched/core.c >> +++ b/kernel/sched/core.c >> @@ -481,8 +481,7 @@ sched_core_dequeue(struct rq *rq, struct task_struct *p, int flags) { } >> * p->se.load, p->rt_priority, >> * p->dl.dl_{runtime, deadline, period, flags, bw, density} >> * - sched_setnuma(): p->numa_preferred_nid >> - * - sched_move_task()/ >> - * cpu_cgroup_fork(): p->sched_task_group >> + * - sched_move_task(): p->sched_task_group >> * - uclamp_update_active() p->uclamp* >> * >> * p->state <- TASK_*: >> @@ -10098,7 +10097,7 @@ void sched_release_group(struct task_group *tg) >> spin_unlock_irqrestore(&task_group_lock, flags); >> } >> >> -static void sched_change_group(struct task_struct *tsk, int type) >> +static void sched_change_group(struct task_struct *tsk) >> { >> struct task_group *tg; >> >> @@ -10114,7 +10113,7 @@ static void sched_change_group(struct task_struct *tsk, int type) >> >> #ifdef CONFIG_FAIR_GROUP_SCHED >> if (tsk->sched_class->task_change_group) >> - tsk->sched_class->task_change_group(tsk, type); >> + tsk->sched_class->task_change_group(tsk); >> else >> #endif >> set_task_rq(tsk, task_cpu(tsk)); >> @@ -10145,7 +10144,7 @@ void sched_move_task(struct task_struct *tsk) >> if (running) >> put_prev_task(rq, tsk); >> >> - sched_change_group(tsk, TASK_MOVE_GROUP); >> + sched_change_group(tsk); >> >> if (queued) >> enqueue_task(rq, tsk, queue_flags); >> @@ -10223,23 +10222,6 @@ static void cpu_cgroup_css_free(struct cgroup_subsys_state *css) >> sched_unregister_group(tg); >> } >> >> -/* >> - * This is called before wake_up_new_task(), therefore we really only >> - * have to set its group bits, all the other stuff does not apply. >> - */ >> -static void cpu_cgroup_fork(struct task_struct *task) >> -{ >> - struct rq_flags rf; >> - struct rq *rq; >> - >> - rq = task_rq_lock(task, &rf); >> - >> - update_rq_clock(rq); >> - sched_change_group(task, TASK_SET_GROUP); >> - >> - task_rq_unlock(rq, task, &rf); >> -} >> - >> static int cpu_cgroup_can_attach(struct cgroup_taskset *tset) >> { >> struct task_struct *task; >> @@ -11136,7 +11118,6 @@ struct cgroup_subsys cpu_cgrp_subsys = { >> .css_released = cpu_cgroup_css_released, >> .css_free = cpu_cgroup_css_free, >> .css_extra_stat_show = cpu_extra_stat_show, >> - .fork = cpu_cgroup_fork, >> .can_attach = cpu_cgroup_can_attach, >> .attach = cpu_cgroup_attach, >> .legacy_cftypes = cpu_legacy_files, >> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c >> index 8bed75757e65..fdb9125fe34e 100644 >> --- a/kernel/sched/fair.c >> +++ b/kernel/sched/fair.c >> @@ -11325,14 +11325,6 @@ static void attach_entity_cfs_rq(struct sched_entity *se) >> { >> struct cfs_rq *cfs_rq = cfs_rq_of(se); >> >> -#ifdef CONFIG_FAIR_GROUP_SCHED >> - /* >> - * Since the real-depth could have been changed (only FAIR >> - * class maintain depth value), reset depth properly. >> - */ >> - se->depth = se->parent ? se->parent->depth + 1 : 0; >> -#endif >> - >> /* Synchronize entity with its cfs_rq */ >> update_load_avg(cfs_rq, se, sched_feat(ATTACH_AGE_LOAD) ? 0 : SKIP_AGE_LOAD); >> attach_entity_load_avg(cfs_rq, se); >> @@ -11431,15 +11423,7 @@ void init_cfs_rq(struct cfs_rq *cfs_rq) >> } >> >> #ifdef CONFIG_FAIR_GROUP_SCHED >> -static void task_set_group_fair(struct task_struct *p) >> -{ >> - struct sched_entity *se = &p->se; >> - >> - set_task_rq(p, task_cpu(p)); >> - se->depth = se->parent ? se->parent->depth + 1 : 0; >> -} >> - >> -static void task_move_group_fair(struct task_struct *p) >> +static void task_change_group_fair(struct task_struct *p) >> { >> detach_task_cfs_rq(p); >> set_task_rq(p, task_cpu(p)); >> @@ -11451,19 +11435,6 @@ static void task_move_group_fair(struct task_struct *p) >> attach_task_cfs_rq(p); >> } >> >> -static void task_change_group_fair(struct task_struct *p, int type) >> -{ >> - switch (type) { >> - case TASK_SET_GROUP: >> - task_set_group_fair(p); >> - break; >> - >> - case TASK_MOVE_GROUP: >> - task_move_group_fair(p); >> - break; >> - } >> -} >> - >> void free_fair_sched_group(struct task_group *tg) >> { >> int i; >> diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h >> index 317480d535b0..8433e22da3ab 100644 >> --- a/kernel/sched/sched.h >> +++ b/kernel/sched/sched.h >> @@ -1901,6 +1901,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu) >> set_task_rq_fair(&p->se, p->se.cfs_rq, tg->cfs_rq[cpu]); >> p->se.cfs_rq = tg->cfs_rq[cpu]; >> p->se.parent = tg->se[cpu]; >> + p->se.depth = tg->se[cpu] ? tg->se[cpu]->depth + 1 : 0; >> #endif >> >> #ifdef CONFIG_RT_GROUP_SCHED >> @@ -2163,11 +2164,8 @@ struct sched_class { >> >> void (*update_curr)(struct rq *rq); >> >> -#define TASK_SET_GROUP 0 >> -#define TASK_MOVE_GROUP 1 >> - >> #ifdef CONFIG_FAIR_GROUP_SCHED >> - void (*task_change_group)(struct task_struct *p, int type); >> + void (*task_change_group)(struct task_struct *p); >> #endif >> }; >> >> -- >> 2.36.1 >>
| |