[PATCH 1/2] cgroup: remove synchronize_rcu() from cgroup_attach_{task|proc}()

[PATCH 1/2] cgroup: remove synchronize_rcu() from cgroup_attach_{task|proc}()

[Li Zefan]

These 2 syncronize_rcu()s make attaching a task to a cgroup
quite slow, and it can't be ignored in some situations.

A real case from Colin Cross: Android uses cgroups heavily to
manage thread priorities, putting threads in a background group
with reduced cpu.shares when they are not visible to the user,
and in a foreground group when they are. Some RPCs from foreground
threads to background threads will temporarily move the background
thread into the foreground group for the duration of the RPC.
This results in many calls to cgroup_attach_task.

In cgroup_attach_task() it's task->cgroups that is protected by RCU,
and put_css_set() calls kfree_rcu() to free it.

If we remove this synchronize_rcu(), there can be threads in RCU-read
sections accessing their old cgroup via current->cgroups with
concurrent rmdir operation, but this is safe.

 # time for ((i=0; i<50; i++)) { echo $$ > /mnt/sub/tasks; echo $$ > /mnt/tasks; }

real    0m2.524s
user    0m0.008s
sys     0m0.004s

With this patch:

real    0m0.004s
user    0m0.004s
sys     0m0.000s

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a5262d9..67b0fa0 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1974,7 +1974,6 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 			ss->attach(cgrp, &tset);
 	}
 
-	synchronize_rcu();
 out:
 	if (retval) {
 		for_each_subsys(root, ss) {
@@ -2143,7 +2142,6 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	/*
 	 * step 5: success! and cleanup
 	 */
-	synchronize_rcu();
 	retval = 0;
 out_put_css_set_refs:
 	if (retval) {
-- 
1.8.0.2

[PATCH 1/2] cgroup: remove synchronize_rcu() from cgroup_attach_{task|proc}()

[Li Zefan]

These 2 syncronize_rcu()s make attaching a task to a cgroup
quite slow, and it can't be ignored in some situations.

A real case from Colin Cross: Android uses cgroups heavily to
manage thread priorities, putting threads in a background group
with reduced cpu.shares when they are not visible to the user,
and in a foreground group when they are. Some RPCs from foreground
threads to background threads will temporarily move the background
thread into the foreground group for the duration of the RPC.
This results in many calls to cgroup_attach_task.

In cgroup_attach_task() it's task->cgroups that is protected by RCU,
and put_css_set() calls kfree_rcu() to free it.

If we remove this synchronize_rcu(), there can be threads in RCU-read
sections accessing their old cgroup via current->cgroups with
concurrent rmdir operation, but this is safe.

 # time for ((i=0; i<50; i++)) { echo $$ > /mnt/sub/tasks; echo $$ > /mnt/tasks; }

real    0m2.524s
user    0m0.008s
sys     0m0.004s

With this patch:

real    0m0.004s
user    0m0.004s
sys     0m0.000s

Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index a5262d9..67b0fa0 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1974,7 +1974,6 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
 			ss->attach(cgrp, &tset);
 	}
 
-	synchronize_rcu();
 out:
 	if (retval) {
 		for_each_subsys(root, ss) {
@@ -2143,7 +2142,6 @@ static int cgroup_attach_proc(struct cgroup *cgrp, struct task_struct *leader)
 	/*
 	 * step 5: success! and cleanup
 	 */
-	synchronize_rcu();
 	retval = 0;
 out_put_css_set_refs:
 	if (retval) {
-- 
1.8.0.2

[PATCH 2/2] cgroup: remove synchronize_rcu() from rebind_subsystems()

[Li Zefan]

Nothing's protected by RCU in rebind_subsystems(), and I can't think
of a reason why it is needed.

Signed-off-by: Li Zefan <lizefan@huawei.com>
---
 kernel/cgroup.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 67b0fa0..f43c929 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1079,7 +1079,6 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 		}
 	}
 	root->subsys_mask = root->actual_subsys_mask = final_subsys_mask;
-	synchronize_rcu();
 
 	return 0;
 }
-- 
1.8.0.2

[PATCH 2/2] cgroup: remove synchronize_rcu() from rebind_subsystems()

[Li Zefan]

Nothing's protected by RCU in rebind_subsystems(), and I can't think
of a reason why it is needed.

Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
---
 kernel/cgroup.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 67b0fa0..f43c929 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -1079,7 +1079,6 @@ static int rebind_subsystems(struct cgroupfs_root *root,
 		}
 	}
 	root->subsys_mask = root->actual_subsys_mask = final_subsys_mask;
-	synchronize_rcu();
 
 	return 0;
 }
-- 
1.8.0.2

Re: [PATCH 1/2] cgroup: remove synchronize_rcu() from cgroup_attach_{task|proc}()

[Tejun Heo]

Hey, Li.

On Mon, Jan 14, 2013 at 05:23:26PM +0800, Li Zefan wrote:
> These 2 syncronize_rcu()s make attaching a task to a cgroup
> quite slow, and it can't be ignored in some situations.
> 
> A real case from Colin Cross: Android uses cgroups heavily to
> manage thread priorities, putting threads in a background group
> with reduced cpu.shares when they are not visible to the user,
> and in a foreground group when they are. Some RPCs from foreground
> threads to background threads will temporarily move the background
> thread into the foreground group for the duration of the RPC.
> This results in many calls to cgroup_attach_task.
> 
> In cgroup_attach_task() it's task->cgroups that is protected by RCU,
> and put_css_set() calls kfree_rcu() to free it.
> 
> If we remove this synchronize_rcu(), there can be threads in RCU-read
> sections accessing their old cgroup via current->cgroups with
> concurrent rmdir operation, but this is safe.

