Skip to contentSkip to conversation

cgroup: post unified hierarchy fixes and updates

PATCH Superseded Tejun Heo 1 participant 6 messages May 6, 2014 cgroup

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Tejun Heo

author maintainer
#0 lore raw

PATCH 1/1 cgroup: post unified hierarchy fixes and updates

Hello,

This patchset contains the following four patches.

0001-cgroup-fix-offlining-child-waiting-in-cgroup_subtree.patch 0002-cgroup-only-allow-space-as-the-separator-for-cgroup..patch 0003-cgroup-use-restart_syscall-for-retries-after-offline.patch 0004-cgroup-use-release_agent_path_lock-in-cgroup_release.patch

0001 fixes two bugs in cgroup_subtree_control_write().

0002 makes subtree_control parsing stricter.

0003 simplifies cgroup_substree_control_write() retry path by using restart_syscall().

0004 makes cgroup_release_agent_show() use release_path_lock. The original conversion missed this one.

This patchset is on top of cgroup/for-3.16 12d3089c192c ("kernel/cpuset.c: convert printk to pr_foo()") and available on the following git branch.

git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup.git review-post-unified-updates

diffstat follows.

 kernel/cgroup.c |   33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

Thanks.

--
tejun

author maintainer
#1 lore raw

PATCH 1/4 cgroup: fix offlining child waiting in cgroup_subtree_control_write()

cgroup_subtree_control_write() waits for offline to complete child-by-child before enabling a controller; however, it has a couple bugs.

* It doesn't initialize the wait_queue_t. This can lead to infinite hang on the following schedule() among other things.

* It forgets to pin the child before releasing cgroup_tree_mutex and performing schedule(). The child may already be gone by the time it wakes up and invokes finish_wait(). Pin the child being waited on.

Signed-off-by Tejun Heo <[email protected]>
---
 kernel/cgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
kernel/cgroup.c +3 -1
index 07815ef..54fd12d 100644
@@ -2594,16 +2594,18 @@ retry:
 			 * cases, wait till it's gone using offline_waitq.
 			 */
 			cgroup_for_each_live_child(child, cgrp) {
-				wait_queue_t wait;
+				DEFINE_WAIT(wait);
 
 				if (!cgroup_css(child, ss))
 					continue;
 
+				cgroup_get(child);
 				prepare_to_wait(&child->offline_waitq, &wait,
 						TASK_UNINTERRUPTIBLE);
 				mutex_unlock(&cgroup_tree_mutex);
 				schedule();
 				finish_wait(&child->offline_waitq, &wait);
+				cgroup_put(child);
 				goto retry;
 			}
Signature · 1 line
1.9.0
author maintainer
#2 lore raw

PATCH 2/4 cgroup: only allow space as the separator for "cgroup.subtree_control"

In reply to Tejun Heo

I was confused that strsep() was equivalent to strtok_r() in skipping over consecutive delimiters. strsep() just splits at the first occurrence of one of the delimiters which makes the parsing very inflexible, which makes allowing multiple whitespace chars as delimters kinda moot. Let's just be consistently strict and require list of tokens separated by a single space. This is what Documentation/cgroups/unified-hierarchy.txt describes too.

Signed-off-by Tejun Heo <[email protected]>
---
 kernel/cgroup.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
