cgroup: don't destroy the default root

PATCH Superseded Li Zefan 2 participants 10 messages Jun 3, 2014 – Jun 5, 2014 cgroup

Share & tools

Li Zefan

author

2014-06-03 04:04 UTC #0 lore raw

PATCH 1/3 cgroup: don't destroy the default root

The default root is allocated and initialized at boot, so we shouldn't destroy the default root when it's umounted, otherwise it will lead to disaster.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

kernel/cgroup.c +9 -5

index a5f75ac..f73fe48 100644
@@ -1027,12 +1027,14 @@ static umode_t cgroup_file_mode(const struct cftype *cft)
 static void cgroup_get(struct cgroup *cgrp)
 {
 	WARN_ON_ONCE(cgroup_is_dead(cgrp));
-	css_get(&cgrp->self);
+	if (!(cgrp->self.flags & CSS_NO_REF))
+		css_get(&cgrp->self);
 }
 
 static void cgroup_put(struct cgroup *cgrp)
 {
-	css_put(&cgrp->self);
+	if (!(cgrp->self.flags & CSS_NO_REF))
+		css_put(&cgrp->self);
 }
 
 /**

@@ -1781,10 +1783,12 @@ static void cgroup_kill_sb(struct super_block *sb)
 	 * This prevents new mounts by disabling percpu_ref_tryget_live().
 	 * cgroup_mount() may wait for @root's release.
 	 */
-	if (css_has_online_children(&root->cgrp.self))
+	if (css_has_online_children(&root->cgrp.self)) {
 		cgroup_put(&root->cgrp);
-	else
-		percpu_ref_kill(&root->cgrp.self.refcnt);
+	} else {
+		if (root != &cgrp_dfl_root)
+			percpu_ref_kill(&root->cgrp.self.refcnt);
+	}
 
 	kernfs_kill_sb(sb);
 }

Signature · 1 line

1.8.0.2

Li Zefan

author

2014-06-03 04:05 UTC #1 lore raw

PATCH 2/3 cgroup: make the default root invisible when it's umounted

Before this patch (in a fresh system):

   # cat /proc/$$/cgroup
   # mount -t cgroup -o __DEVEL__sane_behavior xxx /cgroup
   # umount /cgroup
   # cat /proc/$$/cgroup
   0:cpuset,cpu,cpuacct,memory,devices,freezer,net_cls,blkio,perf_event,net_prio,hugetlb:/

After this patch (in a fresh system):

   # cat ...
   # mount ...
   # umount ...
   # cat /proc/$$/cgroup
   #

You won't see the default root after it's umounted.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c | 2 ++
 1 file changed, 2 insertions(+)

kernel/cgroup.c +2 -0

index f73fe48..dabc486 100644
@@ -1788,6 +1788,8 @@ static void cgroup_kill_sb(struct super_block *sb)
 	} else {
 		if (root != &cgrp_dfl_root)
 			percpu_ref_kill(&root->cgrp.self.refcnt);
+		else
+			cgrp_dfl_root_visible = false;
 	}
 
 	kernfs_kill_sb(sb);

Signature · 1 line

1.8.0.2

Tejun Heo

maintainer

2014-06-03 13:01 UTC #2 lore raw

Re: [PATCH 2/3] cgroup: make the default root invisible when it's umounted

Quoted 10 lines · Li Zefan

Before this patch (in a fresh system):

   # cat /proc/$$/cgroup
   # mount -t cgroup -o __DEVEL__sane_behavior xxx /cgroup
   # umount /cgroup
   # cat /proc/$$/cgroup
   0:cpuset,cpu,cpuacct,memory,devices,freezer,net_cls,blkio,perf_event,net_prio,hugetlb:/

After this patch (in a fresh system):

Quoted 7 lines

   # cat ...
   # mount ...
   # umount ...
   # cat /proc/$$/cgroup
   #
You won't see the default root after it's umounted.

Hmmmm... I intentionally left it visible tho. The only reason we gate its visibility is avoid disturbing userland which doesn't know about and won't use the unified hierarchy. If the userland starts making use of it, there's no reason to hide it again especially as that's consistent with how other hierarchies behave too - they keep showing up if they have lingering refs.

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2014-06-04 08:59 UTC #3 lore raw

Re: [PATCH 2/3] cgroup: make the default root invisible when it's umounted

Quoted 17 lines · Tejun Heo

Quoted 16 lines · Li Zefan

