Skip to contentSkip to conversation

cpuset: Minor cgroup2 interface updates

PATCH In review Tejun Heo 3 participants 7 messages Nov 13, 2018 – Nov 20, 2018

Subsystems (3)
Documentation cgroup Control group - cpuset
lore
Share & tools
lore

Tejun Heo

author maintainer
#0 lore raw

PATCH 1/2 cpuset: Minor cgroup2 interface updates

From b1e3aeb11c5e86ee0988a038c4e7682d6beaa977 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Tue, 13 Nov 2018 12:03:33 -0800

* Rename the partition file from "cpuset.sched.partition" to "cpuset.cpus.partition".

* When writing to the partition file, drop "0" and "1" and only accept "member" and "root".

Signed-off-by Tejun Heo <[email protected]>
Cc Peter Zijlstra (Intel) <[email protected]>
Cc Waiman Long <[email protected]>

---
Two minor cpuset interface followup patches. Applying to cgroup/for-4.21.

Thanks.

 Documentation/admin-guide/cgroup-v2.rst | 6 +++---
 kernel/cgroup/cpuset.c                  | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)
Documentation/admin-guide/cgroup-v2.rst +3 -3
index f83a5231bbe3..07e06136a550 100644
@@ -1708,15 +1708,15 @@ Cpuset Interface Files
 
 	Its value will be affected by memory nodes hotplug events.
 
-  cpuset.sched.partition
+  cpuset.cpus.partition
 	A read-write single value file which exists on non-root
 	cpuset-enabled cgroups.  This flag is owned by the parent cgroup
 	and is not delegatable.
 
         It accepts only the following input values when written to.
 
-        "root" or "1"   - a paritition root
-        "member" or "0" - a non-root member of a partition
+        "root"   - a paritition root
+        "member" - a non-root member of a partition
 
 	When set to be a partition root, the current cgroup is the
 	root of a new partition or scheduling domain that comprises