kernel/cgroup.c +3 -3
index 54fd12d..9066fe9 100644
@@ -2542,11 +2542,11 @@ static int cgroup_subtree_control_write(struct cgroup_subsys_state *dummy_css,
 	int ssid, ret;
 
 	/*
-	 * Parse input - white space separated list of subsystem names
-	 * prefixed with either + or -.
+	 * Parse input - space separated list of subsystem names prefixed
+	 * with either + or -.
 	 */
 	p = buffer;
-	while ((tok = strsep(&p, " \t\n"))) {
+	while ((tok = strsep(&p, " "))) {
 		for_each_subsys(ss, ssid) {
 			if (ss->disabled || strcmp(tok + 1, ss->name))
 				continue;
Signature · 1 line
1.9.0
author maintainer
#3 lore raw

PATCH v2 2/9 cgroup: update and fix parsing of "cgroup.subtree_control"

I was confused that strsep() was equivalent to strtok_r() in skipping over consecutive delimiters. strsep() just splits at the first occurrence of one of the delimiters which makes the parsing very inflexible, which makes allowing multiple whitespace chars as delimters kinda moot. Let's just be consistently strict and require list of tokens separated by spaces. This is what Documentation/cgroups/unified-hierarchy.txt describes too.

Also, parsing may access beyond the end of the string if the string ends with spaces or is zero-length. Make sure it skips zero-length tokens. Note that this also ensures that the parser doesn't puke on multiple consecutive spaces.

v2: Add zero-length token skipping.

Signed-off-by Tejun Heo <[email protected]>
---
 kernel/cgroup.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)
kernel/cgroup.c +5 -3
index 54fd12d..9e6b4ac 100644
@@ -2542,11 +2542,13 @@ static int cgroup_subtree_control_write(struct cgroup_subsys_state *dummy_css,
 	int ssid, ret;
 
 	/*
-	 * Parse input - white space separated list of subsystem names
-	 * prefixed with either + or -.
+	 * Parse input - space separated list of subsystem names prefixed
+	 * with either + or -.
 	 */
 	p = buffer;
-	while ((tok = strsep(&p, " \t\n"))) {
+	while ((tok = strsep(&p, " "))) {
+		if (tok[0] =='\0')
+			continue;
 		for_each_subsys(ss, ssid) {
 			if (ss->disabled || strcmp(tok + 1, ss->name))
 				continue;
Signature · 1 line
1.9.0
author maintainer
#4 lore raw

PATCH 3/4 cgroup: use restart_syscall() for retries after offline waits in cgroup_subtree_control_write()

In reply to Tejun Heo

After waiting for a child to finish offline, cgroup_subtree_control_write() jumps up to retry from after the input parsing and active protection breaking. This retry makes the scheduled locking update more difficult. Let's simplify it by returning with restart_syscall() for retries.

Signed-off-by Tejun Heo <[email protected]>
---
 kernel/cgroup.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)
kernel/cgroup.c +9 -9
index 9066fe9..3ced79a 100644
@@ -2535,7 +2535,7 @@ out_finish:
 static int cgroup_subtree_control_write(struct cgroup_subsys_state *dummy_css,
 					struct cftype *cft, char *buffer)
 {
-	unsigned int enable_req = 0, disable_req = 0, enable, disable;
+	unsigned int enable = 0, disable = 0;
 	struct cgroup *cgrp = dummy_css->cgroup, *child;
 	struct cgroup_subsys *ss;
 	char *tok, *p;

@@ -2552,11 +2552,11 @@ static int cgroup_subtree_control_write(struct cgroup_subsys_state *dummy_css,
 				continue;
 
 			if (*tok == '+') {
-				enable_req |= 1 << ssid;
-				disable_req &= ~(1 << ssid);
+				enable |= 1 << ssid;
+				disable &= ~(1 << ssid);
 			} else if (*tok == '-') {
-				disable_req |= 1 << ssid;
-				enable_req &= ~(1 << ssid);
+				disable |= 1 << ssid;
+				enable &= ~(1 << ssid);
 			} else {
 				return -EINVAL;
 			}

@@ -2574,9 +2574,6 @@ static int cgroup_subtree_control_write(struct cgroup_subsys_state *dummy_css,
 	 */
 	cgroup_get(cgrp);
 	kernfs_break_active_protection(cgrp->control_kn);
-retry:
-	enable = enable_req;
-	disable = disable_req;
 
 	mutex_lock(&cgroup_tree_mutex);
 

@@ -2606,7 +2603,9 @@ retry:
 				schedule();
 				finish_wait(&child->offline_waitq, &wait);
 				cgroup_put(child);
-				goto retry;
+
+				ret = restart_syscall();
+				goto out_unbreak;
 			}
 
 			/* unavailable or not enabled on the parent? */

@@ -2690,6 +2689,7 @@ out_unlock:
 	mutex_unlock(&cgroup_mutex);
 out_unlock_tree:
 	mutex_unlock(&cgroup_tree_mutex);
+out_unbreak:
 	kernfs_unbreak_active_protection(cgrp->control_kn);
 	cgroup_put(cgrp);
 	return ret;
Signature · 1 line
1.9.0
author maintainer
#5 lore raw

PATCH 4/4 cgroup: use release_agent_path_lock in cgroup_release_agent_show()

In reply to Tejun Heo

release_path is now protected by release_agent_path_lock to allow accessing it without grabbing cgroup_mutex; however, cgroup_release_agent_show() was still grabbing cgroup_mutex. Let's convert it to release_agent_path_lock so that we don't have to worry about this one for the planned locking updates.

Signed-off-by Tejun Heo <[email protected]>
---
 kernel/cgroup.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
kernel/cgroup.c +2 -3
index 3ced79a..57c2021 100644
@@ -2373,11 +2373,10 @@ static int cgroup_release_agent_show(struct seq_file *seq, void *v)
 {
 	struct cgroup *cgrp = seq_css(seq)->cgroup;
 
-	if (!cgroup_lock_live_group(cgrp))
-		return -ENODEV;
+	spin_lock(&release_agent_path_lock);
 	seq_puts(seq, cgrp->root->release_agent_path);
+	spin_unlock(&release_agent_path_lock);
 	seq_putc(seq, '\n');
-	mutex_unlock(&cgroup_mutex);
 	return 0;
 }
Signature · 1 line
1.9.0