Received-SPF: pass (google.com: domain of linux-kernel+bounces-60279-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <d9dcd170-0ceb-489b-8df3-3fe35e9b4251@redhat.com>
Date: Fri, 9 Feb 2024 23:34:47 -0500
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH] cpuset: remove /proc/PID/cpuset
Content-Language: en-US
To: "brookxu.cn" <brookxu.cn@gmail.com>, lizefan.x@bytedance.com,
 tj@kernel.org, hannes@cmpxchg.org
Cc: linux-kernel@vger.kernel.org
References: <20240202104911.125894-1-brookxu.cn@gmail.com>
From: Waiman Long <longman@redhat.com>
In-Reply-To: <20240202104911.125894-1-brookxu.cn@gmail.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2/2/24 05:49, brookxu.cn wrote:
> From: Chunguang Xu <chunguang.xu@shopee.com>
>
> Now we can get all cgroup paths from /proc/PID/cgroup for
> a long time, so it maybe useless to keep /proc/PID/cpuset,
> besides the path get from /proc/PID/cpuset is not consistent
> with /proc/PID/cgroup in default hierarchy, so now we may
> can safely remove /proc/PID/cpuset to avoid the mismatch.

With cgroup v2, the value of /proc/PID/cgroup may not match 
/proc/PID/cpuset. That is the expected behavior due to the way cgroup v2 
works. For example,

Root [CS] --> A [CS] --> B --> C where [CS] means cpuset controller is 
enabled at that cgroup.

If a process P is in cgroup C, /proc/PID/cgroup will show /A/B/C. 
However, /proc/PID/cpuset will show /A because it is where the cpuset 
controller setting will apply to process P. Also removing an existing 
procfs file may break an existing application that relies on it. So we 
don't do that unless there is a strong reason to do so.

NAK

