[PATCH 1/2 cgroup/for-6.1] cgroup: Improve cftype add/rm error handling

[PATCH 1/2 cgroup/for-6.1] cgroup: Improve cftype add/rm error handling

[Tejun Heo]

Let's track whether a cftype is currently added or not using a new flag
__CFTYPE_ADDED so that duplicate operations can be failed safely and
consistently allow using empty cftypes.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
---
Hello,

If no one objects, imma apply these two cgroup file handling cleanup patches
to cgroup/for-6.1 in a few days.

Thanks.

 include/linux/cgroup-defs.h |    1 +
 kernel/cgroup/cgroup.c      |   27 ++++++++++++++++++++-------
 2 files changed, 21 insertions(+), 7 deletions(-)

--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -131,6 +131,7 @@ enum {
 	/* internal flags, do not use outside cgroup core proper */
 	__CFTYPE_ONLY_ON_DFL	= (1 << 16),	/* only on default hierarchy */
 	__CFTYPE_NOT_ON_DFL	= (1 << 17),	/* not on default hierarchy */
+	__CFTYPE_ADDED		= (1 << 18),
 };
 
 /*
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4166,19 +4166,26 @@ static void cgroup_exit_cftypes(struct c
 		cft->ss = NULL;
 
 		/* revert flags set by cgroup core while adding @cfts */
