Re: [PATCH 5/6] Lets ss->can_attach and ss->attach do whole threadgroups at a time

[Benjamin Blum <bblum@google.com>]

On Sun, Aug 2, 2009 at 7:22 PM, Li Zefan<lizf@cn.fujitsu.com> wrote:
> 09:51, Ben Blum wrote:
>> Lets ss->can_attach and ss->attach do whole threadgroups at a time
>>
>> This patch alters the ss->can_attach and ss->attach functions to be able to
>> deal with a whole threadgroup at a time, for use in cgroup_attach_proc. (This
>> is a pre-patch to cgroup-procs-writable.patch.)
>>
>> Currently, new mode of the attach function can only tell the subsystem about
>> the old cgroup of the threadgroup leader. No subsystem currently needs that
>> information for each thread that's being moved, but if one were to be added
>> (for example, one that counts tasks within a group) this bit would need to be
>> reworked a bit to tell the subsystem the right information.
>>
>> Signed-off-by: Ben Blum <bblum@google.com>
>>
>> ---
>>
>>  include/linux/cgroup.h   |    7 +++--
>>  kernel/cgroup.c          |    4 +--
>>  kernel/cgroup_freezer.c  |   15 +++++++++--
>>  kernel/cpuset.c          |   65 ++++++++++++++++++++++++++++++++++++----------
>>  kernel/ns_cgroup.c       |   16 ++++++++++-
>>  kernel/sched.c           |   37 ++++++++++++++++++++++++--
>>  mm/memcontrol.c          |    3 +-
>>  security/device_cgroup.c |    3 +-
>>  8 files changed, 122 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
>> index 24e3f1a..8286758 100644
>> --- a/include/linux/cgroup.h
>> +++ b/include/linux/cgroup.h
>> @@ -408,10 +408,11 @@ struct cgroup_subsys {
>>                                                 struct cgroup *cgrp);
>>       int (*pre_destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
>>       void (*destroy)(struct cgroup_subsys *ss, struct cgroup *cgrp);
>> -     int (*can_attach)(struct cgroup_subsys *ss,
>> -                       struct cgroup *cgrp, struct task_struct *tsk);
>> +     int (*can_attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
>> +                       struct task_struct *tsk, bool threadgroup);
>>       void (*attach)(struct cgroup_subsys *ss, struct cgroup *cgrp,
>> -                     struct cgroup *old_cgrp, struct task_struct *tsk);
>> +                     struct cgroup *old_cgrp, struct task_struct *tsk,
>> +                     bool threadgroup);
>
> Could you also update Documentation/cgroups/cgroups.txt?
>
>>       void (*fork)(struct cgroup_subsys *ss, struct task_struct *task);
>>       void (*exit)(struct cgroup_subsys *ss, struct task_struct *task);
>>       int (*populate)(struct cgroup_subsys *ss,
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index a12bc8e..ea05d6b 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -1324,7 +1324,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>>
>>       for_each_subsys(root, ss) {
>>               if (ss->can_attach) {
>> -                     retval = ss->can_attach(ss, cgrp, tsk);
>> +                     retval = ss->can_attach(ss, cgrp, tsk, false);
>>                       if (retval)
>>                               return retval;
>>               }
>> @@ -1362,7 +1362,7 @@ int cgroup_attach_task(struct cgroup *cgrp, struct task_struct *tsk)
>>
>>       for_each_subsys(root, ss) {
>>               if (ss->attach)
>> -                     ss->attach(ss, cgrp, oldcgrp, tsk);
>> +                     ss->attach(ss, cgrp, oldcgrp, tsk, false);
>>       }
>>       set_bit(CGRP_RELEASABLE, &oldcgrp->flags);
>>       synchronize_rcu();
>> diff --git a/kernel/cgroup_freezer.c b/kernel/cgroup_freezer.c
>> index fb249e2..4e352ab 100644
>> --- a/kernel/cgroup_freezer.c
>> +++ b/kernel/cgroup_freezer.c
>> @@ -159,10 +159,9 @@ static bool is_task_frozen_enough(struct task_struct *task)
>>   */
>>  static int freezer_can_attach(struct cgroup_subsys *ss,
>>                             struct cgroup *new_cgroup,
>> -                           struct task_struct *task)
>> +                           struct task_struct *task, bool threadgroup)
>>  {
>>       struct freezer *freezer;
>> -
>
> Please reserve this blank line.
>
>>       /*
>>        * Anything frozen can't move or be moved to/from.
>>        *
>> @@ -177,6 +176,18 @@ static int freezer_can_attach(struct cgroup_subsys *ss,
>>       if (freezer->state == CGROUP_FROZEN)
>>               return -EBUSY;
>>
>> +     if (threadgroup) {
>> +             struct task_struct *c;
>
> And better have a blank line here. ;)
>
>> +             rcu_read_lock();
>> +             list_for_each_entry_rcu(c, &task->thread_group, thread_group) {
>> +                     if (is_task_frozen_enough(c)) {
>> +                             rcu_read_unlock();
>> +                             return -EBUSY;
>> +                     }
>> +             }
>> +             rcu_read_unlock();
>> +     }
>> +
>>       return 0;
>>  }
>>
>> diff --git a/kernel/cpuset.c b/kernel/cpuset.c
>> index 7e75a41..86397f4 100644
>> --- a/kernel/cpuset.c
>> +++ b/kernel/cpuset.c
>> @@ -1324,9 +1324,10 @@ static int fmeter_getrate(struct fmeter *fmp)
>>  static cpumask_var_t cpus_attach;
>>
>>  /* Called by cgroups to determine if a cpuset is usable; cgroup_mutex held */
>> -static int cpuset_can_attach(struct cgroup_subsys *ss,
>> -                          struct cgroup *cont, struct task_struct *tsk)
>> +static int cpuset_can_attach(struct cgroup_subsys *ss, struct cgroup *cont,
>> +                          struct task_struct *tsk, bool threadgroup)
>>  {
>> +     int ret;
>>       struct cpuset *cs = cgroup_cs(cont);
>>
>>       if (cpumask_empty(cs->cpus_allowed) || nodes_empty(cs->mems_allowed))
>> @@ -1343,18 +1344,50 @@ static int cpuset_can_attach(struct cgroup_subsys *ss,
>>       if (tsk->flags & PF_THREAD_BOUND)
>>               return -EINVAL;
>>
>> -     return security_task_setscheduler(tsk, 0, NULL);
>> +     ret = security_task_setscheduler(tsk, 0, NULL);
>> +     if (ret)
>> +             return ret;
>> +     if (threadgroup) {
>> +             struct task_struct *c;
>
> ditto
>
>> +             rcu_read_lock();
>> +             list_for_each_entry_rcu(c, &tsk->thread_group, thread_group) {
>> +                     ret = security_task_setscheduler(c, 0, NULL);
>> +                     if (ret) {
>> +                             rcu_read_unlock();
>> +                             return ret;
>> +                     }
>> +             }
>> +             rcu_read_unlock();
>> +     }
>> +     return 0;
>> +}
>
>

All taken care of. Thanks.