>
> root@test:~# cat /proc/1186/cgroup
> 0::/system.slice/lmeter.service
>
> root@test:~# cat /proc/1186/cpuset
> /system.slice
>
> Signed-off-by: Chunguang Xu <chunguang.xu@shopee.com>
> ---
>   arch/mips/configs/sb1250_swarm_defconfig |  1 -
>   arch/sh/configs/sdk7786_defconfig        |  1 -
>   arch/sh/configs/urquell_defconfig        |  1 -
>   fs/proc/base.c                           |  6 ----
>   kernel/cgroup/cpuset.c                   | 40 ------------------------
>   5 files changed, 49 deletions(-)
>
> diff --git a/arch/mips/configs/sb1250_swarm_defconfig b/arch/mips/configs/sb1250_swarm_defconfig
> index ce855b644bb0..2fbea9c604df 100644
> --- a/arch/mips/configs/sb1250_swarm_defconfig
> +++ b/arch/mips/configs/sb1250_swarm_defconfig
> @@ -3,7 +3,6 @@ CONFIG_HIGH_RES_TIMERS=y
>   CONFIG_LOG_BUF_SHIFT=15
>   CONFIG_CGROUPS=y
>   CONFIG_CPUSETS=y
> -# CONFIG_PROC_PID_CPUSET is not set
>   CONFIG_CGROUP_CPUACCT=y
>   CONFIG_NAMESPACES=y
>   CONFIG_RELAY=y
> diff --git a/arch/sh/configs/sdk7786_defconfig b/arch/sh/configs/sdk7786_defconfig
> index 7b427c17fbfe..6fe340d4860c 100644
> --- a/arch/sh/configs/sdk7786_defconfig
> +++ b/arch/sh/configs/sdk7786_defconfig
> @@ -13,7 +13,6 @@ CONFIG_CGROUP_DEBUG=y
>   CONFIG_CGROUP_FREEZER=y
>   CONFIG_CGROUP_DEVICE=y
>   CONFIG_CPUSETS=y
> -# CONFIG_PROC_PID_CPUSET is not set
>   CONFIG_CGROUP_CPUACCT=y
>   CONFIG_CGROUP_MEMCG=y
>   CONFIG_CGROUP_SCHED=y
> diff --git a/arch/sh/configs/urquell_defconfig b/arch/sh/configs/urquell_defconfig
> index 445bb451a5ec..c960221a0549 100644
> --- a/arch/sh/configs/urquell_defconfig
> +++ b/arch/sh/configs/urquell_defconfig
> @@ -11,7 +11,6 @@ CONFIG_CGROUP_DEBUG=y
>   CONFIG_CGROUP_FREEZER=y
>   CONFIG_CGROUP_DEVICE=y
>   CONFIG_CPUSETS=y
> -# CONFIG_PROC_PID_CPUSET is not set
>   CONFIG_CGROUP_CPUACCT=y
>   CONFIG_CGROUP_MEMCG=y
>   CONFIG_CGROUP_SCHED=y
> diff --git a/fs/proc/base.c b/fs/proc/base.c
> index 98a031ac2648..8dcd23b9212a 100644
> --- a/fs/proc/base.c
> +++ b/fs/proc/base.c
> @@ -3309,9 +3309,6 @@ static const struct pid_entry tgid_base_stuff[] = {
>   #ifdef CONFIG_LATENCYTOP
>   	REG("latency",  S_IRUGO, proc_lstats_operations),
>   #endif
> -#ifdef CONFIG_PROC_PID_CPUSET
> -	ONE("cpuset",     S_IRUGO, proc_cpuset_show),
> -#endif
>   #ifdef CONFIG_CGROUPS
>   	ONE("cgroup",  S_IRUGO, proc_cgroup_show),
>   #endif
> @@ -3658,9 +3655,6 @@ static const struct pid_entry tid_base_stuff[] = {
>   #ifdef CONFIG_LATENCYTOP
>   	REG("latency",  S_IRUGO, proc_lstats_operations),
>   #endif
> -#ifdef CONFIG_PROC_PID_CPUSET
> -	ONE("cpuset",    S_IRUGO, proc_cpuset_show),
> -#endif
>   #ifdef CONFIG_CGROUPS
>   	ONE("cgroup",  S_IRUGO, proc_cgroup_show),
>   #endif
> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
> index ba36c073304a..908cd7e6efa8 100644
> --- a/kernel/cgroup/cpuset.c
> +++ b/kernel/cgroup/cpuset.c
> @@ -5066,46 +5066,6 @@ void __cpuset_memory_pressure_bump(void)
>   	rcu_read_unlock();
>   }
>   
> -#ifdef CONFIG_PROC_PID_CPUSET
> -/*
> - * proc_cpuset_show()
> - *  - Print tasks cpuset path into seq_file.
> - *  - Used for /proc/<pid>/cpuset.
> - *  - No need to task_lock(tsk) on this tsk->cpuset reference, as it
> - *    doesn't really matter if tsk->cpuset changes after we read it,
> - *    and we take cpuset_mutex, keeping cpuset_attach() from changing it
> - *    anyway.
> - */
> -int proc_cpuset_show(struct seq_file *m, struct pid_namespace *ns,
> -		     struct pid *pid, struct task_struct *tsk)
> -{
> -	char *buf;
> -	struct cgroup_subsys_state *css;
> -	int retval;
> -
> -	retval = -ENOMEM;
> -	buf = kmalloc(PATH_MAX, GFP_KERNEL);
> -	if (!buf)
> -		goto out;
> -
> -	css = task_get_css(tsk, cpuset_cgrp_id);
> -	retval = cgroup_path_ns(css->cgroup, buf, PATH_MAX,
> -				current->nsproxy->cgroup_ns);
> -	css_put(css);
> -	if (retval == -E2BIG)
> -		retval = -ENAMETOOLONG;
> -	if (retval < 0)
> -		goto out_free;
> -	seq_puts(m, buf);
> -	seq_putc(m, '\n');
> -	retval = 0;
> -out_free:
> -	kfree(buf);
> -out:
> -	return retval;
> -}
> -#endif /* CONFIG_PROC_PID_CPUSET */
> -
>   /* Display task mems_allowed in /proc/<pid>/status file. */
>   void cpuset_task_status_allowed(struct seq_file *m, struct task_struct *task)
>   {