| Message ID | 20250304153801.597907-2-mkoutny@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | cgroup v1 deprecation warnings | expand |
| Context | Check | Description |
|---|---|---|
| shin/vmtest-linus-master-PR | success | PR summary |
| shin/vmtest-linus-master-VM_Test-0 | success | Logs for build-kernel |
On 3/4/25 10:37 AM, Michal Koutný wrote: > These two v1 feature have analogues in cgroup v2. > > Signed-off-by: Michal Koutný <mkoutny@suse.com> > --- > kernel/cgroup/cpuset-v1.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/kernel/cgroup/cpuset-v1.c b/kernel/cgroup/cpuset-v1.c > index 25c1d7b77e2f2..3e81ac76578c7 100644 > --- a/kernel/cgroup/cpuset-v1.c > +++ b/kernel/cgroup/cpuset-v1.c > @@ -430,12 +430,14 @@ static int cpuset_write_u64(struct cgroup_subsys_state *css, struct cftype *cft, > retval = cpuset_update_flag(CS_MEM_HARDWALL, cs, val); > break; > case FILE_SCHED_LOAD_BALANCE: > + pr_warn_once("cpuset.%s is deprecated, use cpus.partition instead\n", cft->name); Should you use the full name "cpuset.cpus.partition" instead? > retval = cpuset_update_flag(CS_SCHED_LOAD_BALANCE, cs, val); > break; > case FILE_MEMORY_MIGRATE: > retval = cpuset_update_flag(CS_MEMORY_MIGRATE, cs, val); > break; > case FILE_MEMORY_PRESSURE_ENABLED: > + pr_warn_once("cpuset.%s is deprecated, use memory.pressure instead\n", cft->name); memory.pressure depends on CONFIG_PSI, so it may not available in some cases. My suggestion is to add a "if available" suffix. I do have some concern with the use of pr_warn*() because some users may attempt to use the panic_on_warn command line option. Cheers, Longman
On Tue, Mar 04, 2025 at 11:19:00AM -0500, Waiman Long wrote: ... > I do have some concern with the use of pr_warn*() because some users may > attempt to use the panic_on_warn command line option. Yeah, let's print these as info. Thanks.
On Tue, Mar 04, 2025 at 06:52:41AM -1000, Tejun Heo <tj@kernel.org> wrote: > On Tue, Mar 04, 2025 at 11:19:00AM -0500, Waiman Long wrote: > ... > > I do have some concern with the use of pr_warn*() because some users may > > attempt to use the panic_on_warn command line option. > > Yeah, let's print these as info. The intention is _not_ to cause panic by any of this this. Note the difference between WARN() and pr_warn() (only the former panics). Warn level has precedent in mm/memcontrol-v1.c already. Michal
On 3/4/25 12:10 PM, Michal Koutný wrote: > On Tue, Mar 04, 2025 at 06:52:41AM -1000, Tejun Heo <tj@kernel.org> wrote: >> On Tue, Mar 04, 2025 at 11:19:00AM -0500, Waiman Long wrote: >> ... >>> I do have some concern with the use of pr_warn*() because some users may >>> attempt to use the panic_on_warn command line option. >> Yeah, let's print these as info. > The intention is _not_ to cause panic by any of this this. > Note the difference between WARN() and pr_warn() (only the former > panics). > Warn level has precedent in mm/memcontrol-v1.c already. I think you are right. The pr_warn() function should not cause a panic. I have the misconception that pr_warn() will be affected by panic_on_warn before. In that case, I have no objection to use pr_warn(). Thanks, Longman
On Tue, Mar 04, 2025 at 12:33:32PM -0500, Waiman Long wrote: > > On 3/4/25 12:10 PM, Michal Koutný wrote: > > On Tue, Mar 04, 2025 at 06:52:41AM -1000, Tejun Heo <tj@kernel.org> wrote: > > > On Tue, Mar 04, 2025 at 11:19:00AM -0500, Waiman Long wrote: > > > ... > > > > I do have some concern with the use of pr_warn*() because some users may > > > > attempt to use the panic_on_warn command line option. > > > Yeah, let's print these as info. > > The intention is _not_ to cause panic by any of this this. > > Note the difference between WARN() and pr_warn() (only the former > > panics). > > Warn level has precedent in mm/memcontrol-v1.c already. > > I think you are right. The pr_warn() function should not cause a panic. I > have the misconception that pr_warn() will be affected by panic_on_warn > before. In that case, I have no objection to use pr_warn(). I'm apprehensive about adding warning messages which may be triggered consistently without anything end users can do about them. I think that deprecation messages, unless such deprecation is immediate and would have direct consequences on how the system can be used, should be informational. Thanks.
On Tue, Mar 04, 2025 at 08:04:06AM -1000, Tejun Heo <tj@kernel.org> wrote: > I'm apprehensive about adding warning messages which may be triggered > consistently without anything end users can do about them. That means you'd distinguish RE (replacement exists) vs DN (dropped as non-ideal) categories? > I think that deprecation messages, unless such deprecation is > immediate and would have direct consequences on how the system can be > used, should be informational. I could subscribe to that if there weren't so many other places to evaluate: $ git grep -i "pr_warn.*deprec" torvalds/master -- | wc -l 62 $ git grep -i "pr_info.*deprec" torvalds/master -- | wc -l 2 So is the disctinction worth the hassle? Michal
Hello, On Wed, Mar 05, 2025 at 11:12:21AM +0100, Michal Koutný wrote: > On Tue, Mar 04, 2025 at 08:04:06AM -1000, Tejun Heo <tj@kernel.org> wrote: > > I'm apprehensive about adding warning messages which may be triggered > > consistently without anything end users can do about them. > > That means you'd distinguish RE (replacement exists) vs DN (dropped as > non-ideal) categories? I don't think I am. I'm just concerned about emitting warn messages on every boot without users being able to do anything about them. > > I think that deprecation messages, unless such deprecation is > > immediate and would have direct consequences on how the system can be > > used, should be informational. > > I could subscribe to that if there weren't so many other places to > evaluate: > $ git grep -i "pr_warn.*deprec" torvalds/master -- | wc -l > 62 > $ git grep -i "pr_info.*deprec" torvalds/master -- | wc -l > 2 > > So is the disctinction worth the hassle? Well, not all deprecations are the same. If users are stuck on cgroup1, they can be really stuck - there can be a tall stack of software with dependencies that users can't do much about, at least not immediately. We will deprecate cgroup1 but this is going to be a long stretched out process at the end of which we should be fairly comfortable in stating that there aren't major users left which are stuck on cgroup1. It's almost certain that that future won't arrive in, say, three years. Five years may be too ambitious too but let's say that at that point we are relatively sure that most platforms have moved on (but there may still be users on older versions of those platforms). Maybe it'd make sense to increase the deprecation warning temperature by then to warn and drain existing users and maybe after a few years we'd actually be able to drop cgroup1 support. So, I don't want to be emitting warnings on every boot for the good part of a decade on every boot for those users. Doing so feels silly and annoying to me. Let's inform that it's coming down the pipeline but I personally don't want to be warned by something that's close to a decade out. Thanks.
diff --git a/kernel/cgroup/cpuset-v1.c b/kernel/cgroup/cpuset-v1.c index 25c1d7b77e2f2..3e81ac76578c7 100644 --- a/kernel/cgroup/cpuset-v1.c +++ b/kernel/cgroup/cpuset-v1.c @@ -430,12 +430,14 @@ static int cpuset_write_u64(struct cgroup_subsys_state *css, struct cftype *cft, retval = cpuset_update_flag(CS_MEM_HARDWALL, cs, val); break; case FILE_SCHED_LOAD_BALANCE: + pr_warn_once("cpuset.%s is deprecated, use cpus.partition instead\n", cft->name); retval = cpuset_update_flag(CS_SCHED_LOAD_BALANCE, cs, val); break; case FILE_MEMORY_MIGRATE: retval = cpuset_update_flag(CS_MEMORY_MIGRATE, cs, val); break; case FILE_MEMORY_PRESSURE_ENABLED: + pr_warn_once("cpuset.%s is deprecated, use memory.pressure instead\n", cft->name); cpuset_memory_pressure_enabled = !!val; break; case FILE_SPREAD_PAGE:
These two v1 feature have analogues in cgroup v2. Signed-off-by: Michal Koutný <mkoutny@suse.com> --- kernel/cgroup/cpuset-v1.c | 2 ++ 1 file changed, 2 insertions(+)