[PATCH 2/3] cgroups: add inactive subsystems to rootnode.root_list

[PATCH 2/3] cgroups: add inactive subsystems to rootnode.root_list

[Li Zefan]

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

This conflicts with the code in find_css_set():

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
		...
		if (ss->root->subsys_list.next == &ss->sibling) {
			...
		}
	}
	if (list_empty(&rootnode.subsys_list)) {
		...
	}

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

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---
 kernel/cgroup.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 33ba756..4e50e97 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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);
 
 	/* Create the top cgroup state for this subsystem */
+	list_add(&ss->sibling, &rootnode.root_list);
 	ss->root = &rootnode;
 	css = ss->create(ss, dummytop);
 	/* We don't handle early failures gracefully */
-- 
1.5.4.rc3

[PATCH 2/3 -v2] cgroups: add inactive subsystems to rootnode.subsys_list

[Li Zefan]

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

This conflicts with the code in find_css_set():

	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
		...
		if (ss->root->subsys_list.next == &ss->sibling) {
			...
		}
	}
	if (list_empty(&rootnode.subsys_list)) {
		...
	}

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

Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
---

fix typo in title and changelog: s/root_list/subsys_list

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

diff --git a/kernel/cgroup.c b/kernel/cgroup.c
index 33ba756..4e50e97 100644
--- a/kernel/cgroup.c
+++ b/kernel/cgroup.c
@@ -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);
 
 	/* Create the top cgroup state for this subsystem */
+	list_add(&ss->sibling, &rootnode.root_list);
 	ss->root = &rootnode;
 	css = ss->create(ss, dummytop);
 	/* We don't handle early failures gracefully */
-- 
1.5.4.rc3
	

Re: [PATCH 2/3] cgroups: add inactive subsystems to rootnode.root_list

[Paul Menage]

On Fri, Nov 28, 2008 at 2:02 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> 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".

>
> The above code assumes rootnode.subsys_list links all inactive
> hierarchies.
>
> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
> ---
>  kernel/cgroup.c |    5 +++--
>  1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
> index 33ba756..4e50e97 100644
> --- a/kernel/cgroup.c
> +++ b/kernel/cgroup.c
> @@ -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);
>
>        /* 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?

Paul

Re: [PATCH 2/3] cgroups: add inactive subsystems to rootnode.root_list

[Li Zefan]

Paul Menage wrote:
> On Fri, Nov 28, 2008 at 2:02 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>> 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.

>> The above code assumes rootnode.subsys_list links all inactive
>> hierarchies.
>>
>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>> ---
>>  kernel/cgroup.c |    5 +++--
>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>> index 33ba756..4e50e97 100644
>> --- a/kernel/cgroup.c
>> +++ b/kernel/cgroup.c
>> @@ -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);
>>
>>        /* 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.

Re: [PATCH 2/3] cgroups: add inactive subsystems to rootnode.root_list

[Paul Menage]

On Mon, Dec 1, 2008 at 7:24 PM, Li Zefan <lizf@cn.fujitsu.com> wrote:
> Paul Menage wrote:
>> On Fri, Nov 28, 2008 at 2:02 AM, Li Zefan <lizf@cn.fujitsu.com> wrote:
>>> 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.
>
>>> The above code assumes rootnode.subsys_list links all inactive
>>> hierarchies.
>>>
>>> Signed-off-by: Li Zefan <lizf@cn.fujitsu.com>
>>> ---
>>>  kernel/cgroup.c |    5 +++--
>>>  1 files changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/kernel/cgroup.c b/kernel/cgroup.c
>>> index 33ba756..4e50e97 100644
>>> --- a/kernel/cgroup.c
>>> +++ b/kernel/cgroup.c
>>> @@ -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);
>>>
>>>        /* 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.

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).

Paul

Re: [PATCH 2/3] cgroups: add inactive subsystems to rootnode.root_list

[Li Zefan]

>>>> @@ -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?
>>>
>> 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.
> 

Yes, since it's not broken, I didn't claim this patch as a bug fix that
should go into 2.6.28.

> 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.
> 

I found this confusion when I was trying to understand this part of code.

> 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).
> 

I'll fix it. :)