Due to the overhead of the memory controller. The
memory controller is now disabled by default. This patch changes
cgroup_disable to cgroup_toggle, so that each controller can decide
whether it wants to be enabled/disabled by default.

If everyone agrees on this approach and likes it, should we push this
into 2.6.25?

Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
---

 Documentation/kernel-parameters.txt |    5 +++--
 kernel/cgroup.c                     |   11 ++++++-----
 mm/memcontrol.c                     |    1 +
 3 files changed, 10 insertions(+), 7 deletions(-)

diff -puN kernel/cgroup.c~memory-controller-default-option-off kernel/cgroup.c
--- linux-2.6.25-rc8/kernel/cgroup.c~memory-controller-default-option-off	2008-04-07 16:24:28.000000000 +0530
+++ linux-2.6.25-rc8-balbir/kernel/cgroup.c	2008-04-07 16:47:48.000000000 +0530
@@ -3063,7 +3063,7 @@ static void cgroup_release_agent(struct 
 	mutex_unlock(&cgroup_mutex);
 }
 
-static int __init cgroup_disable(char *str)
+static int __init cgroup_toggle(char *str)
 {
 	int i;
 	char *token;
@@ -3076,13 +3076,14 @@ static int __init cgroup_disable(char *s
 			struct cgroup_subsys *ss = subsys[i];
 
 			if (!strcmp(token, ss->name)) {
-				ss->disabled = 1;
-				printk(KERN_INFO "Disabling %s control group"
-					" subsystem\n", ss->name);
+				ss->disabled = !ss->disabled;
+				if (ss->disabled)
+					printk(KERN_INFO "%s control group "
+						"is disabled", ss->name);
 				break;
 			}
 		}
 	}
 	return 1;
 }
-__setup("cgroup_disable=", cgroup_disable);
+__setup("cgroup_toggle=", cgroup_toggle);
diff -puN mm/memcontrol.c~memory-controller-default-option-off mm/memcontrol.c
--- linux-2.6.25-rc8/mm/memcontrol.c~memory-controller-default-option-off	2008-04-07 16:24:28.000000000 +0530
+++ linux-2.6.25-rc8-balbir/mm/memcontrol.c	2008-04-07 16:40:22.000000000 +0530
@@ -1104,4 +1104,5 @@ struct cgroup_subsys mem_cgroup_subsys =
 	.populate = mem_cgroup_populate,
 	.attach = mem_cgroup_move_task,
 ...
[ message continues ]

First I like the change to make it disabled by default.

I don't think "toggle" is good semantics for a user visible switch
because that changes the meaning when the kernel default changes
(which it will likely once the current default overhead is fixed)

It should be rather: cgroup=on/off 

-Andi
--

[ message continues ]

The boot control options apply to all controllers and we want to allow
controllers to decide whether they should be turned on or off. With sufficient
documentation support in Documentation/kernel-parameters.txt, don't you think we
can expect this to work as the user intended?

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL
--

[ message continues ]

Ok that's fine too (to have finer grained options), just those should

Even with documentation support semantics changes over releases are not nice.
So "toggle" is bad, always have it absolute values.

So if an user decides they want full cgroup support and stick in a option
for .25 into their boot loader config they should always get full cgroup support in 
all future kernels.  Similiar if someone decides they don't need it. 

-Andi
--

[ message continues ]

2 parameter is wrong?

       cgroup_disable= [KNL] Disable a particular controller
                       Format: {name of the controller(s) to disable}
       cgroup_enable= [KNL] Enable a particular controller
                       Format: {name of the controller(s) to enable}

e.g.
user specified cgroup_enable=mem.
if default value is disable, it mean turn to enable.
if default value is enable,  it is meaningless param.
--

[ message continues ]

No, it is not all bad. That can be done, but we need to guard against a usage like

cgroup_disable=memory cgroup_enable=memory

The user will probably get what he/she deserves for it.

-- 
	Warm Regards,
	Balbir Singh
	Linux Technology Center
	IBM, ISTL
--

[ message continues ]

I don't think we need to guard against that. It seems perfectly valid
to have a lilo config with

  append="cgroup_disable=memory"

and then want to boot with the memory controller enabled you can do

  lilo -R <image> cgroup_enable=memory

The kernel command line will then look like

  "... cgroup_disable=memory cgroup_enable=memory"

and the last switch should win.

Paul
--

[ message continues ]

Hmm..

toggle parameter seems no good idea.
because if change default value in the future, boot parmeter becomes
an opposite meaning.

thus, we can't change default value even if we will be able to get
enough performance improvement in the future.

Thanks
--

[ message continues ]