cgroups: add inactive subsystems to rootnode.root_list

Though for an inactive hierarchy, we have subsys->root == &rootnode, but rootnode's root_list is always empty.

The above code assumes rootnode.subsys_list links all inactive hierarchies.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

kernel/cgroup.c +2 -2

index 33ba756..4e50e97 100644
@@ -716,7 +716,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
                       cgrp->subsys[i] = dummytop->subsys[i];
                       cgrp->subsys[i]->cgroup = cgrp;
-                       list_add(&ss->sibling, &root->subsys_list);
+                       list_move(&ss->sibling, &root->subsys_list);
                       rcu_assign_pointer(ss->root, root);
                       if (ss->bind)
                               ss->bind(ss, cgrp);

@@ -730,7 +730,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       dummytop->subsys[i]->cgroup = dummytop;
                       cgrp->subsys[i] = NULL;
                       rcu_assign_pointer(subsys[i]->root, &rootnode);
-                       list_del(&ss->sibling);
+                       list_move(&ss->sibling, &rootnode.subsys_list);
               } else if (bit & final_bits) {
                       /* Subsystem state should already exist */
                       BUG_ON(!cgrp->subsys[i]);

Though for an inactive hierarchy, we have subsys->root == &rootnode, but rootnode's root_list is always empty.

The above code assumes rootnode.subsys_list links all inactive hierarchies.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

kernel/cgroup.c +2 -2

index 33ba756..4e50e97 100644
@@ -716,7 +716,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
                       cgrp->subsys[i] = dummytop->subsys[i];
                       cgrp->subsys[i]->cgroup = cgrp;
-                       list_add(&ss->sibling, &root->subsys_list);
+                       list_move(&ss->sibling, &root->subsys_list);
                       rcu_assign_pointer(ss->root, root);
                       if (ss->bind)
                               ss->bind(ss, cgrp);

@@ -730,7 +730,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       dummytop->subsys[i]->cgroup = dummytop;
                       cgrp->subsys[i] = NULL;
                       rcu_assign_pointer(subsys[i]->root, &rootnode);
-                       list_del(&ss->sibling);
+                       list_move(&ss->sibling, &rootnode.subsys_list);
               } else if (bit & final_bits) {
                       /* Subsystem state should already exist */
                       BUG_ON(!cgrp->subsys[i]);

Quoted 2 lines · Li Zefan

Though for an inactive hierarchy, we have subsys->root == &rootnode, but rootnode's root_list is always empty.

I think you mean "rootnode's subsys_list is always empty".

Quoted 33 lines

The above code assumes rootnode.subsys_list links all inactive hierarchies.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

kernel/cgroup.c +2 -2

index 33ba756..4e50e97 100644
@@ -716,7 +716,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
                       cgrp->subsys[i] = dummytop->subsys[i];
                       cgrp->subsys[i]->cgroup = cgrp;
-                       list_add(&ss->sibling, &root->subsys_list);
+                       list_move(&ss->sibling, &root->subsys_list);
                       rcu_assign_pointer(ss->root, root);
                       if (ss->bind)
                               ss->bind(ss, cgrp);

@@ -730,7 +730,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       dummytop->subsys[i]->cgroup = dummytop;
                       cgrp->subsys[i] = NULL;
                       rcu_assign_pointer(subsys[i]->root, &rootnode);
-                       list_del(&ss->sibling);
+                       list_move(&ss->sibling, &rootnode.subsys_list);
               } else if (bit & final_bits) {
                       /* Subsystem state should already exist */
                       BUG_ON(!cgrp->subsys[i]);

@@ -2522,6 +2522,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss)
       printk(KERN_INFO "Initializing cgroup subsys %s\n", ss->name);
 

kernel/cgroup.c static void __init cgroup_init_subsys(struct cgroup_subsys *ss) · line 2525

Quoted 2 lines

kernel/cgroup.c +1 -0

       /* Create the top cgroup state for this subsystem */