Before this patch (in a fresh system):

   # cat /proc/$$/cgroup
   # mount -t cgroup -o __DEVEL__sane_behavior xxx /cgroup
   # umount /cgroup
   # cat /proc/$$/cgroup
   0:cpuset,cpu,cpuacct,memory,devices,freezer,net_cls,blkio,perf_event,net_prio,hugetlb:/

After this patch (in a fresh system):

   # cat ...
   # mount ...
   # umount ...
   # cat /proc/$$/cgroup
   #

Quoted 9 lines

Quoted 1 line
You won't see the default root after it's umounted.
Hmmmm... I intentionally left it visible tho. The only reason we gate its visibility is avoid disturbing userland which doesn't know about and won't use the unified hierarchy. If the userland starts making use of it, there's no reason to hide it again especially as that's consistent with how other hierarchies behave too - they keep showing up if they have lingering refs.

The example I gave is the same result if sane_behavior is not specified, so this is a behavioural change for the old interface?

Do we need a fix like this?

kernel/cgroup.c +3 -0

index 3f46165..6f10cff 100644
@@ -1789,6 +1789,9 @@ static void cgroup_kill_sb(struct super_block *sb)
 	else
 		percpu_ref_kill(&root->cgrp.self.refcnt);
 
+	if (root == &cgrp_dfl_root && !cgroup_sane_behavior(&root->cgrp))
+		cgrp_dfl_root_visible = false;
+
 	kernfs_kill_sb(sb);

Tejun Heo

maintainer

2014-06-05 01:20 UTC #4 lore raw

Re: [PATCH 2/3] cgroup: make the default root invisible when it's umounted

Hello,

Quoted 2 lines · Li Zefan

The example I gave is the same result if sane_behavior is not specified, so this is a behavioural change for the old interface?

Hmmm? Either the userland knows about unified hierarchy or not and there's no point in hiding it if we know that userland knows. It's really a system-wide thing which happens once.

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2014-06-05 01:49 UTC #5 lore raw

Re: [PATCH 2/3] cgroup: make the default root invisible when it's umounted

Quoted 10 lines · Tejun Heo

Hello,
Quoted 2 lines · Li Zefan
The example I gave is the same result if sane_behavior is not specified, so this is a behavioural change for the old interface?
Hmmm? Either the userland knows about unified hierarchy or not and there's no point in hiding it if we know that userland knows. It's really a system-wide thing which happens once.

Yeah, it's reasonable.

I thought "mount -t cgroup xxx /cgroup" will mount the default hierarchy, but I was wrong.

Li Zefan

author

2014-06-03 04:05 UTC #6 lore raw

PATCH 3/3 cgroup: set visible flag only after we've mounted the default root

In reply to Li Zefan

This fixes the failure path, so we won't set the visible flag though the mount is failed.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

kernel/cgroup.c +3 -1

