Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Mon, 14 Feb 2022 09:17:12 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Peter Zijlstra <peterz@infradead.org>
Cc:     Linus Torvalds <torvalds@linux-foundation.org>,
        Borislav Petkov <bp@suse.de>,
        Tadeusz Struk <tadeusz.struk@linaro.org>,
        x86-ml <x86@kernel.org>, lkml <linux-kernel@vger.kernel.org>,
        zhangqiao22@huawei.com, dietmar.eggemann@arm.com
Subject: Re: [GIT PULL] sched/urgent for 5.17-rc4
Message-ID: <YgqquDnQe3SihgJU@slm.duckdns.org>
References: <Ygj7feK+vdtPw6zj@zn.tnic>
 <CAHk-=wiHUWHHcPLCvyXQKf2wbL3L1SOQSGVuCdf-py6QZGnuqQ@mail.gmail.com>
 <YgoWoh6pIzlsQx6d@hirez.programming.kicks-ass.net>
 <YgoeCbwj5mbCR0qA@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YgoeCbwj5mbCR0qA@hirez.programming.kicks-ass.net>
Precedence: bulk

Hello, Peter.

On Mon, Feb 14, 2022 at 10:16:57AM +0100, Peter Zijlstra wrote:
> diff --git a/kernel/fork.c b/kernel/fork.c
> index d75a528f7b21..05faebafe2b5 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -2266,6 +2266,13 @@ static __latent_entropy struct task_struct *copy_process(
>  	if (retval)
>  		goto bad_fork_put_pidfd;
>  
> +	/*
> +	 * Now that the cgroups are pinned, re-clone the parent cgroup and put
> +	 * the new task on the correct runqueue. All this *before* the task
> +	 * becomes visible.
> +	 */
> +	sched_cgroup_fork(p, args);

Would it be less confusing to comment that this isn't ->can_fork() because
scheduler task_group needs to be initialized for autogroup even when cgroup
is disabled and maybe name it sched_cgroup_can_fork() even if it always
succeeds?

> +void sched_cgroup_fork(struct task_struct *p, struct kernel_clone_args *kargs)
>  {
>  	unsigned long flags;
> -#ifdef CONFIG_CGROUP_SCHED
> -	struct task_group *tg;
> -#endif
>  
> +	/*
> +	 * Because we're not yet on the pid-hash, p->pi_lock isn't strictly
> +	 * required yet, but lockdep gets upset if rules are violated.
> +	 */
>  	raw_spin_lock_irqsave(&p->pi_lock, flags);
>  #ifdef CONFIG_CGROUP_SCHED
> -	tg = container_of(kargs->cset->subsys[cpu_cgrp_id],
> -			  struct task_group, css);
> -	p->sched_task_group = autogroup_task_group(p, tg);
> +	if (1) {
> +		struct task_group *tg;
> +		tg = container_of(kargs->cset->subsys[cpu_cgrp_id],
> +				  struct task_group, css);
> +		tg = autogroup_task_group(p, tg);
> +		p->sched_task_group = autogroup_task_group(p, tg);
> +	}

I suppose the double autogroup_task_group() call is unintentional?

Otherwise, looks good to me. The only requirement from cgroup side is that
the membership should be initialized between ->can_fork() and ->fork()
inclusively, and sans autogroup this would have been done as a part of
->can_fork() so the proposed change makes sense to me.

Thanks.

-- 
tejun