* [PATCH 1/3] cgroup: don't destroy the default root
@ 2014-06-03 4:04 ` Li Zefan
0 siblings, 0 replies; 18+ messages in thread
From: Li Zefan @ 2014-06-03 4:04 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML, Cgroups
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 <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
kernel/cgroup.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a5f75ac..f73fe48 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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);
}
--
1.8.0.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 1/3] cgroup: don't destroy the default root
@ 2014-06-03 4:04 ` Li Zefan
0 siblings, 0 replies; 18+ messages in thread
From: Li Zefan @ 2014-06-03 4:04 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML, Cgroups
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 <lizefan@huawei.com>
---
kernel/cgroup.c | 14 +++++++++-----
1 file changed, 9 insertions(+), 5 deletions(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a5f75ac..f73fe48 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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);
}
--
1.8.0.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 2/3] cgroup: make the default root invisible when it's umounted
2014-06-03 4:04 ` Li Zefan
(?)
@ 2014-06-03 4:05 ` Li Zefan
[not found] ` <538D4982.10905-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
-1 siblings, 1 reply; 18+ messages in thread
From: Li Zefan @ 2014-06-03 4:05 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML, Cgroups
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 <lizefan@huawei.com>
---
kernel/cgroup.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index f73fe48..dabc486 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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);
--
1.8.0.2
^ permalink raw reply related [flat|nested] 18+ messages in thread[parent not found: <538D4956.5050205-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]
* [PATCH 3/3] cgroup: set visible flag only after we've mounted the default root
2014-06-03 4:04 ` Li Zefan
@ 2014-06-03 4:05 ` Li Zefan
-1 siblings, 0 replies; 18+ messages in thread
From: Li Zefan @ 2014-06-03 4:05 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML, Cgroups
This fixes the failure path, so we won't set the visible flag though
the mount is failed.
Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
kernel/cgroup.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index dabc486..0b6b44e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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;
}
--
1.8.0.2
^ permalink raw reply related [flat|nested] 18+ messages in thread* [PATCH 3/3] cgroup: set visible flag only after we've mounted the default root
@ 2014-06-03 4:05 ` Li Zefan
0 siblings, 0 replies; 18+ messages in thread
From: Li Zefan @ 2014-06-03 4:05 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML, Cgroups
This fixes the failure path, so we won't set the visible flag though
the mount is failed.
Signed-off-by: Li Zefan <lizefan@huawei.com>
---
kernel/cgroup.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index dabc486..0b6b44e 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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;
}
--
1.8.0.2
^ permalink raw reply related [flat|nested] 18+ messages in thread[parent not found: <538D49A7.2010403-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>]
* Re: [PATCH 3/3] cgroup: set visible flag only after we've mounted the default root
2014-06-03 4:05 ` Li Zefan
@ 2014-06-03 13:02 ` Tejun Heo
-1 siblings, 0 replies; 18+ messages in thread
From: Tejun Heo @ 2014-06-03 13:02 UTC (permalink / raw)
To: Li Zefan; +Cc: LKML, Cgroups
On Tue, Jun 03, 2014 at 12:05:59PM +0800, Li Zefan wrote:
> 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.
--
tejun
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] cgroup: don't destroy the default root
2014-06-03 4:04 ` Li Zefan
@ 2014-06-03 12:57 ` Tejun Heo
-1 siblings, 0 replies; 18+ messages in thread
From: Tejun Heo @ 2014-06-03 12:57 UTC (permalink / raw)
To: Li Zefan; +Cc: LKML, Cgroups
Hello, Li.
On Tue, Jun 03, 2014 at 12:04:38PM +0800, Li Zefan wrote:
> 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);
Hmmm? The same condition is tested by css_get(). Why should it be
tested again here?
> static void cgroup_put(struct cgroup *cgrp)
> {
> - css_put(&cgrp->self);
> + if (!(cgrp->self.flags & CSS_NO_REF))
> + css_put(&cgrp->self);
Ditto.
> @@ -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.
--
tejun
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/3] cgroup: don't destroy the default root
@ 2014-06-03 12:57 ` Tejun Heo
0 siblings, 0 replies; 18+ messages in thread
From: Tejun Heo @ 2014-06-03 12:57 UTC (permalink / raw)
To: Li Zefan; +Cc: LKML, Cgroups
Hello, Li.
On Tue, Jun 03, 2014 at 12:04:38PM +0800, Li Zefan wrote:
> 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);
Hmmm? The same condition is tested by css_get(). Why should it be
tested again here?
> static void cgroup_put(struct cgroup *cgrp)
> {
> - css_put(&cgrp->self);
> + if (!(cgrp->self.flags & CSS_NO_REF))
> + css_put(&cgrp->self);
Ditto.
> @@ -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.
--
tejun
^ permalink raw reply [flat|nested] 18+ messages in thread[parent not found: <20140603125728.GA26210-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org>]
* Re: [PATCH 1/3] cgroup: don't destroy the default root
2014-06-03 12:57 ` Tejun Heo
@ 2014-06-04 8:40 ` Li Zefan
-1 siblings, 0 replies; 18+ messages in thread
From: Li Zefan @ 2014-06-04 8:40 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML, Cgroups
On 2014/6/3 20:57, Tejun Heo wrote:
> Hello, Li.
>
> On Tue, Jun 03, 2014 at 12:04:38PM +0800, Li Zefan wrote:
>> 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);
>
> Hmmm? The same condition is tested by css_get(). Why should it be
> tested again here?
>
Oh, I completely ignored that.
>> static void cgroup_put(struct cgroup *cgrp)
>> {
>> - css_put(&cgrp->self);
>> + if (!(cgrp->self.flags & CSS_NO_REF))
>> + css_put(&cgrp->self);
>
> Ditto.
>
>> @@ -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().
> 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.
> if (root == &cgrp_dfl_root || css_has_online_children(...))
> cgroup_put(&root->cgrp);
> else
> percpu_ref_kill(...);
>
> Thanks.
>
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 1/3] cgroup: don't destroy the default root
@ 2014-06-04 8:40 ` Li Zefan
0 siblings, 0 replies; 18+ messages in thread
From: Li Zefan @ 2014-06-04 8:40 UTC (permalink / raw)
To: Tejun Heo; +Cc: LKML, Cgroups
On 2014/6/3 20:57, Tejun Heo wrote:
> Hello, Li.
>
> On Tue, Jun 03, 2014 at 12:04:38PM +0800, Li Zefan wrote:
>> 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);
>
> Hmmm? The same condition is tested by css_get(). Why should it be
> tested again here?
>
Oh, I completely ignored that.
>> static void cgroup_put(struct cgroup *cgrp)
>> {
>> - css_put(&cgrp->self);
>> + if (!(cgrp->self.flags & CSS_NO_REF))
>> + css_put(&cgrp->self);
>
> Ditto.
>
>> @@ -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().
> 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.
> if (root == &cgrp_dfl_root || css_has_online_children(...))
> cgroup_put(&root->cgrp);
> else
> percpu_ref_kill(...);
>
> Thanks.
>
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-06-05 1:49 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-06-03 4:04 [PATCH 1/3] cgroup: don't destroy the default root Li Zefan
2014-06-03 4:04 ` Li Zefan
2014-06-03 4:05 ` [PATCH 2/3] cgroup: make the default root invisible when it's umounted Li Zefan
[not found] ` <538D4982.10905-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2014-06-03 13:01 ` Tejun Heo
2014-06-03 13:01 ` Tejun Heo
[not found] ` <20140603130144.GB26210-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org>
2014-06-04 8:59 ` Li Zefan
2014-06-04 8:59 ` Li Zefan
[not found] ` <538EE00F.10406-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2014-06-05 1:20 ` Tejun Heo
2014-06-05 1:20 ` Tejun Heo
2014-06-05 1:49 ` Li Zefan
[not found] ` <538D4956.5050205-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2014-06-03 4:05 ` [PATCH 3/3] cgroup: set visible flag only after we've mounted the default root Li Zefan
2014-06-03 4:05 ` Li Zefan
[not found] ` <538D49A7.2010403-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
2014-06-03 13:02 ` Tejun Heo
2014-06-03 13:02 ` Tejun Heo
2014-06-03 12:57 ` [PATCH 1/3] cgroup: don't destroy " Tejun Heo
2014-06-03 12:57 ` Tejun Heo
[not found] ` <20140603125728.GA26210-Gd/HAXX7CRxy/B6EtB590w@public.gmane.org>
2014-06-04 8:40 ` Li Zefan
2014-06-04 8:40 ` Li Zefan
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.