Re: [GIT PULL] sched/urgent for 5.17-rc4

[Tejun Heo]

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