Messages in this thread Patch in this message | | | Date | Sun, 18 May 2008 01:45:45 +0530 | From | Balbir Singh <> | Subject | Re: [-mm][PATCH 4/4] Add memrlimit controller accounting and control (v4) |
| |
* Paul Menage <menage@google.com> [2008-05-15 08:28:46]:
> On Thu, May 15, 2008 at 1:25 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote: > > > > > > But the only *new* cases of taking the mmap_sem that this would > > > introduce would be: > > > > > > - on a failed vm limit charge > > > > Why a failed charge? Aren't we talking of moving all charge/uncharge > > under mmap_sem? > > > > Sorry, I worded that wrongly - I meant "cleaning up a successful > charge after an expansion fails for other reasons" > > I thought that all the charges and most of the uncharges were already > under mmap_sem, and it would just be a few of the cleanup paths that > needed to take it. > > > > > > - when a task moves between two cgroups in the memrlimit hierarchy. > > > > > > > Yes, this would nest cgroup_mutex and mmap_sem. Not sure if that would > > be a bad side-effect. > > > > I think it's already nested that way - e.g. the cpusets code can call > various migration functions (which take mmap_sem) while holding > cgroup_mutex. > > > > > Refactor the code to try and use mmap_sem and see what I come up > > with. Basically use mmap_sem for all charge/uncharge operations as > > well use mmap_sem in read_mode in the move_task() and > > mm_owner_changed() callbacks. That should take care of the race > > conditions discussed, unless I missed something. > > Sounds good. > > Thanks, > I've revamped the last two patches. Please review
This patch adds an additional field to the mm_owner callbacks. This field is required to get to the mm that changed. Hold mmap_sem in write mode before calling the mm_owner_changed callback
Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com> ---
include/linux/cgroup.h | 3 ++- kernel/cgroup.c | 4 +++- kernel/exit.c | 3 +++ 3 files changed, 8 insertions(+), 2 deletions(-)
diff -puN include/linux/cgroup.h~cgroup-add-task-to-mm-owner-callbacks include/linux/cgroup.h --- linux-2.6.26-rc2/include/linux/cgroup.h~cgroup-add-task-to-mm-owner-callbacks 2008-05-14 18:36:59.000000000 +0530 +++ linux-2.6.26-rc2-balbir/include/linux/cgroup.h 2008-05-14 18:36:59.000000000 +0530 @@ -310,7 +310,8 @@ struct cgroup_subsys { */ void (*mm_owner_changed)(struct cgroup_subsys *ss, struct cgroup *old, - struct cgroup *new); + struct cgroup *new, + struct task_struct *p); int subsys_id; int active; int disabled; diff -puN kernel/cgroup.c~cgroup-add-task-to-mm-owner-callbacks kernel/cgroup.c --- linux-2.6.26-rc2/kernel/cgroup.c~cgroup-add-task-to-mm-owner-callbacks 2008-05-14 18:36:59.000000000 +0530 +++ linux-2.6.26-rc2-balbir/kernel/cgroup.c 2008-05-17 22:09:57.000000000 +0530 @@ -2758,6 +2758,8 @@ void cgroup_fork_callbacks(struct task_s * Called on every change to mm->owner. mm_init_owner() does not * invoke this routine, since it assigns the mm->owner the first time * and does not change it. + * + * The callbacks are invoked with mmap_sem held in read mode. */ void cgroup_mm_owner_callbacks(struct task_struct *old, struct task_struct *new) { @@ -2772,7 +2774,7 @@ void cgroup_mm_owner_callbacks(struct ta if (oldcgrp == newcgrp) continue; if (ss->mm_owner_changed) - ss->mm_owner_changed(ss, oldcgrp, newcgrp); + ss->mm_owner_changed(ss, oldcgrp, newcgrp, new); } } } diff -puN kernel/exit.c~cgroup-add-task-to-mm-owner-callbacks kernel/exit.c --- linux-2.6.26-rc2/kernel/exit.c~cgroup-add-task-to-mm-owner-callbacks 2008-05-17 22:10:00.000000000 +0530 +++ linux-2.6.26-rc2-balbir/kernel/exit.c 2008-05-17 23:14:44.000000000 +0530 @@ -621,6 +621,7 @@ retry: assign_new_owner: BUG_ON(c == p); get_task_struct(c); + down_write(&mm->mmap_sem); /* * The task_lock protects c->mm from changing. * We always want mm->owner->mm == mm @@ -634,12 +635,14 @@ assign_new_owner: if (c->mm != mm) { task_unlock(c); put_task_struct(c); + up_write(&mm->mmap_sem); goto retry; } cgroup_mm_owner_callbacks(mm->owner, c); mm->owner = c; task_unlock(c); put_task_struct(c); + up_write(&mm->mmap_sem); } #endif /* CONFIG_MM_OWNER */ _
-- Warm Regards, Balbir Singh Linux Technology Center IBM, ISTL
| |