Messages in this thread | | | Date | Wed, 29 Feb 2012 17:21:52 +0100 | From | Frederic Weisbecker <> | Subject | Re: [RFC][PATCH v2] cgroups: Run subsystem fork callback from cgroup_post_fork() |
| |
On Wed, Feb 29, 2012 at 07:55:00AM -0800, Mandeep Singh Baines wrote: > Frederic Weisbecker (fweisbec@gmail.com) wrote: > > When a user freezes a cgroup, the freezer sets the subsystem state > > to CGROUP_FREEZING and then iterates over the tasks in the cgroup links. > > > > But there is a possible race here, although unlikely, if a task > > forks and the parent is preempted between write_unlock(tasklist_lock) > > and cgroup_post_fork(). If we freeze the cgroup while the parent > > So what if you moved cgroup_post_forks() a few lines up to be > inside the tasklist_lock?
It won't work. Consider this scenario:
CPU 0 CPU 1
cgroup_fork_callbacks() write_lock(tasklist_lock) try_to_freeze_cgroup() { add child to task list etc... cgroup_iter_start() freeze tasks cgroup_iter_end() } cgroup_post_fork() write_unlock(tasklist_lock)
If this is not the first time we call cgroup_iter_start(), we won't go through the whole tasklist, we simply iterate through the css set task links.
Plus we try to avoid anything under tasklist_lock when possible.
> > I agree with you on the race and believe your solution is correct. > > > is sleeping and the parent wakes up thereafter, its child will > > be missing from the set of tasks to freeze because: > > > > - The child was not yet linked to its css_set->tasks, as is done > > from cgroup_post_fork(). cgroup_iter_start() has thus missed it. > > > > - The cgroup freezer's fork callback can handle that child but > > cgroup_fork_callbacks() has been called already. > > > > One way to fix this is to call the fork callbacks after we link > > the task to the css set. The cgroup freezer is the only user of > > this callback anyway. > > > > v2: Keep the call to cgroup_exit to put the css_set on fork error. > > > > Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com> > > Cc: Li Zefan <lizf@cn.fujitsu.com> > > Cc: Tejun Heo <tj@kernel.org> > > Cc: Oleg Nesterov <oleg@redhat.com> > > Cc: Andrew Morton <akpm@linux-foundation.org> > > Cc: Mandeep Singh Baines <msb@chromium.org> > > --- > > > > Not sure this is the right solution, especially as I still need > > a cancellable fork callback for my task counter and for this I > > need the fork callbacks to be called before the task is added > > on the tasklist. But anyway at least that reports this race. > > > > I'm new to the task counter stuff. Would you mind providing a > reference.
Sure, have a look at this:
https://lkml.org/lkml/2012/1/31/489
Especially this patch: https://lkml.org/lkml/2012/1/31/495
And this one that implements a fork callback: https://lkml.org/lkml/2012/1/31/497
The fork callback may return an error to cancel the fork. But doing this at cgroup_post_fork() time is too late.
| |