Re: [PATCH 1/2] cgroup: remove synchronize_rcu() fromcgroup_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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/