Re: [External] Re: [PATCH 3/8] sched/fair: remove redundant cpu_cgrp_subsys->fork()

[Peter Zijlstra <peterz@infradead.org>]

On Mon, Jul 11, 2022 at 09:02:07PM +0800, Chengming Zhou wrote:
> On 2022/7/11 15:35, Peter Zijlstra wrote:
> > On Sat, Jul 09, 2022 at 11:13:48PM +0800, Chengming Zhou 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 patch's change, task_change_group_fair() only need to
> >> care about task cgroup migration, make the code much simplier.
> > 
> > This:
> > 
> >> This patch also move the task se depth setting to set_task_rq(), which
> >> will set correct depth for the new task se in sched_cgroup_fork().
> >>
> >> The se depth setting in attach_entity_cfs_rq() is removed since
> >> set_task_rq() is a better place to do this when task moves across
> >> CPUs/groups.
> > 
> > really should have been it's own patch. And this actually scares me. Did
> > you test with priority inheritance bumping the task to FIFO while things
> > change?
> > 
> > This has nothing to do with fork().
> 
> Ok, will put this in another patch, so this patch still need this line:
> 
>   p->se.depth = tg->se[cpu] ? tg->se[cpu]->depth + 1 : 0;
> 
> in set_task_rq() to set depth for new forked task.

That would suggest you ordered your patches wrong.

> I didn't test with "priority inheritance bumping the task to FIFO" case,
> do you mean the rt_mutex_setprio() bump a fair task to FIFO?
> 
> Sorry, I don't get how removing depth setting in attach_entity_cfs_rq()
> affect that. Could you explain more so I can test it?

Well, if you look at the commit that introduced that code:

  eb7a59b2c888 ("sched/fair: Reset se-depth when task switched to FAIR")

then it's clear that the original problem was the task temporarily not
being in the fair class. The most common way for that to be so is
through PI.