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;
Sender: Tejun Heo <htejun@gmail.com>
Date:   Fri, 5 May 2023 09:04:37 -1000
From:   Tejun Heo <tj@kernel.org>
To:     Michal =?iso-8859-1?Q?Koutn=FD?= <mkoutny@suse.com>
Cc:     cgroups@vger.kernel.org, linux-kernel@vger.kernel.org,
        Zefan Li <lizefan.x@bytedance.com>,
        Johannes Weiner <hannes@cmpxchg.org>,
        Benjamin Berg <benjamin@sipsolutions.net>
Subject: Re: [RFC PATCH] cgroup: Return error when attempting to migrate a
 zombie process
Message-ID: <ZFVTRdP6eK1soMVR@slm.duckdns.org>
References: <20230503125359.14789-1-mkoutny@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20230503125359.14789-1-mkoutny@suse.com>
Precedence: bulk

Hello,

On Wed, May 03, 2023 at 02:53:59PM +0200, Michal Koutn? wrote:
> Zombies aren't migrated. However, return value of a migration write may
> suggest a zombie process was migrated and causing confusion about lack
> of cgroup.events:populated between origin and target cgroups (e.g.
> target cgroup rmdir).
> 
> Notify the users about no effect of their action by a return value.
> (update_dfl_csses migration of zombies still silently passes since it is
> not meant to be user-visible migration anyway.)
> 
> Suggested-by: Benjamin Berg <benjamin@sipsolutions.net>
> Signed-off-by: Michal Koutn? <mkoutny@suse.com>
> ---
>  kernel/cgroup/cgroup.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> Reasons for RFC:
> 1) Some users may notice the change,
> 2) EINVAL vs ESCHR,
> 3) add a selftest?
> 
> diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
> index 625d7483951c..306547dd7b76 100644
> --- a/kernel/cgroup/cgroup.c
> +++ b/kernel/cgroup/cgroup.c
> @@ -2968,7 +2968,8 @@ struct task_struct *cgroup_procs_write_start(char *buf, bool threadgroup,
>  	 * become trapped in a cpuset, or RT kthread may be born in a
>  	 * cgroup with no rt_runtime allocated.  Just say no.
>  	 */
> -	if (tsk->no_cgroup_migration || (tsk->flags & PF_NO_SETAFFINITY)) {
> +	if (tsk->no_cgroup_migration || (tsk->flags & PF_NO_SETAFFINITY) ||
> +	    !atomic_read(&tsk->signal->live)) {

This seems racy to me. The liveness state can change between here and the
PF_EXITING check in cgroup_migrate_add_task(), right? Wouldn't it be better
to just track how many tasks are tracked and return -ESRCH if none was
migrated?

Thanks.

-- 
tejun