Re: [PATCH 1/3] cgroups: make root_list contains active hierarchies only

[Li Zefan <lizf@cn.fujitsu.com>]

Paul Menage wrote:
> On Fri, Nov 28, 2008 at 2:02 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> Don't link rootnode to the root list, so root_list contains active
>> hierarchies only as the comment indicates. And rename for_each_root()
>> to for_each_active_root().
> 
> Why is that change an improvement? If you're concerned about the
> comment's accuracy then it would be better to just change it to say "A
> list running through all hierarchies (including the inactive subsystem
> hierarchy)"?
> 

I noticed the comments for root_list and for_each_root() conflicts with
the code. But I don't have strong option whether to change the code or
change the comments. If you vote for the latter, I'll rewrite the patch.

> Also, with this change, "root_count" is no longer the length of the
> "roots" list, but instead is length + 1, which doesn't seem good.
> 

Or rename 'roots' to 'active_roots'?

> Paul
> 
>> Also remove redundant check in cgroup_kill_sb().
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  kernel/cgroup.c |   19 +++++++------------
>>  1 files changed, 7 insertions(+), 12 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index fe00b3b..33ba756 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -84,7 +84,7 @@ struct cgroupfs_root {
>>        /* Tracks how many cgroups are currently defined in hierarchy.*/
>>        int number_of_cgroups;
>>
>> -       /* A list running through the mounted hierarchies */
>> +       /* A list running through the active hierarchies */
>>        struct list_head root_list;
>>
>>        /* Hierarchy-specific flags */
>> @@ -149,8 +149,8 @@ static int notify_on_release(const struct cgroup *cgrp)
>>  #define for_each_subsys(_root, _ss) \
>>  list_for_each_entry(_ss, &_root->subsys_list, sibling)
>>
>> -/* for_each_root() allows you to iterate across the active hierarchies */
>> -#define for_each_root(_root) \
>> +/* for_each_active_root() allows you to iterate across the active hierarchies */
>> +#define for_each_active_root(_root) \
>>  list_for_each_entry(_root, &roots, root_list)
>>
>>  /* the list of cgroups eligible for automatic release. Protected by
>> @@ -1114,10 +1114,9 @@ static void cgroup_kill_sb(struct super_block *sb) {
>>        }
>>        write_unlock(&css_set_lock);
>>
>> -       if (!list_empty(&root->root_list)) {
>> -               list_del(&root->root_list);
>> -               root_count--;
>> -       }
>> +       list_del(&root->root_list);
>> +       root_count--;
>> +
>>        mutex_unlock(&cgroup_mutex);
>>
>>        kfree(root);
>> @@ -2561,7 +2560,6 @@ int __init cgroup_init_early(void)
>>        INIT_HLIST_NODE(&init_css_set.hlist);
>>        css_set_count = 1;
>>        init_cgroup_root(&rootnode);
>> -       list_add(&rootnode.root_list, &roots);
>>        root_count = 1;
>>        init_task.cgroups = &init_css_set;
>>
>> @@ -2668,15 +2666,12 @@ static int proc_cgroup_show(struct seq_file *m, void *v)
>>
>>        mutex_lock(&cgroup_mutex);
>>
>> -       for_each_root(root) {
>> +       for_each_active_root(root) {
>>                struct cgroup_subsys *ss;
>>                struct cgroup *cgrp;
>>                int subsys_id;
>>                int count = 0;
>>
>> -               /* Skip this hierarchy if it has no active subsystems */
>> -               if (!root->actual_subsys_bits)
>> -                       continue;
>>                seq_printf(m, "%lu:", root->subsys_bits);
>>                for_each_subsys(root, ss)
>>                        seq_printf(m, "%s%s", count++ ? "," : "", ss->name);
>> --
>> 1.5.4.rc3
>>
>>
>