Yeah, these synchronize_rcu()s are utterly confused.
synchornize_rcu() necessarily have to come between two operations to
guarantee that the changes made before it are visible to all rcu
readers before proceeding beyond it.  Here, synchornize_rcu() are at
the end of attach operations with nothing beyond it.  Duh.... its only
effect would be delaying completion of write(2) to sysfs tasks/procs
files until all rcu readers see the change, which is utterly
irrelevant.  It doesn't mean anything.

Applying to cgroup/for-3.9.

Thanks.

-- 
tejun

Re: [PATCH 1/2] cgroup: remove synchronize_rcu() from cgroup_attach_{task|proc}()

[Tejun Heo]

Hey, Li.

On Mon, Jan 14, 2013 at 05:23:26PM +0800, Li Zefan wrote:
> These 2 syncronize_rcu()s make attaching a task to a cgroup
> quite slow, and it can't be ignored in some situations.
> 
> A real case from Colin Cross: Android uses cgroups heavily to
> manage thread priorities, putting threads in a background group
> with reduced cpu.shares when they are not visible to the user,
> and in a foreground group when they are. Some RPCs from foreground
> threads to background threads will temporarily move the background
> thread into the foreground group for the duration of the RPC.
> This results in many calls to cgroup_attach_task.
> 
> In cgroup_attach_task() it's task->cgroups that is protected by RCU,
> and put_css_set() calls kfree_rcu() to free it.
> 
> If we remove this synchronize_rcu(), there can be threads in RCU-read
> sections accessing their old cgroup via current->cgroups with
> concurrent rmdir operation, but this is safe.

Yeah, these synchronize_rcu()s are utterly confused.
synchornize_rcu() necessarily have to come between two operations to
guarantee that the changes made before it are visible to all rcu
readers before proceeding beyond it.  Here, synchornize_rcu() are at
the end of attach operations with nothing beyond it.  Duh.... its only
effect would be delaying completion of write(2) to sysfs tasks/procs
files until all rcu readers see the change, which is utterly
irrelevant.  It doesn't mean anything.

Applying to cgroup/for-3.9.

Thanks.

-- 
tejun

Re: [PATCH 2/2] cgroup: remove synchronize_rcu() from rebind_subsystems()

[Tejun Heo]

On Mon, Jan 14, 2013 at 05:24:18PM +0800, Li Zefan wrote:
> Nothing's protected by RCU in rebind_subsystems(), and I can't think
> of a reason why it is needed.
> 
> Signed-off-by: Li Zefan <lizefan@huawei.com>

Applied to cgroup/for-3.9.  We probably should synchornize_rcu() from
cgroup_diput() too.  Probably by punting css_free() and cgrp freeing
to a work item from call_rcu() callback.

Thanks.

-- 
tejun

Re: [PATCH 2/2] cgroup: remove synchronize_rcu() from rebind_subsystems()

[Tejun Heo]

On Mon, Jan 14, 2013 at 05:24:18PM +0800, Li Zefan wrote:
> Nothing's protected by RCU in rebind_subsystems(), and I can't think
> of a reason why it is needed.
> 
> Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>

Applied to cgroup/for-3.9.  We probably should synchornize_rcu() from
cgroup_diput() too.  Probably by punting css_free() and cgrp freeing
to a work item from call_rcu() callback.

Thanks.

-- 
tejun

Re: [PATCH 2/2] cgroup: remove synchronize_rcu() from rebind_subsystems()

[Tejun Heo]

On Mon, Jan 14, 2013 at 10:55:12AM -0800, Tejun Heo wrote:
> On Mon, Jan 14, 2013 at 05:24:18PM +0800, Li Zefan wrote:
> > Nothing's protected by RCU in rebind_subsystems(), and I can't think
> > of a reason why it is needed.
> > 
> > Signed-off-by: Li Zefan <lizefan@huawei.com>
> 
> Applied to cgroup/for-3.9.  We probably should synchornize_rcu() from
                                                ^
						remove

> cgroup_diput() too.  Probably by punting css_free() and cgrp freeing
> to a work item from call_rcu() callback.

-- 
tejun

Re: [PATCH 2/2] cgroup: remove synchronize_rcu() from rebind_subsystems()

[Tejun Heo]

On Mon, Jan 14, 2013 at 10:55:12AM -0800, Tejun Heo wrote:
> On Mon, Jan 14, 2013 at 05:24:18PM +0800, Li Zefan wrote:
> > Nothing's protected by RCU in rebind_subsystems(), and I can't think
> > of a reason why it is needed.
> > 
> > Signed-off-by: Li Zefan <lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> 
> Applied to cgroup/for-3.9.  We probably should synchornize_rcu() from
                                                ^
						remove

> cgroup_diput() too.  Probably by punting css_free() and cgrp freeing
> to a work item from call_rcu() callback.

-- 
tejun