-		cft->flags &= ~(__CFTYPE_ONLY_ON_DFL | __CFTYPE_NOT_ON_DFL);
+		cft->flags &= ~(__CFTYPE_ONLY_ON_DFL | __CFTYPE_NOT_ON_DFL |
+				__CFTYPE_ADDED);
 	}
 }
 
 static int cgroup_init_cftypes(struct cgroup_subsys *ss, struct cftype *cfts)
 {
 	struct cftype *cft;
+	int ret = 0;
 
 	for (cft = cfts; cft->name[0] != '\0'; cft++) {
 		struct kernfs_ops *kf_ops;
 
 		WARN_ON(cft->ss || cft->kf_ops);
 
+		if (cft->flags & __CFTYPE_ADDED) {
+			ret = -EBUSY;
+			break;
+		}
+
 		if ((cft->flags & CFTYPE_PRESSURE) && !cgroup_psi_enabled())
 			continue;
 
@@ -4194,26 +4201,26 @@ static int cgroup_init_cftypes(struct cg
 		if (cft->max_write_len && cft->max_write_len != PAGE_SIZE) {
 			kf_ops = kmemdup(kf_ops, sizeof(*kf_ops), GFP_KERNEL);
 			if (!kf_ops) {
-				cgroup_exit_cftypes(cfts);
-				return -ENOMEM;
+				ret = -ENOMEM;
+				break;
 			}
 			kf_ops->atomic_write_len = cft->max_write_len;
 		}
 
 		cft->kf_ops = kf_ops;
 		cft->ss = ss;
+		cft->flags |= __CFTYPE_ADDED;
 	}
 
-	return 0;
+	if (ret)
+		cgroup_exit_cftypes(cfts);
+	return ret;
 }
 
 static int cgroup_rm_cftypes_locked(struct cftype *cfts)
 {
 	lockdep_assert_held(&cgroup_mutex);
 
-	if (!cfts || !cfts[0].ss)
-		return -ENOENT;
-
 	list_del(&cfts->node);
 	cgroup_apply_cftypes(cfts, false);
 	cgroup_exit_cftypes(cfts);
@@ -4235,6 +4242,12 @@ int cgroup_rm_cftypes(struct cftype *cft
 {
 	int ret;
 
+	if (!cfts || cfts[0].name[0] == '\0')
+		return 0;
+
+	if (!(cfts[0].flags & __CFTYPE_ADDED))
+		return -ENOENT;
+
 	mutex_lock(&cgroup_mutex);
 	ret = cgroup_rm_cftypes_locked(cfts);
 	mutex_unlock(&cgroup_mutex);

[PATCH 2/2 cgroup/for-6.1] cgroup: Remove CFTYPE_PRESSURE

[Tejun Heo]

CFTYPE_PRESSURE is used to flag PSI related files so that they are not
created if PSI is disabled during boot. It's a bit weird to use a generic
flag to mark a specific file type. Let's instead move the PSI files into its
own cftypes array and add/rm them conditionally. This is a bit more code but
cleaner.

No userland visible changes.

Signed-off-by: Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
Cc: Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>
---
 include/linux/cgroup-defs.h |    1 
 kernel/cgroup/cgroup.c      |   64 +++++++++++++++++++++++++-------------------
 2 files changed, 37 insertions(+), 28 deletions(-)

--- a/include/linux/cgroup-defs.h
+++ b/include/linux/cgroup-defs.h
@@ -126,7 +126,6 @@ enum {
 	CFTYPE_NO_PREFIX	= (1 << 3),	/* (DON'T USE FOR NEW FILES) no subsys prefix */
 	CFTYPE_WORLD_WRITABLE	= (1 << 4),	/* (DON'T USE FOR NEW FILES) S_IWUGO */
 	CFTYPE_DEBUG		= (1 << 5),	/* create when cgroup_debug */
-	CFTYPE_PRESSURE		= (1 << 6),	/* only if pressure feature is enabled */
 
 	/* internal flags, do not use outside cgroup core proper */
 	__CFTYPE_ONLY_ON_DFL	= (1 << 16),	/* only on default hierarchy */
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -217,6 +217,7 @@ struct cgroup_namespace init_cgroup_ns =
 
 static struct file_system_type cgroup2_fs_type;
 static struct cftype cgroup_base_files[];
+static struct cftype cgroup_psi_files[];
 
 /* cgroup optional features */
 enum cgroup_opt_features {
@@ -1689,12 +1690,16 @@ static void css_clear_dir(struct cgroup_
 	css->flags &= ~CSS_VISIBLE;
 
 	if (!css->ss) {
-		if (cgroup_on_dfl(cgrp))
-			cfts = cgroup_base_files;
-		else
-			cfts = cgroup1_base_files;
-
-		cgroup_addrm_files(css, cgrp, cfts, false);
+		if (cgroup_on_dfl(cgrp)) {
+			cgroup_addrm_files(css, cgrp,
+					   cgroup_base_files, false);
+			if (cgroup_psi_enabled())
+				cgroup_addrm_files(css, cgrp,
+						   cgroup_psi_files, false);
+		} else {
+			cgroup_addrm_files(css, cgrp,
+					   cgroup1_base_files, false);
+		}
 	} else {
 		list_for_each_entry(cfts, &css->ss->cfts, node)
 			cgroup_addrm_files(css, cgrp, cfts, false);
@@ -1717,14 +1722,22 @@ static int css_populate_dir(struct cgrou
 		return 0;
 
 	if (!css->ss) {
-		if (cgroup_on_dfl(cgrp))
-			cfts = cgroup_base_files;
-		else
-			cfts = cgroup1_base_files;
-
-		ret = cgroup_addrm_files(&cgrp->self, cgrp, cfts, true);
-		if (ret < 0)
-			return ret;
+		if (cgroup_on_dfl(cgrp)) {
+			ret = cgroup_addrm_files(&cgrp->self, cgrp,
+						 cgroup_base_files, true);
+			if (ret < 0)
+				return ret;
+
+			if (cgroup_psi_enabled()) {
+				ret = cgroup_addrm_files(&cgrp->self, cgrp,
+							 cgroup_psi_files, true);
+				if (ret < 0)
+					return ret;
+			}
+		} else {
+			cgroup_addrm_files(css, cgrp,
+					   cgroup1_base_files, true);
+		}
 	} else {
 		list_for_each_entry(cfts, &css->ss->cfts, node) {
 			ret = cgroup_addrm_files(css, cgrp, cfts, true);
@@ -4100,8 +4113,6 @@ static int cgroup_addrm_files(struct cgr
 restart:
 	for (cft = cfts; cft != cft_end && cft->name[0] != '\0'; cft++) {
 		/* does cft->flags tell us to skip this file on @cgrp? */
-		if ((cft->flags & CFTYPE_PRESSURE) && !cgroup_psi_enabled())
-			continue;
 		if ((cft->flags & __CFTYPE_ONLY_ON_DFL) && !cgroup_on_dfl(cgrp))
 			continue;
 		if ((cft->flags & __CFTYPE_NOT_ON_DFL) && cgroup_on_dfl(cgrp))
@@ -4186,9 +4197,6 @@ static int cgroup_init_cftypes(struct cg
 			break;
 		}
 
-		if ((cft->flags & CFTYPE_PRESSURE) && !cgroup_psi_enabled())
-			continue;
-
 		if (cft->seq_start)
 			kf_ops = &cgroup_kf_ops;
 		else
@@ -5134,10 +5142,13 @@ static struct cftype cgroup_base_files[]
 		.name = "cpu.stat",
 		.seq_show = cpu_stat_show,
 	},
+	{ }	/* terminate */
+};
+
+static struct cftype cgroup_psi_files[] = {
 #ifdef CONFIG_PSI
 	{
 		.name = "io.pressure",
-		.flags = CFTYPE_PRESSURE,
 		.seq_show = cgroup_io_pressure_show,
 		.write = cgroup_io_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5145,7 +5156,6 @@ static struct cftype cgroup_base_files[]
 	},
 	{
 		.name = "memory.pressure",
-		.flags = CFTYPE_PRESSURE,
 		.seq_show = cgroup_memory_pressure_show,
 		.write = cgroup_memory_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5153,7 +5163,6 @@ static struct cftype cgroup_base_files[]
 	},
 	{
 		.name = "cpu.pressure",
-		.flags = CFTYPE_PRESSURE,
 		.seq_show = cgroup_cpu_pressure_show,
 		.write = cgroup_cpu_pressure_write,
 		.poll = cgroup_pressure_poll,
@@ -5921,6 +5930,7 @@ int __init cgroup_init(void)
 
 	BUILD_BUG_ON(CGROUP_SUBSYS_COUNT > 16);
 	BUG_ON(cgroup_init_cftypes(NULL, cgroup_base_files));
+	BUG_ON(cgroup_init_cftypes(NULL, cgroup_psi_files));
 	BUG_ON(cgroup_init_cftypes(NULL, cgroup1_base_files));
 
 	cgroup_rstat_boot();
@@ -6792,9 +6802,6 @@ static ssize_t show_delegatable_files(st
 		if (!(cft->flags & CFTYPE_NS_DELEGATABLE))
 			continue;
 
-		if ((cft->flags & CFTYPE_PRESSURE) && !cgroup_psi_enabled())
-			continue;
-
 		if (prefix)
 			ret += snprintf(buf + ret, size - ret, "%s.", prefix);
 
@@ -6814,8 +6821,11 @@ static ssize_t delegate_show(struct kobj
 	int ssid;
 	ssize_t ret = 0;
 
-	ret = show_delegatable_files(cgroup_base_files, buf, PAGE_SIZE - ret,
-				     NULL);
+	ret = show_delegatable_files(cgroup_base_files, buf + ret,
+				     PAGE_SIZE - ret, NULL);
+	if (cgroup_psi_enabled())
+		ret += show_delegatable_files(cgroup_psi_files, buf + ret,
+					      PAGE_SIZE - ret, NULL);
 
 	for_each_subsys(ss, ssid)
 		ret += show_delegatable_files(ss->dfl_cftypes, buf + ret,

Re: [PATCH 1/2 cgroup/for-6.1] cgroup: Improve cftype add/rm error handling

[Michal Koutný]

Hello.

On Sun, Sep 04, 2022 at 11:09:53AM -1000, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> Let's track whether a cftype is currently added or not using a new flag
> __CFTYPE_ADDED

IIUC, the flag is equal to (cft->ss || cft->kf_ops), particularly the
information is carried in cfs->kf_ops too.

Is the effect of cgroup_init_cftypes proper setup of cft->kf_ops?
I.e. isn't it simpler to just check that field (without the new flag)?

(No objection to current form, just asking whether I understand the
impact.)

Thanks,
Michal

Re: [PATCH 1/2 cgroup/for-6.1] cgroup: Improve cftype add/rm error handling

[Tejun Heo]

Hello,

On Mon, Sep 05, 2022 at 03:14:35PM +0200, Michal Koutný wrote:
> IIUC, the flag is equal to (cft->ss || cft->kf_ops), particularly the
> information is carried in cfs->kf_ops too.

->ss will be NULL for core files even after added. ->kf_ops can be used
instead of the flag.

> Is the effect of cgroup_init_cftypes proper setup of cft->kf_ops?
> I.e. isn't it simpler to just check that field (without the new flag)?
> 
> (No objection to current form, just asking whether I understand the
> impact.)

I prefer having it as a separate flag because it's explicit and can be
tested together with other flags. It's a weak preference tho and I can go
either way if it bothers you much.

Thanks.

-- 
tejun

Re: [PATCH 1/2 cgroup/for-6.1] cgroup: Improve cftype add/rm error handling

[Michal Koutný]

[-- Attachment #1: Type: text/plain, Size: 1726 bytes --]

On Tue, Sep 06, 2022 at 07:25:08AM -1000, Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> I prefer having it as a separate flag because it's explicit and can be
> tested together with other flags. It's a weak preference tho and I can go
> either way if it bothers you much.

No trouble, please proceed with the new flag.

BTW, while I was just looking over the patch I noticed that in

@@ -1717,14 +1722,22 @@ static int css_populate_dir(struct cgrou
                return 0;

        if (!css->ss) {
-               if (cgroup_on_dfl(cgrp))
-                       cfts = cgroup_base_files;
-               else
-                       cfts = cgroup1_base_files;
-
-               ret = cgroup_addrm_files(&cgrp->self, cgrp, cfts, true);
-               if (ret < 0)
-                       return ret;
+               if (cgroup_on_dfl(cgrp)) {
+                       ret = cgroup_addrm_files(&cgrp->self, cgrp,
+                                                cgroup_base_files, true);
+                       if (ret < 0)
+                               return ret;
+
+                       if (cgroup_psi_enabled()) {
+                               ret = cgroup_addrm_files(&cgrp->self, cgrp,
+                                                        cgroup_psi_files, true);
+                               if (ret < 0)
+                                       return ret;

Before the return here, the function should revert the base files first (or
silence the return value to 0 if such a partial population is acceptable).

(Actually, it looks like the revert in the subsys branch is unnecessary as
callers of css_populate_dir() would issue css_clear_dir() upon failure
eventually.)


Thanks,
Michal

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/2 cgroup/for-6.1] cgroup: Improve cftype add/rm error handling

[Tejun Heo]

Hello,

On Tue, Sep 06, 2022 at 09:11:12PM +0200, Michal Koutný wrote:
> Before the return here, the function should revert the base files first (or
> silence the return value to 0 if such a partial population is acceptable).
> 
> (Actually, it looks like the revert in the subsys branch is unnecessary as
> callers of css_populate_dir() would issue css_clear_dir() upon failure
> eventually.)

Yeah, so, the contract there is a bit unusual in that on failure the helpers
don't need to cleanup after themselves as they'll get cleaned up together by
the caller when it nukes the cgroup which was being created. While a bit
unusual, it's simpler / safer this way, so...

Thanks.

-- 
tejun

Re: [PATCH 1/2 cgroup/for-6.1] cgroup: Improve cftype add/rm error handling

[Tejun Heo]

On Sun, Sep 04, 2022 at 11:09:53AM -1000, Tejun Heo wrote:
> Let's track whether a cftype is currently added or not using a new flag
> __CFTYPE_ADDED so that duplicate operations can be failed safely and
> consistently allow using empty cftypes.

Applying 1-2 to cgroup/for-6.1.

Thanks.

-- 
tejun