Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <00d2373c-4b80-64a7-740c-dae87a565fe3@bytedance.com>
Date:   Thu, 14 Jul 2022 21:06:04 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.0.1
Subject: Re: [External] Re: [PATCH v2 04/10] sched/fair: remove redundant
 cpu_cgrp_subsys->fork()
Content-Language: en-US
To:     Dietmar Eggemann <dietmar.eggemann@arm.com>, mingo@redhat.com,
        peterz@infradead.org, vincent.guittot@linaro.org,
        rostedt@goodmis.org, bsegall@google.com, vschneid@redhat.com
Cc:     linux-kernel@vger.kernel.org
References: <20220713040430.25778-1-zhouchengming@bytedance.com>
 <20220713040430.25778-5-zhouchengming@bytedance.com>
 <fa74892c-2956-6ddf-e022-398b89056e5a@arm.com>
From:   Chengming Zhou <zhouchengming@bytedance.com>
In-Reply-To: <fa74892c-2956-6ddf-e022-398b89056e5a@arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 2022/7/14 20:31, Dietmar Eggemann wrote:
> On 13/07/2022 06:04, 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()
> 
> Don't we set the task group (`p->sched_task_group = tg`) now directly in
> sched_cgroup_fork() and not in __set_task_cpu()?

Correct, it's my fault, what I want to say is "__set_task_cpu() --> set_task_rq()".
I will fix it next version.

> 
>>     __set_task_cpu	--> set task group
>>   cgroup_post_fork()
>>     ss->fork() := cpu_cgroup_fork()	--> set again
> 
> nit pick:
> 
> cpu_cgroup_fork() -> sched_change_group(..., TASK_SET_GROUP)

Ok, will add this.

Thanks.

> 
>> After this patch's change, task_change_group_fair() only need to
>> care about task cgroup migration, make the code much simplier.
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> Reviewed-by: Vincent Guittot <vincent.guittot@linaro.org>
> 
> LGTM too.
> 
> Reviewed-by: Dietmar Eggemann <dietmar.eggemann@arm.com>
> 
> [...]