+       list_add(&ss->sibling, &rootnode.root_list);

This is wrong. cgroup_subsys.sibling is a member of the list of subsystems attached to a given root, and rootnode.root_list is a member of the list of hierarchies. Did you actually meant &rootnode.subsys_list?

Quoted 6 lines · Paul Menage

Quoted 2 lines · Li Zefan

Though for an inactive hierarchy, we have subsys->root == &rootnode, but rootnode's root_list is always empty.

I think you mean "rootnode's subsys_list is always empty".

Yes, I mean subsys_list not root_list.

Quoted 33 lines

Quoted 33 lines

The above code assumes rootnode.subsys_list links all inactive hierarchies.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

kernel/cgroup.c +2 -2

index 33ba756..4e50e97 100644
@@ -716,7 +716,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
                       cgrp->subsys[i] = dummytop->subsys[i];
                       cgrp->subsys[i]->cgroup = cgrp;
-                       list_add(&ss->sibling, &root->subsys_list);
+                       list_move(&ss->sibling, &root->subsys_list);
                       rcu_assign_pointer(ss->root, root);
                       if (ss->bind)
                               ss->bind(ss, cgrp);

@@ -730,7 +730,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       dummytop->subsys[i]->cgroup = dummytop;
                       cgrp->subsys[i] = NULL;
                       rcu_assign_pointer(subsys[i]->root, &rootnode);
-                       list_del(&ss->sibling);
+                       list_move(&ss->sibling, &rootnode.subsys_list);
               } else if (bit & final_bits) {
                       /* Subsystem state should already exist */
                       BUG_ON(!cgrp->subsys[i]);

@@ -2522,6 +2522,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss)
       printk(KERN_INFO "Initializing cgroup subsys %s\n", ss->name);
 

Quoted 8 lines

kernel/cgroup.c static void __init cgroup_init_subsys(struct cgroup_subsys *ss) · line 2525

Quoted 2 lines

kernel/cgroup.c +1 -0

       /* Create the top cgroup state for this subsystem */
+       list_add(&ss->sibling, &rootnode.root_list);

This is wrong. cgroup_subsys.sibling is a member of the list of subsystems attached to a given root, and rootnode.root_list is a member of the list of hierarchies. Did you actually meant &rootnode.subsys_list?