index dabc486..0b6b44e 100644
@@ -1671,7 +1671,6 @@ static struct dentry *cgroup_mount(struct file_system_type *fs_type,
 
 	/* look for a matching existing root */
 	if (!opts.subsys_mask && !opts.none && !opts.name) {
-		cgrp_dfl_root_visible = true;
 		root = &cgrp_dfl_root;
 		cgroup_get(&root->cgrp);
 		ret = 0;

@@ -1770,6 +1769,9 @@ out_free:
 	dentry = kernfs_mount(fs_type, flags, root->kf_root, &new_sb);
 	if (IS_ERR(dentry) || !new_sb)
 		cgroup_put(&root->cgrp);
+	else if (root == &cgrp_dfl_root)
+		cgrp_dfl_root_visible = true;
+
 	return dentry;
 }

Signature · 1 line

1.8.0.2

Tejun Heo

maintainer

2014-06-03 13:02 UTC #7 lore raw

Re: [PATCH 3/3] cgroup: set visible flag only after we've mounted the default root

Quoted 2 lines · Li Zefan

This fixes the failure path, so we won't set the visible flag though the mount is failed.

Same rationale. If the userland knows about it, there's no point in hiding it.

Thanks.

Signature · 1 line

tejun

Tejun Heo

maintainer

2014-06-03 12:57 UTC #8 lore raw

In reply to Li Zefan

Hello, Li.

kernel/cgroup.c static umode_t cgroup_file_mode(const struct cftype *cft) · line 1031

Quoted 6 lines · Li Zefan

static void cgroup_get(struct cgroup *cgrp)
{

kernel/cgroup.c +2 -1

 	WARN_ON_ONCE(cgroup_is_dead(cgrp));
-	css_get(&cgrp->self);
+	if (!(cgrp->self.flags & CSS_NO_REF))
+		css_get(&cgrp->self);

Hmmm? The same condition is tested by css_get(). Why should it be tested again here?

kernel/cgroup.c static umode_t cgroup_file_mode(const struct cftype *cft) · line 1037

Quoted 5 lines

static void cgroup_put(struct cgroup *cgrp)
{
kernel/cgroup.c +2 -1
-	css_put(&cgrp->self);
+	if (!(cgrp->self.flags & CSS_NO_REF))
+		css_put(&cgrp->self);

Ditto.

kernel/cgroup.c static void cgroup_kill_sb(struct super_block *sb) · line 1791

Quoted 13 lines

kernel/cgroup.c +5 -3

@@ -1781,10 +1783,12 @@ static void cgroup_kill_sb(struct super_block *sb)
 	 * This prevents new mounts by disabling percpu_ref_tryget_live().
 	 * cgroup_mount() may wait for @root's release.
 	 */
-	if (css_has_online_children(&root->cgrp.self))
+	if (css_has_online_children(&root->cgrp.self)) {
 		cgroup_put(&root->cgrp);
-	else
-		percpu_ref_kill(&root->cgrp.self.refcnt);
+	} else {
+		if (root != &cgrp_dfl_root)
+			percpu_ref_kill(&root->cgrp.self.refcnt);
+	}

As conceptually percpu_ref_kill() just puts the base ref and the dfl_root's refcnt never reaches zero, it won't actually trigger. Hmmm.... wouldn't the above leak a ref each time the default hierarchy is unmounted tho? Shouldn't it be like the following?

	if (root == &cgrp_dfl_root || css_has_online_children(...))
		cgroup_put(&root->cgrp);
	else
		percpu_ref_kill(...);

Thanks.

Signature · 1 line

tejun

Li Zefan

author

2014-06-04 08:40 UTC #9 lore raw

Quoted 13 lines · Tejun Heo

Hello, Li.
kernel/cgroup.c static umode_t cgroup_file_mode(const struct cftype *cft) · line 1031
Quoted 6 lines · Li Zefan
static void cgroup_get(struct cgroup *cgrp)
{
kernel/cgroup.c +2 -1
 	WARN_ON_ONCE(cgroup_is_dead(cgrp));
-	css_get(&cgrp->self);
+	if (!(cgrp->self.flags & CSS_NO_REF))
+		css_get(&cgrp->self);
Hmmm? The same condition is tested by css_get(). Why should it be tested again here?

Oh, I completely ignored that.

Quoted 8 lines

kernel/cgroup.c static umode_t cgroup_file_mode(const struct cftype *cft) · line 1037
Quoted 5 lines
static void cgroup_put(struct cgroup *cgrp)
{
kernel/cgroup.c +2 -1
-	css_put(&cgrp->self);
+	if (!(cgrp->self.flags & CSS_NO_REF))
+		css_put(&cgrp->self);
Ditto.

Quoted 16 lines

kernel/cgroup.c static void cgroup_kill_sb(struct super_block *sb) · line 1791

Quoted 13 lines

kernel/cgroup.c +5 -3

@@ -1781,10 +1783,12 @@ static void cgroup_kill_sb(struct super_block *sb)
 	 * This prevents new mounts by disabling percpu_ref_tryget_live().
 	 * cgroup_mount() may wait for @root's release.
 	 */
-	if (css_has_online_children(&root->cgrp.self))
+	if (css_has_online_children(&root->cgrp.self)) {
 		cgroup_put(&root->cgrp);
-	else
-		percpu_ref_kill(&root->cgrp.self.refcnt);
+	} else {
+		if (root != &cgrp_dfl_root)
+			percpu_ref_kill(&root->cgrp.self.refcnt);
+	}

As conceptually percpu_ref_kill() just puts the base ref and the dfl_root's refcnt never reaches zero, it won't actually trigger.

Yes it will, just try mount && umount.

I think it's because cgroup_get() is a no-op for CSS_NO_REF, so it has only the base ref, so percpu_ref_iill() will actually schedule the call to css_release().

Quoted 3 lines

Hmmm.... wouldn't the above leak a ref each time the default hierarchy is unmounted tho? Shouldn't it be like the following?

cgroup_get() is a no-op for root cgroup of the default root, so there's no leak, but still better to call cgroup_put().

I'll send an updated patch.

Quoted 7 lines

	if (root == &cgrp_dfl_root || css_has_online_children(...))
		cgroup_put(&root->cgrp);
	else
		percpu_ref_kill(...);

Thanks.