Re: [PATCH 2/2] cgroups: convert ss->attach to use whole threadgroup flex_array (cpuset, memcontrol)

[Paul Menage <paul@paulmenage.org>]

On Thu, Oct 13, 2011 at 5:36 PM, Ben Blum <bblum@andrew.cmu.edu> wrote:
> Convert ss->attach to take a flex_array of tasks instead of just the leader.
>
> From: Ben Blum <bblum@andrew.cmu.edu>
>
> This lets subsystems with whole-threadgroup attach calls (i.e., cpuset and
> memcontrol) to accurately find the group's mm even when a non-leader does exec
> and leaves the leader with a NULL mm pointer.
>
> Also converts cpuset and memcontrol to take the flex_array and iterate down it
> until an mm is found, instead of just attempting to use the leader's mm.
>
> Signed-off-by: Ben Blum <bblum@andrew.cmu.edu>

Reviewed-by: Paul Menage <paul@paulmenage.org>

Thanks,
Paul

> ---
>  Documentation/cgroups/cgroups.txt |    7 ++++++-
>  include/linux/cgroup.h            |    4 +++-
>  kernel/cgroup.c                   |   16 +++++++++++++---
>  kernel/cpuset.c                   |   16 ++++++++++++++--
>  mm/memcontrol.c                   |   17 +++++++++++++++--
>  5 files changed, 51 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/cgroups/cgroups.txt b/Documentation/cgroups/cgroups.txt
> index 3fa646f..8e900ec 100644
> --- a/Documentation/cgroups/cgroups.txt
> +++ b/Documentation/cgroups/cgroups.txt
> @@ -637,12 +637,17 @@ For any non-per-thread attachment work that needs to happen before
>  attach_task. Needed by cpuset.
>
>  void attach(struct cgroup_subsys *ss, struct cgroup *cgrp,
> -           struct cgroup *old_cgrp, struct task_struct *task)
> +           struct cgroup *old_cgrp, struct flex_array *group,
> +           int group_size)

Maybe flex_array should have a size field, so that you can just pass a
single pointer here?

> @@ -1452,7 +1455,16 @@ static void cpuset_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>         */
>        cpuset_attach_nodemask_from = oldcs->mems_allowed;
>        cpuset_attach_nodemask_to = cs->mems_allowed;
> -       mm = get_task_mm(tsk);
> +       /*
> +        * Find the first task in the group that still has its mm. (This could
> +        * be not the first one if another did exec() and the leader exited.

Maybe rephrase this as "This might not be the first one"?

Paul