[-mm] Disable the memory controller by default

[-mm] Disable the memory controller by default

[Balbir Singh]

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,
 	.early_init = 0,
+	.disabled = 1,
 };
diff -puN Documentation/kernel-parameters.txt~memory-controller-default-option-off Documentation/kernel-parameters.txt
--- linux-2.6.25-rc8/Documentation/kernel-parameters.txt~memory-controller-default-option-off	2008-04-07 16:38:25.000000000 +0530
+++ linux-2.6.25-rc8-balbir/Documentation/kernel-parameters.txt	2008-04-07 17:20:08.000000000 +0530
@@ -381,9 +381,10 @@ and is between 256 and 4096 characters. 
 	ccw_timeout_log [S390]
 			See Documentation/s390/CommonIO for details.
 
-	cgroup_disable= [KNL] Disable a particular controller
-			Format: {name of the controller(s) to disable}
+	cgroup_toggle= [KNL] Toggle (enable/disable) a particular controller
+			Format: {name of the controller(s) to enable/disable}
 				{Currently supported controllers - "memory"}
+				{The memory controller is disabled by default}
 
 	checkreqprot	[SELINUX] Set initial checkreqprot flag value.
 			Format: { "0" | "1" }
_

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

Re: [-mm] Disable the memory controller by default

[Balbir Singh]

Andi Kleen wrote:
> On Mon, Apr 07, 2008 at 05:21:37PM +0530, Balbir Singh wrote:
>>
>> 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?
> 
> 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 
> 

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

Re: [-mm] Disable the memory controller by default

[Andi Kleen]

On Mon, Apr 07, 2008 at 05:21:37PM +0530, Balbir Singh wrote:
> 
> 
> 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?

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

Re: [-mm] Disable the memory controller by default

[KOSAKI Motohiro]

Hi

>  diff -puN Documentation/kernel-parameters.txt~memory-controller-default-option-off Documentation/kernel-parameters.txt
>  --- linux-2.6.25-rc8/Documentation/kernel-parameters.txt~memory-controller-default-option-off   2008-04-07 16:38:25.000000000 +0530
>  +++ linux-2.6.25-rc8-balbir/Documentation/kernel-parameters.txt 2008-04-07 17:20:08.000000000 +0530
>  @@ -381,9 +381,10 @@ and is between 256 and 4096 characters.
>         ccw_timeout_log [S390]
>                         See Documentation/s390/CommonIO for details.
>
>  -       cgroup_disable= [KNL] Disable a particular controller
>  -                       Format: {name of the controller(s) to disable}
>  +       cgroup_toggle= [KNL] Toggle (enable/disable) a particular controller
>  +                       Format: {name of the controller(s) to enable/disable}
>                                 {Currently supported controllers - "memory"}
>  +                               {The memory controller is disabled by default}
>
>         checkreqprot    [SELINUX] Set initial checkreqprot flag value.
>                         Format: { "0" | "1" }

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

Re: [-mm] Disable the memory controller by default

[Andi Kleen]

> The boot control options apply to all controllers and we want to allow
> controllers to decide whether they should be turned on or off.

Ok that's fine too (to have finer grained options), just those should
be on/off too, not toggle.

> documentation support in Documentation/kernel-parameters.txt, don't you think we
> can expect this to work as the user intended?

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

Re: [-mm] Disable the memory controller by default

[KOSAKI Motohiro]

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

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.

Re: [-mm] Disable the memory controller by default

[Balbir Singh]

KOSAKI Motohiro wrote:
>>  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?
> 
> 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}
> 

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

Re: [-mm] Disable the memory controller by default

[Paul Menage]

On Mon, Apr 7, 2008 at 5:16 AM, Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>  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.

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