Re: [-mm][PATCH 4/4] Add memrlimit controller accounting andcontrol (v4)

From: Balbir Singh
Date: Sat May 17 2008 - 16:16:49 EST


* Paul Menage <menage@xxxxxxxxxx> [2008-05-15 08:28:46]:

> On Thu, May 15, 2008 at 1:25 AM, Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
---

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
--
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/