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

[Paul Menage]

On Fri, Nov 28, 2008 at 2:02 AM, Li Zefan <lizf at 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)"?

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.

Paul

>
> Also remove redundant check in cgroup_kill_sb().
>
> Signed-off-by: Li Zefan <lizf at 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
>
>
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers