Received-SPF: pass (google.com: domain of linux-kernel+bounces-87843-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240202104911.125894-1-brookxu.cn@gmail.com> <d9dcd170-0ceb-489b-8df3-3fe35e9b4251@redhat.com>
In-Reply-To: <d9dcd170-0ceb-489b-8df3-3fe35e9b4251@redhat.com>
From: =?UTF-8?B?6K645pil5YWJ?= <brookxu.cn@gmail.com>
Date: Fri, 1 Mar 2024 10:43:16 +0800
Message-ID: <CADtkEefJCbijj7eS+L3kcMCBtHqze3oMkX4yXbfoJG1mT3YkQw@mail.gmail.com>
Subject: Re: [RFC PATCH] cpuset: remove /proc/PID/cpuset
To: Waiman Long <longman@redhat.com>
Cc: lizefan.x@bytedance.com, tj@kernel.org, hannes@cmpxchg.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi waiman:

Thanks for analyse in detail, we found some guy use /proc/PID/cpuset
to get the path and cause a live issue.

Thanks.

Waiman Long <longman@redhat.com> =E4=BA=8E2024=E5=B9=B42=E6=9C=8810=E6=97=
=A5=E5=91=A8=E5=85=AD 12:34=E5=86=99=E9=81=93=EF=BC=9A
>
> 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/confi=
gs/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=3Dy
> >   CONFIG_LOG_BUF_SHIFT=3D15
> >   CONFIG_CGROUPS=3Dy
> >   CONFIG_CPUSETS=3Dy
> > -# CONFIG_PROC_PID_CPUSET is not set
> >   CONFIG_CGROUP_CPUACCT=3Dy
> >   CONFIG_NAMESPACES=3Dy
> >   CONFIG_RELAY=3Dy
> > diff --git a/arch/sh/configs/sdk7786_defconfig b/arch/sh/configs/sdk778=
6_defconfig
> > index 7b427c17fbfe..6fe340d4860c 100644
> > --- a/arch/sh/configs/sdk7786_defconfig
> > +++ b/arch/sh/configs/sdk7786_defconfig
> > @@ -13,7 +13,6 @@ CONFIG_CGROUP_DEBUG=3Dy
> >   CONFIG_CGROUP_FREEZER=3Dy
> >   CONFIG_CGROUP_DEVICE=3Dy
> >   CONFIG_CPUSETS=3Dy
> > -# CONFIG_PROC_PID_CPUSET is not set
> >   CONFIG_CGROUP_CPUACCT=3Dy
> >   CONFIG_CGROUP_MEMCG=3Dy
> >   CONFIG_CGROUP_SCHED=3Dy
> > diff --git a/arch/sh/configs/urquell_defconfig b/arch/sh/configs/urquel=
l_defconfig
> > index 445bb451a5ec..c960221a0549 100644
> > --- a/arch/sh/configs/urquell_defconfig
> > +++ b/arch/sh/configs/urquell_defconfig
> > @@ -11,7 +11,6 @@ CONFIG_CGROUP_DEBUG=3Dy
> >   CONFIG_CGROUP_FREEZER=3Dy
> >   CONFIG_CGROUP_DEVICE=3Dy
> >   CONFIG_CPUSETS=3Dy
> > -# CONFIG_PROC_PID_CPUSET is not set
> >   CONFIG_CGROUP_CPUACCT=3Dy
> >   CONFIG_CGROUP_MEMCG=3Dy
> >   CONFIG_CGROUP_SCHED=3Dy
> > 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[] =
=3D {
> >   #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[] =
=3D {
> >   #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 =3D -ENOMEM;
> > -     buf =3D kmalloc(PATH_MAX, GFP_KERNEL);
> > -     if (!buf)
> > -             goto out;
> > -
> > -     css =3D task_get_css(tsk, cpuset_cgrp_id);
> > -     retval =3D cgroup_path_ns(css->cgroup, buf, PATH_MAX,
> > -                             current->nsproxy->cgroup_ns);
> > -     css_put(css);
> > -     if (retval =3D=3D -E2BIG)
> > -             retval =3D -ENAMETOOLONG;
> > -     if (retval < 0)
> > -             goto out_free;
> > -     seq_puts(m, buf);
> > -     seq_putc(m, '\n');
> > -     retval =3D 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_struc=
t *task)
> >   {
>