kernel/cgroup/cpuset.c +4 -4
index b897314bab53..1151e93d71b6 100644
@@ -2468,11 +2468,11 @@ static ssize_t sched_partition_write(struct kernfs_open_file *of, char *buf,
 	buf = strstrip(buf);
 
 	/*
-	 * Convert "root"/"1" to 1, and convert "member"/"0" to 0.
+	 * Convert "root" to ENABLED, and convert "member" to DISABLED.
 	 */
-	if (!strcmp(buf, "root") || !strcmp(buf, "1"))
+	if (!strcmp(buf, "root"))
 		val = PRS_ENABLED;
-	else if (!strcmp(buf, "member") || !strcmp(buf, "0"))
+	else if (!strcmp(buf, "member"))
 		val = PRS_DISABLED;
 	else
 		return -EINVAL;

@@ -2631,7 +2631,7 @@ static struct cftype dfl_files[] = {
 	},
 
 	{
-		.name = "sched.partition",
+		.name = "cpus.partition",
 		.seq_show = sched_partition_show,
 		.write = sched_partition_write,
 		.private = FILE_PARTITION_ROOT,
Signature · 1 line
2.17.1
author maintainer
#1 lore raw

PATCH 2/2 cgroup: Add .__DEBUG__. prefix to debug file names

From c1bbd933e5fae83f96acd3f748bb01a0300b315d Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Tue, 13 Nov 2018 12:06:41 -0800

Clearly mark the debug files and hide them by default by prefixing ".__DEBUG__.".

Signed-off-by Tejun Heo <[email protected]>
Cc Peter Zijlstra (Intel) <[email protected]>
Cc Waiman Long <[email protected]>
---
 kernel/cgroup/cgroup.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)
kernel/cgroup/cgroup.c +7 -4
index ed7f0bfe6429..e06994fd4e34 100644
@@ -1400,12 +1400,15 @@ static char *cgroup_file_name(struct cgroup *cgrp, const struct cftype *cft,
 	struct cgroup_subsys *ss = cft->ss;
 
 	if (cft->ss && !(cft->flags & CFTYPE_NO_PREFIX) &&
-	    !(cgrp->root->flags & CGRP_ROOT_NOPREFIX))
-		snprintf(buf, CGROUP_FILE_NAME_MAX, "%s.%s",
-			 cgroup_on_dfl(cgrp) ? ss->name : ss->legacy_name,
+	    !(cgrp->root->flags & CGRP_ROOT_NOPREFIX)) {
+		const char *dbg = (cft->flags & CFTYPE_DEBUG) ? ".__DEBUG__." : "";
+
+		snprintf(buf, CGROUP_FILE_NAME_MAX, "%s%s.%s",
+			 dbg, cgroup_on_dfl(cgrp) ? ss->name : ss->legacy_name,
 			 cft->name);
-	else
+	} else {
 		strscpy(buf, cft->name, CGROUP_FILE_NAME_MAX);
+	}
 	return buf;
 }
Signature · 1 line
2.17.1
#2 lore raw

Re: [PATCH cgroup/for-4.21 2/2] cgroup: Add .__DEBUG__. prefix to debug file names

Quoted 6 lines · Tejun Heo

From c1bbd933e5fae83f96acd3f748bb01a0300b315d Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Tue, 13 Nov 2018 12:06:41 -0800

Clearly mark the debug files and hide them by default by prefixing ".__DEBUG__.".

ACK

maintainer Acked-by on v1 1/2
#3 lore raw
In reply to Tejun Heo
Quoted 66 lines · Tejun Heo

From b1e3aeb11c5e86ee0988a038c4e7682d6beaa977 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Tue, 13 Nov 2018 12:03:33 -0800

* Rename the partition file from "cpuset.sched.partition" to "cpuset.cpus.partition".

* When writing to the partition file, drop "0" and "1" and only accept "member" and "root".

Signed-off-by Tejun Heo <[email protected]>
Cc Peter Zijlstra (Intel) <[email protected]>
Cc Waiman Long <[email protected]>

---
Two minor cpuset interface followup patches. Applying to cgroup/for-4.21.

Thanks.

 Documentation/admin-guide/cgroup-v2.rst | 6 +++---
 kernel/cgroup/cpuset.c                  | 8 ++++----
 2 files changed, 7 insertions(+), 7 deletions(-)
Documentation/admin-guide/cgroup-v2.rst +3 -3
index f83a5231bbe3..07e06136a550 100644
@@ -1708,15 +1708,15 @@ Cpuset Interface Files
 
 	Its value will be affected by memory nodes hotplug events.
 
-  cpuset.sched.partition
+  cpuset.cpus.partition
 	A read-write single value file which exists on non-root
 	cpuset-enabled cgroups.  This flag is owned by the parent cgroup
 	and is not delegatable.
 
         It accepts only the following input values when written to.
 
-        "root" or "1"   - a paritition root
-        "member" or "0" - a non-root member of a partition
+        "root"   - a paritition root
+        "member" - a non-root member of a partition
 
 	When set to be a partition root, the current cgroup is the
 	root of a new partition or scheduling domain that comprises
kernel/cgroup/cpuset.c +3 -3
index b897314bab53..1151e93d71b6 100644
@@ -2468,11 +2468,11 @@ static ssize_t sched_partition_write(struct kernfs_open_file *of, char *buf,
 	buf = strstrip(buf);
 
 	/*
-	 * Convert "root"/"1" to 1, and convert "member"/"0" to 0.
+	 * Convert "root" to ENABLED, and convert "member" to DISABLED.
 	 */
-	if (!strcmp(buf, "root") || !strcmp(buf, "1"))
+	if (!strcmp(buf, "root"))
 		val = PRS_ENABLED;
-	else if (!strcmp(buf, "member") || !strcmp(buf, "0"))
+	else if (!strcmp(buf, "member"))
 		val = PRS_DISABLED;
 	else
 		return -EINVAL;

kernel/cgroup/cpuset.c static struct cftype dfl_files[] = { · line 2637

Quoted 8 lines
kernel/cgroup/cpuset.c +1 -1
@@ -2631,7 +2631,7 @@ static struct cftype dfl_files[] = {
 	},
 
 	{
-		.name = "sched.partition",
+		.name = "cpus.partition",
 		.seq_show = sched_partition_show,
 		.write = sched_partition_write,
 		.private = FILE_PARTITION_ROOT,

I am fine with both of your patches.

Acked-by Waiman Long <[email protected]>
#4 lore raw
In reply to Tejun Heo
Quoted 9 lines · Tejun Heo

From b1e3aeb11c5e86ee0988a038c4e7682d6beaa977 Mon Sep 17 00:00:00 2001
From: Tejun Heo <[email protected]>
Date: Tue, 13 Nov 2018 12:03:33 -0800

* Rename the partition file from "cpuset.sched.partition" to "cpuset.cpus.partition".

* When writing to the partition file, drop "0" and "1" and only accept "member" and "root".

Why though? The Changelog doesn't give rationale for the actual changes.

And I'm not sure I agree with either one of them.

The partition is a scheduling feature; and I like 0/1 much better to type, so why not allow that?

author maintainer
#5 lore raw

Hello, Peter.

Quoted 1 line · Peter Zijlstra

Why though? The Changelog doesn't give rationale for the actual changes.

Ah yeah, sorry about that.

Quoted 3 lines

And I'm not sure I agree with either one of them.

The partition is a scheduling feature;

So is everything with cpuset.cpus prefix. They're all modifying how scheduler handles the cpus.

Quoted 1 line

and I like 0/1 much better to type, so why not allow that?

Mostly for consistency and it's generally better to keep interfaces minimal - e.g. what if we need to add support for more key words to the file? Would we assign incrementing integers to them?

Thanks.

Signature · 1 line
tejun
#6 lore raw
Quoted 5 lines · Tejun Heo

Hello, Peter.

Quoted 1 line · Peter Zijlstra

Why though? The Changelog doesn't give rationale for the actual changes.

Quoted 8 lines

Ah yeah, sorry about that.

Quoted 3 lines

And I'm not sure I agree with either one of them.

The partition is a scheduling feature;

So is everything with cpuset.cpus prefix. They're all modifying how scheduler handles the cpus.

Fair enough I suppose.

Quoted 5 lines
Quoted 1 line

and I like 0/1 much better to type, so why not allow that?

Mostly for consistency and it's generally better to keep interfaces minimal - e.g. what if we need to add support for more key words to the file? Would we assign incrementing integers to them?

Or just not give them a numeric alias at all.

But numbers are more minimal than words, so lets just get rid of them pesky word things ;-)

Alternatively; we could use kstrtobool(). That accepts a metric ton of input ;-)