I thought I corrected all the s/root_list/subsys_list/ in patch v2. :(

If this patch makes sense, I'll fix it, otherwise we'll going to ask Andrew to drop it from -mm.

Quoted 6 lines · Paul Menage

Quoted 2 lines · Li Zefan

Though for an inactive hierarchy, we have subsys->root == &rootnode, but rootnode's root_list is always empty.

I think you mean "rootnode's subsys_list is always empty".

Yes, I mean subsys_list not root_list.

Quoted 33 lines

Quoted 33 lines

The above code assumes rootnode.subsys_list links all inactive hierarchies.

Signed-off-by Li Zefan <[email protected]>

---
 kernel/cgroup.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

kernel/cgroup.c +2 -2

index 33ba756..4e50e97 100644
@@ -716,7 +716,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       BUG_ON(dummytop->subsys[i]->cgroup != dummytop);
                       cgrp->subsys[i] = dummytop->subsys[i];
                       cgrp->subsys[i]->cgroup = cgrp;
-                       list_add(&ss->sibling, &root->subsys_list);
+                       list_move(&ss->sibling, &root->subsys_list);
                       rcu_assign_pointer(ss->root, root);
                       if (ss->bind)
                               ss->bind(ss, cgrp);

@@ -730,7 +730,7 @@ static int rebind_subsystems(struct cgroupfs_root *root,
                       dummytop->subsys[i]->cgroup = dummytop;
                       cgrp->subsys[i] = NULL;
                       rcu_assign_pointer(subsys[i]->root, &rootnode);
-                       list_del(&ss->sibling);
+                       list_move(&ss->sibling, &rootnode.subsys_list);
               } else if (bit & final_bits) {
                       /* Subsystem state should already exist */
                       BUG_ON(!cgrp->subsys[i]);

@@ -2522,6 +2522,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss)
       printk(KERN_INFO "Initializing cgroup subsys %s\n", ss->name);
 

Quoted 8 lines

kernel/cgroup.c static void __init cgroup_init_subsys(struct cgroup_subsys *ss) · line 2525

Quoted 2 lines

kernel/cgroup.c +1 -0

       /* Create the top cgroup state for this subsystem */
+       list_add(&ss->sibling, &rootnode.root_list);

This is wrong. cgroup_subsys.sibling is a member of the list of subsystems attached to a given root, and rootnode.root_list is a member of the list of hierarchies. Did you actually meant &rootnode.subsys_list?

I thought I corrected all the s/root_list/subsys_list/ in patch v2. :(

If this patch makes sense, I'll fix it, otherwise we'll going to ask Andrew to drop it from -mm.

Quoted 10 lines

Quoted 10 lines

kernel/cgroup.c static void __init cgroup_init_subsys(struct cgroup_subsys *ss) · line 2525

Quoted 5 lines

kernel/cgroup.c +1 -0

@@ -2522,6 +2522,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss)
       printk(KERN_INFO "Initializing cgroup subsys %s\n", ss->name);
 
       /* Create the top cgroup state for this subsystem */
+       list_add(&ss->sibling, &rootnode.root_list);

This is wrong. cgroup_subsys.sibling is a member of the list of subsystems attached to a given root, and rootnode.root_list is a member of the list of hierarchies. Did you actually meant &rootnode.subsys_list?

Quoted 3 lines

I thought I corrected all the s/root_list/subsys_list/ in patch v2. :(

If this patch makes sense, I'll fix it, otherwise we'll going to ask Andrew to drop it from -mm.

Looking at the existing code, it's not actually broken - but only by accident.

We aim to add a link for each cgroup, which we do by finding the first subsystem in each hierarchy and adding a link for its cgroup, and then adding a link for dummytop if there weren't any subsystems in the inactive hierarchy.

But since rootnode.subsys_list is always empty, we always skip the inactive hierarchy in the main loop (if it has any subsystems), and always add the inactive hierarchy afterwards. So everything works out OK (all hierarchies do get linked), but it's horribly unreadable.

Having the subsys_list be correct for the inactive hierarchy is probably a good thing, so this patch should go ahead (once the typos is fixed).

Quoted 10 lines

Quoted 10 lines

kernel/cgroup.c static void __init cgroup_init_subsys(struct cgroup_subsys *ss) · line 2525

Quoted 5 lines

kernel/cgroup.c +1 -0

@@ -2522,6 +2522,7 @@ static void __init cgroup_init_subsys(struct cgroup_subsys *ss)
       printk(KERN_INFO "Initializing cgroup subsys %s\n", ss->name);
 
       /* Create the top cgroup state for this subsystem */
+       list_add(&ss->sibling, &rootnode.root_list);

This is wrong. cgroup_subsys.sibling is a member of the list of subsystems attached to a given root, and rootnode.root_list is a member of the list of hierarchies. Did you actually meant &rootnode.subsys_list?

Quoted 3 lines

I thought I corrected all the s/root_list/subsys_list/ in patch v2. :(

If this patch makes sense, I'll fix it, otherwise we'll going to ask Andrew to drop it from -mm.

Looking at the existing code, it's not actually broken - but only by accident.

We aim to add a link for each cgroup, which we do by finding the first subsystem in each hierarchy and adding a link for its cgroup, and then adding a link for dummytop if there weren't any subsystems in the inactive hierarchy.

But since rootnode.subsys_list is always empty, we always skip the inactive hierarchy in the main loop (if it has any subsystems), and always add the inactive hierarchy afterwards. So everything works out OK (all hierarchies do get linked), but it's horribly unreadable.

Having the subsys_list be correct for the inactive hierarchy is probably a good thing, so this patch should go ahead (once the typos is fixed).