[PATCH] Add cgroup support for enabling controllers at boot time

[PATCH] Add cgroup support for enabling controllers at boot time

[Balbir Singh]

From: Paul Menage <menage@google.com>

The effects of cgroup_disable=foo are:

- foo doesn't show up in /proc/cgroups
- foo isn't auto-mounted if you mount all cgroups in a single hierarchy
- foo isn't visible as an individually mountable subsystem

As a result there will only ever be one call to foo->create(), at init
time; all processes will stay in this group, and the group will never
be mounted on a visible hierarchy. Any additional effects (e.g. not
allocating metadata) are up to the foo subsystem.

This doesn't handle early_init subsystems (their "disabled" bit isn't
set be, but it could easily be extended to do so if any of the early_init
systems wanted it - I think it would just involve some nastier parameter
processing since it would occur before the command-line argument parser
had been run.

[Balbir added Documentation/kernel-parameters updates]

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

 Documentation/kernel-parameters.txt |    4 ++++
 include/linux/cgroup.h              |    1 +
 kernel/cgroup.c                     |   27 +++++++++++++++++++++++++--
 3 files changed, 30 insertions(+), 2 deletions(-)

diff -puN include/linux/cgroup.h~cgroup_disable include/linux/cgroup.h
--- linux-2.6.25-rc4/include/linux/cgroup.h~cgroup_disable	2008-03-06 12:19:38.000000000 +0530
+++ linux-2.6.25-rc4-balbir/include/linux/cgroup.h	2008-03-06 12:19:38.000000000 +0530
@@ -256,6 +256,7 @@ struct cgroup_subsys {
 	void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
 	int subsys_id;
 	int active;
+	int disabled;
 	int early_init;
 #define MAX_CGROUP_TYPE_NAMELEN 32
 	const char *name;
diff -puN kernel/cgroup.c~cgroup_disable kernel/cgroup.c
--- linux-2.6.25-rc4/kernel/cgroup.c~cgroup_disable	2008-03-06 12:19:38.000000000 +0530
+++ linux-2.6.25-rc4-balbir/kernel/cgroup.c	2008-03-06 12:19:38.000000000 +0530
@@ -782,7 +782,14 @@ static int parse_cgroupfs_options(char *
 		if (!*token)
 			return -EINVAL;
 		if (!strcmp(token, "all")) {
-			opts->subsys_bits = (1 << CGROUP_SUBSYS_COUNT) - 1;
+			/* Add all non-disabled subsystems */
+			int i;
+			opts->subsys_bits = 0;
+			for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
+				struct cgroup_subsys *ss = subsys[i];
+				if (!ss->disabled)
+					opts->subsys_bits |= 1ul << i;
+			}
 		} else if (!strcmp(token, "noprefix")) {
 			set_bit(ROOT_NOPREFIX, &opts->flags);
 		} else if (!strncmp(token, "release_agent=", 14)) {
@@ -800,7 +807,8 @@ static int parse_cgroupfs_options(char *
 			for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 				ss = subsys[i];
 				if (!strcmp(token, ss->name)) {
-					set_bit(i, &opts->subsys_bits);
+					if (!ss->disabled)
+						set_bit(i, &opts->subsys_bits);
 					break;
 				}
 			}
@@ -2604,6 +2612,8 @@ static int proc_cgroupstats_show(struct 
 	mutex_lock(&cgroup_mutex);
 	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
 		struct cgroup_subsys *ss = subsys[i];
+		if (ss->disabled)
+			continue;
 		seq_printf(m, "%s\t%lu\t%d\n",
 			   ss->name, ss->root->subsys_bits,
 			   ss->root->number_of_cgroups);
@@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct 
 	spin_unlock(&release_list_lock);
 	mutex_unlock(&cgroup_mutex);
 }
+
+static int __init cgroup_disable(char *str)
+{
+	int i;
+	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
+		struct cgroup_subsys *ss = subsys[i];
+		if (!strcmp(str, ss->name)) {
+			ss->disabled = 1;
+			break;
+		}
+	}
+}
+__setup("cgroup_disable=", cgroup_disable);
diff -puN Documentation/kernel-parameters.txt~cgroup_disable Documentation/kernel-parameters.txt
--- linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable	2008-03-06 17:57:32.000000000 +0530
+++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt	2008-03-06 18:00:32.000000000 +0530
@@ -383,6 +383,10 @@ and is between 256 and 4096 characters. 
 	ccw_timeout_log [S390]
 			See Documentation/s390/CommonIO for details.
 
+	cgroup_disable= [KNL] Enable disable a particular controller
+			Format: {name of the controller}
+			See /proc/cgroups for a list of compiled controllers
+
 	checkreqprot	[SELINUX] Set initial checkreqprot flag value.
 			Format: { "0" | "1" }
 			See security/selinux/Kconfig help text.
_

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

[PATCH] Make memory resource control aware of boot options

[Balbir Singh]

A boot option for the memory controller was discussed on lkml. It is a good
idea to add it, since it saves memory for people who want to turn off the
memory controller.

By default the option is on for the following two reasons

1. It provides compatibility with the current scheme where the memory
   controller turns on if the config option is enabled
2. It allows for wider testing of the memory controller, once the config
   option is enabled

We still allow the create, destroy callbacks to succeed, since they are
not aware of boot options. We do not populate the directory will
memory resource controller specific files.

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

 mm/memcontrol.c |   17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff -puN mm/memcontrol.c~memory-controller-add-boot-option mm/memcontrol.c
--- linux-2.6.25-rc4/mm/memcontrol.c~memory-controller-add-boot-option	2008-03-07 00:14:10.000000000 +0530
+++ linux-2.6.25-rc4-balbir/mm/memcontrol.c	2008-03-07 00:14:10.000000000 +0530
@@ -533,6 +533,9 @@ static int mem_cgroup_charge_common(stru
 	unsigned long nr_retries = MEM_CGROUP_RECLAIM_RETRIES;
 	struct mem_cgroup_per_zone *mz;
 
+	if (mem_cgroup_subsys.disabled)
+		return 0;
+
 	/*
 	 * Should page_cgroup's go to their own slab?
 	 * One could optimize the performance of the charging routine
@@ -665,6 +668,9 @@ void mem_cgroup_uncharge_page(struct pag
 	struct mem_cgroup_per_zone *mz;
 	unsigned long flags;
 
+	if (mem_cgroup_subsys.disabled)
+		return;
+
 	/*
 	 * Check if our page_cgroup is valid
 	 */
@@ -705,6 +711,9 @@ int mem_cgroup_prepare_migration(struct 
 {
 	struct page_cgroup *pc;
 
+	if (mem_cgroup_subsys.disabled)
+		return 0;
+
 	lock_page_cgroup(page);
 	pc = page_get_page_cgroup(page);
 	if (pc)
@@ -803,6 +812,9 @@ static int mem_cgroup_force_empty(struct
 	int ret = -EBUSY;
 	int node, zid;
 
+	if (mem_cgroup_subsys.disabled)
+		return 0;
+
 	css_get(&mem->css);
 	/*
 	 * page reclaim code (kswapd etc..) will move pages between
@@ -1053,6 +1065,8 @@ static void mem_cgroup_destroy(struct cg
 static int mem_cgroup_populate(struct cgroup_subsys *ss,
 				struct cgroup *cont)
 {
+	if (mem_cgroup_subsys.disabled)
+		return 0;
 	return cgroup_add_files(cont, ss, mem_cgroup_files,
 					ARRAY_SIZE(mem_cgroup_files));
 }
@@ -1065,6 +1079,9 @@ static void mem_cgroup_move_task(struct 
 	struct mm_struct *mm;
 	struct mem_cgroup *mem, *old_mem;
 
+	if (mem_cgroup_subsys.disabled)
+		return;
+
 	mm = get_task_mm(p);
 	if (mm == NULL)
 		return;
_

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

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[David Rientjes]

On Fri, 7 Mar 2008, Balbir Singh wrote:

> @@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct 
>  	spin_unlock(&release_list_lock);
>  	mutex_unlock(&cgroup_mutex);
>  }
> +
> +static int __init cgroup_disable(char *str)
> +{
> +	int i;
> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> +		struct cgroup_subsys *ss = subsys[i];
> +		if (!strcmp(str, ss->name)) {
> +			ss->disabled = 1;
> +			break;
> +		}
> +	}
> +}
> +__setup("cgroup_disable=", cgroup_disable);

This doesn't handle spaces very well, so isn't it possible for the name of 
a current or future cgroup subsystem to be specified after cgroup_disable= 
on the command line and have it disabled by accident?

> diff -puN Documentation/kernel-parameters.txt~cgroup_disable Documentation/kernel-parameters.txt
> --- linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable	2008-03-06 17:57:32.000000000 +0530
> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt	2008-03-06 18:00:32.000000000 +0530
> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters. 
>  	ccw_timeout_log [S390]
>  			See Documentation/s390/CommonIO for details.
>  
> +	cgroup_disable= [KNL] Enable disable a particular controller
> +			Format: {name of the controller}
> +			See /proc/cgroups for a list of compiled controllers
> +

This works on multiple controllers, though, if they follow 
cgroup_disable=, so the documentation and format should reflect that.

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Randy Dunlap]

On Fri, 07 Mar 2008 00:29:52 +0530 Balbir Singh wrote:

> From: Paul Menage <menage@google.com>
> 
> The effects of cgroup_disable=foo are:
> 
> - foo doesn't show up in /proc/cgroups
> - foo isn't auto-mounted if you mount all cgroups in a single hierarchy
> - foo isn't visible as an individually mountable subsystem
> 
> As a result there will only ever be one call to foo->create(), at init
> time; all processes will stay in this group, and the group will never
> be mounted on a visible hierarchy. Any additional effects (e.g. not
> allocating metadata) are up to the foo subsystem.
> 
> This doesn't handle early_init subsystems (their "disabled" bit isn't
> set be, but it could easily be extended to do so if any of the early_init
> systems wanted it - I think it would just involve some nastier parameter
> processing since it would occur before the command-line argument parser
> had been run.
> 
> [Balbir added Documentation/kernel-parameters updates]
> 
> Signed-off-by: Paul Menage <menage@google.com>
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
> 
>  Documentation/kernel-parameters.txt |    4 ++++
>  include/linux/cgroup.h              |    1 +
>  kernel/cgroup.c                     |   27 +++++++++++++++++++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff -puN Documentation/kernel-parameters.txt~cgroup_disable Documentation/kernel-parameters.txt
> --- linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable	2008-03-06 17:57:32.000000000 +0530
> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt	2008-03-06 18:00:32.000000000 +0530
> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters. 
>  	ccw_timeout_log [S390]
>  			See Documentation/s390/CommonIO for details.
>  
> +	cgroup_disable= [KNL] Enable disable a particular controller

So it can enable or disable?  or the text has extra text?

> +			Format: {name of the controller}
> +			See /proc/cgroups for a list of compiled controllers
> +
>  	checkreqprot	[SELINUX] Set initial checkreqprot flag value.
>  			Format: { "0" | "1" }
>  			See security/selinux/Kconfig help text.


---
~Randy

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Li Zefan]

Randy Dunlap wrote:
> On Fri, 07 Mar 2008 00:29:52 +0530 Balbir Singh wrote:
> 
>> From: Paul Menage <menage@google.com>
>>
>> The effects of cgroup_disable=foo are:
>>
>> - foo doesn't show up in /proc/cgroups
>> - foo isn't auto-mounted if you mount all cgroups in a single hierarchy
>> - foo isn't visible as an individually mountable subsystem
>>
>> As a result there will only ever be one call to foo->create(), at init
>> time; all processes will stay in this group, and the group will never
>> be mounted on a visible hierarchy. Any additional effects (e.g. not
>> allocating metadata) are up to the foo subsystem.
>>
>> This doesn't handle early_init subsystems (their "disabled" bit isn't
>> set be, but it could easily be extended to do so if any of the early_init
>> systems wanted it - I think it would just involve some nastier parameter
>> processing since it would occur before the command-line argument parser
>> had been run.
>>
>> [Balbir added Documentation/kernel-parameters updates]
>>
>> Signed-off-by: Paul Menage <menage@google.com>
>> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
>> ---
>>
>>  Documentation/kernel-parameters.txt |    4 ++++
>>  include/linux/cgroup.h              |    1 +
>>  kernel/cgroup.c                     |   27 +++++++++++++++++++++++++--
>>  3 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff -puN Documentation/kernel-parameters.txt~cgroup_disable Documentation/kernel-parameters.txt
>> --- linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable	2008-03-06 17:57:32.000000000 +0530
>> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt	2008-03-06 18:00:32.000000000 +0530
>> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters. 
>>  	ccw_timeout_log [S390]
>>  			See Documentation/s390/CommonIO for details.
>>  
>> +	cgroup_disable= [KNL] Enable disable a particular controller
> 
> So it can enable or disable?  or the text has extra text?
>

don't think so, should be "Disable a particular controller"
 
>> +			Format: {name of the controller}
>> +			See /proc/cgroups for a list of compiled controllers
>> +
>>  	checkreqprot	[SELINUX] Set initial checkreqprot flag value.
>>  			Format: { "0" | "1" }
>>  			See security/selinux/Kconfig help text.
> 
> 
> ---
> ~Randy
> 
> 

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Li Zefan]

Balbir Singh wrote:
> From: Paul Menage <menage@google.com>
> 
> The effects of cgroup_disable=foo are:
> 
> - foo doesn't show up in /proc/cgroups
> - foo isn't auto-mounted if you mount all cgroups in a single hierarchy
> - foo isn't visible as an individually mountable subsystem
> 
> As a result there will only ever be one call to foo->create(), at init
> time; all processes will stay in this group, and the group will never
> be mounted on a visible hierarchy. Any additional effects (e.g. not
> allocating metadata) are up to the foo subsystem.
> 
> This doesn't handle early_init subsystems (their "disabled" bit isn't
> set be, but it could easily be extended to do so if any of the early_init
> systems wanted it - I think it would just involve some nastier parameter
> processing since it would occur before the command-line argument parser
> had been run.
> 
> [Balbir added Documentation/kernel-parameters updates]
> 
> Signed-off-by: Paul Menage <menage@google.com>
> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
> ---
> 
>  Documentation/kernel-parameters.txt |    4 ++++
>  include/linux/cgroup.h              |    1 +
>  kernel/cgroup.c                     |   27 +++++++++++++++++++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff -puN include/linux/cgroup.h~cgroup_disable include/linux/cgroup.h
> --- linux-2.6.25-rc4/include/linux/cgroup.h~cgroup_disable	2008-03-06 12:19:38.000000000 +0530
> +++ linux-2.6.25-rc4-balbir/include/linux/cgroup.h	2008-03-06 12:19:38.000000000 +0530
> @@ -256,6 +256,7 @@ struct cgroup_subsys {
>  	void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
>  	int subsys_id;
>  	int active;
> +	int disabled;
>  	int early_init;
>  #define MAX_CGROUP_TYPE_NAMELEN 32
>  	const char *name;
> diff -puN kernel/cgroup.c~cgroup_disable kernel/cgroup.c
> --- linux-2.6.25-rc4/kernel/cgroup.c~cgroup_disable	2008-03-06 12:19:38.000000000 +0530
> +++ linux-2.6.25-rc4-balbir/kernel/cgroup.c	2008-03-06 12:19:38.000000000 +0530
> @@ -782,7 +782,14 @@ static int parse_cgroupfs_options(char *
>  		if (!*token)
>  			return -EINVAL;
>  		if (!strcmp(token, "all")) {
> -			opts->subsys_bits = (1 << CGROUP_SUBSYS_COUNT) - 1;
> +			/* Add all non-disabled subsystems */
> +			int i;
> +			opts->subsys_bits = 0;
> +			for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> +				struct cgroup_subsys *ss = subsys[i];
> +				if (!ss->disabled)
> +					opts->subsys_bits |= 1ul << i;
> +			}
>  		} else if (!strcmp(token, "noprefix")) {
>  			set_bit(ROOT_NOPREFIX, &opts->flags);
>  		} else if (!strncmp(token, "release_agent=", 14)) {
> @@ -800,7 +807,8 @@ static int parse_cgroupfs_options(char *
>  			for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>  				ss = subsys[i];
>  				if (!strcmp(token, ss->name)) {
> -					set_bit(i, &opts->subsys_bits);
> +					if (!ss->disabled)
> +						set_bit(i, &opts->subsys_bits);
>  					break;
>  				}
>  			}
> @@ -2604,6 +2612,8 @@ static int proc_cgroupstats_show(struct 
>  	mutex_lock(&cgroup_mutex);
>  	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>  		struct cgroup_subsys *ss = subsys[i];
> +		if (ss->disabled)
> +			continue;
>  		seq_printf(m, "%s\t%lu\t%d\n",
>  			   ss->name, ss->root->subsys_bits,
>  			   ss->root->number_of_cgroups);
> @@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct 
>  	spin_unlock(&release_list_lock);
>  	mutex_unlock(&cgroup_mutex);
>  }
> +
> +static int __init cgroup_disable(char *str)
> +{
> +	int i;
> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> +		struct cgroup_subsys *ss = subsys[i];
> +		if (!strcmp(str, ss->name)) {
> +			ss->disabled = 1;
> +			break;
> +		}
> +	}
> +}
> +__setup("cgroup_disable=", cgroup_disable);
> diff -puN Documentation/kernel-parameters.txt~cgroup_disable Documentation/kernel-parameters.txt
> --- linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable	2008-03-06 17:57:32.000000000 +0530
> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt	2008-03-06 18:00:32.000000000 +0530
> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters. 
>  	ccw_timeout_log [S390]
>  			See Documentation/s390/CommonIO for details.
>  
> +	cgroup_disable= [KNL] Enable disable a particular controller
> +			Format: {name of the controller}
> +			See /proc/cgroups for a list of compiled controllers
> +

The changelog of this patch:
- foo doesn't show up in /proc/cgroups

So a disabled subsystem won't show up in /proc/cgroups. In a previous
mail, I asked whether it will be useful to print out the disable bit
in /proc/cgroups, so we can distinguish a subsystem from disaled and
not-compiled.

>  	checkreqprot	[SELINUX] Set initial checkreqprot flag value.
>  			Format: { "0" | "1" }
>  			See security/selinux/Kconfig help text.
> _
> 

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Balbir Singh]

Li Zefan wrote:
> Balbir Singh wrote:
>> From: Paul Menage <menage@google.com>
>>
>> The effects of cgroup_disable=foo are:
>>
>> - foo doesn't show up in /proc/cgroups
>> - foo isn't auto-mounted if you mount all cgroups in a single hierarchy
>> - foo isn't visible as an individually mountable subsystem
>>
>> As a result there will only ever be one call to foo->create(), at init
>> time; all processes will stay in this group, and the group will never
>> be mounted on a visible hierarchy. Any additional effects (e.g. not
>> allocating metadata) are up to the foo subsystem.
>>
>> This doesn't handle early_init subsystems (their "disabled" bit isn't
>> set be, but it could easily be extended to do so if any of the early_init
>> systems wanted it - I think it would just involve some nastier parameter
>> processing since it would occur before the command-line argument parser
>> had been run.
>>
>> [Balbir added Documentation/kernel-parameters updates]
>>
>> Signed-off-by: Paul Menage <menage@google.com>
>> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
>> ---
>>
>>  Documentation/kernel-parameters.txt |    4 ++++
>>  include/linux/cgroup.h              |    1 +
>>  kernel/cgroup.c                     |   27 +++++++++++++++++++++++++--
>>  3 files changed, 30 insertions(+), 2 deletions(-)
>>
>> diff -puN include/linux/cgroup.h~cgroup_disable include/linux/cgroup.h
>> --- linux-2.6.25-rc4/include/linux/cgroup.h~cgroup_disable   
>> 2008-03-06 12:19:38.000000000 +0530
>> +++ linux-2.6.25-rc4-balbir/include/linux/cgroup.h    2008-03-06
>> 12:19:38.000000000 +0530
>> @@ -256,6 +256,7 @@ struct cgroup_subsys {
>>      void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
>>      int subsys_id;
>>      int active;
>> +    int disabled;
>>      int early_init;
>>  #define MAX_CGROUP_TYPE_NAMELEN 32
>>      const char *name;
>> diff -puN kernel/cgroup.c~cgroup_disable kernel/cgroup.c
>> --- linux-2.6.25-rc4/kernel/cgroup.c~cgroup_disable    2008-03-06
>> 12:19:38.000000000 +0530
>> +++ linux-2.6.25-rc4-balbir/kernel/cgroup.c    2008-03-06
>> 12:19:38.000000000 +0530
>> @@ -782,7 +782,14 @@ static int parse_cgroupfs_options(char *
>>          if (!*token)
>>              return -EINVAL;
>>          if (!strcmp(token, "all")) {
>> -            opts->subsys_bits = (1 << CGROUP_SUBSYS_COUNT) - 1;
>> +            /* Add all non-disabled subsystems */
>> +            int i;
>> +            opts->subsys_bits = 0;
>> +            for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>> +                struct cgroup_subsys *ss = subsys[i];
>> +                if (!ss->disabled)
>> +                    opts->subsys_bits |= 1ul << i;
>> +            }
>>          } else if (!strcmp(token, "noprefix")) {
>>              set_bit(ROOT_NOPREFIX, &opts->flags);
>>          } else if (!strncmp(token, "release_agent=", 14)) {
>> @@ -800,7 +807,8 @@ static int parse_cgroupfs_options(char *
>>              for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>                  ss = subsys[i];
>>                  if (!strcmp(token, ss->name)) {
>> -                    set_bit(i, &opts->subsys_bits);
>> +                    if (!ss->disabled)
>> +                        set_bit(i, &opts->subsys_bits);
>>                      break;
>>                  }
>>              }
>> @@ -2604,6 +2612,8 @@ static int proc_cgroupstats_show(struct     
>> mutex_lock(&cgroup_mutex);
>>      for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>          struct cgroup_subsys *ss = subsys[i];
>> +        if (ss->disabled)
>> +            continue;
>>          seq_printf(m, "%s\t%lu\t%d\n",
>>                 ss->name, ss->root->subsys_bits,
>>                 ss->root->number_of_cgroups);
>> @@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct     
>> spin_unlock(&release_list_lock);
>>      mutex_unlock(&cgroup_mutex);
>>  }
>> +
>> +static int __init cgroup_disable(char *str)
>> +{
>> +    int i;
>> +    for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>> +        struct cgroup_subsys *ss = subsys[i];
>> +        if (!strcmp(str, ss->name)) {
>> +            ss->disabled = 1;
>> +            break;
>> +        }
>> +    }
>> +}
>> +__setup("cgroup_disable=", cgroup_disable);
>> diff -puN Documentation/kernel-parameters.txt~cgroup_disable
>> Documentation/kernel-parameters.txt
>> ---
>> linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable   
>> 2008-03-06 17:57:32.000000000 +0530
>> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt   
>> 2008-03-06 18:00:32.000000000 +0530
>> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters.     
>> ccw_timeout_log [S390]
>>              See Documentation/s390/CommonIO for details.
>>  
>> +    cgroup_disable= [KNL] Enable disable a particular controller
>> +            Format: {name of the controller}
>> +            See /proc/cgroups for a list of compiled controllers
>> +
> 
> The changelog of this patch:
> - foo doesn't show up in /proc/cgroups
> 
> So a disabled subsystem won't show up in /proc/cgroups. In a previous
> mail, I asked whether it will be useful to print out the disable bit
> in /proc/cgroups, so we can distinguish a subsystem from disaled and
> not-compiled.

Hi, Li,

That is a good idea, but can that come in later? We need to get the boot option
in, so that users can decide at boot time whether they want the page_container
overhead. I'll send out another set of patches to add that feature or work
with Paul to see what he thinks about it.

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

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Balbir Singh]

Li Zefan wrote:
> Randy Dunlap wrote:
>> On Fri, 07 Mar 2008 00:29:52 +0530 Balbir Singh wrote:
>>
>>> From: Paul Menage <menage@google.com>
>>>
>>> The effects of cgroup_disable=foo are:
>>>
>>> - foo doesn't show up in /proc/cgroups
>>> - foo isn't auto-mounted if you mount all cgroups in a single hierarchy
>>> - foo isn't visible as an individually mountable subsystem
>>>
>>> As a result there will only ever be one call to foo->create(), at init
>>> time; all processes will stay in this group, and the group will never
>>> be mounted on a visible hierarchy. Any additional effects (e.g. not
>>> allocating metadata) are up to the foo subsystem.
>>>
>>> This doesn't handle early_init subsystems (their "disabled" bit isn't
>>> set be, but it could easily be extended to do so if any of the
>>> early_init
>>> systems wanted it - I think it would just involve some nastier parameter
>>> processing since it would occur before the command-line argument parser
>>> had been run.
>>>
>>> [Balbir added Documentation/kernel-parameters updates]
>>>
>>> Signed-off-by: Paul Menage <menage@google.com>
>>> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
>>> ---
>>>
>>>  Documentation/kernel-parameters.txt |    4 ++++
>>>  include/linux/cgroup.h              |    1 +
>>>  kernel/cgroup.c                     |   27 +++++++++++++++++++++++++--
>>>  3 files changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff -puN Documentation/kernel-parameters.txt~cgroup_disable
>>> Documentation/kernel-parameters.txt
>>> ---
>>> linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable   
>>> 2008-03-06 17:57:32.000000000 +0530
>>> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt   
>>> 2008-03-06 18:00:32.000000000 +0530
>>> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters.     
>>> ccw_timeout_log [S390]
>>>              See Documentation/s390/CommonIO for details.
>>>  
>>> +    cgroup_disable= [KNL] Enable disable a particular controller
>>
>> So it can enable or disable?  or the text has extra text?
>>
> 
> don't think so, should be "Disable a particular controller"
> 

Agreed and fixed.

Thanks,


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

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Balbir Singh]

David Rientjes wrote:
> On Fri, 7 Mar 2008, Balbir Singh wrote:
> 
>> @@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct 
>>  	spin_unlock(&release_list_lock);
>>  	mutex_unlock(&cgroup_mutex);
>>  }
>> +
>> +static int __init cgroup_disable(char *str)
>> +{
>> +	int i;
>> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>> +		struct cgroup_subsys *ss = subsys[i];
>> +		if (!strcmp(str, ss->name)) {
>> +			ss->disabled = 1;
>> +			break;
>> +		}
>> +	}
>> +}
>> +__setup("cgroup_disable=", cgroup_disable);
> 
> This doesn't handle spaces very well, so isn't it possible for the name of 
> a current or future cgroup subsystem to be specified after cgroup_disable= 
> on the command line and have it disabled by accident?
> 

How do you distinguish that from the user wanting to disable the controller on
purpose? My understanding is that after parsing cgroup_disable=, the rest of the
text is passed to cgroup_disable to process further. You'll find that all the
__setup() code in the kernel is implemented this way.

>> diff -puN Documentation/kernel-parameters.txt~cgroup_disable Documentation/kernel-parameters.txt
>> --- linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable	2008-03-06 17:57:32.000000000 +0530
>> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt	2008-03-06 18:00:32.000000000 +0530
>> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters. 
>>  	ccw_timeout_log [S390]
>>  			See Documentation/s390/CommonIO for details.
>>  
>> +	cgroup_disable= [KNL] Enable disable a particular controller
>> +			Format: {name of the controller}
>> +			See /proc/cgroups for a list of compiled controllers
>> +
> 
> This works on multiple controllers, though, if they follow 
> cgroup_disable=, so the documentation and format should reflect that.

Absolutely! done.

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

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Li Zefan]

Balbir Singh wrote:
> Li Zefan wrote:
>> Balbir Singh wrote:
>>> From: Paul Menage <menage@google.com>
>>>
>>> The effects of cgroup_disable=foo are:
>>>
>>> - foo doesn't show up in /proc/cgroups
>>> - foo isn't auto-mounted if you mount all cgroups in a single hierarchy
>>> - foo isn't visible as an individually mountable subsystem
>>>
>>> As a result there will only ever be one call to foo->create(), at init
>>> time; all processes will stay in this group, and the group will never
>>> be mounted on a visible hierarchy. Any additional effects (e.g. not
>>> allocating metadata) are up to the foo subsystem.
>>>
>>> This doesn't handle early_init subsystems (their "disabled" bit isn't
>>> set be, but it could easily be extended to do so if any of the early_init
>>> systems wanted it - I think it would just involve some nastier parameter
>>> processing since it would occur before the command-line argument parser
>>> had been run.
>>>
>>> [Balbir added Documentation/kernel-parameters updates]
>>>
>>> Signed-off-by: Paul Menage <menage@google.com>
>>> Signed-off-by: Balbir Singh <balbir@linux.vnet.ibm.com>
>>> ---
>>>
>>>  Documentation/kernel-parameters.txt |    4 ++++
>>>  include/linux/cgroup.h              |    1 +
>>>  kernel/cgroup.c                     |   27 +++++++++++++++++++++++++--
>>>  3 files changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff -puN include/linux/cgroup.h~cgroup_disable include/linux/cgroup.h
>>> --- linux-2.6.25-rc4/include/linux/cgroup.h~cgroup_disable   
>>> 2008-03-06 12:19:38.000000000 +0530
>>> +++ linux-2.6.25-rc4-balbir/include/linux/cgroup.h    2008-03-06
>>> 12:19:38.000000000 +0530
>>> @@ -256,6 +256,7 @@ struct cgroup_subsys {
>>>      void (*bind)(struct cgroup_subsys *ss, struct cgroup *root);
>>>      int subsys_id;
>>>      int active;
>>> +    int disabled;
>>>      int early_init;
>>>  #define MAX_CGROUP_TYPE_NAMELEN 32
>>>      const char *name;
>>> diff -puN kernel/cgroup.c~cgroup_disable kernel/cgroup.c
>>> --- linux-2.6.25-rc4/kernel/cgroup.c~cgroup_disable    2008-03-06
>>> 12:19:38.000000000 +0530
>>> +++ linux-2.6.25-rc4-balbir/kernel/cgroup.c    2008-03-06
>>> 12:19:38.000000000 +0530
>>> @@ -782,7 +782,14 @@ static int parse_cgroupfs_options(char *
>>>          if (!*token)
>>>              return -EINVAL;
>>>          if (!strcmp(token, "all")) {
>>> -            opts->subsys_bits = (1 << CGROUP_SUBSYS_COUNT) - 1;
>>> +            /* Add all non-disabled subsystems */
>>> +            int i;
>>> +            opts->subsys_bits = 0;
>>> +            for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>> +                struct cgroup_subsys *ss = subsys[i];
>>> +                if (!ss->disabled)
>>> +                    opts->subsys_bits |= 1ul << i;
>>> +            }
>>>          } else if (!strcmp(token, "noprefix")) {
>>>              set_bit(ROOT_NOPREFIX, &opts->flags);
>>>          } else if (!strncmp(token, "release_agent=", 14)) {
>>> @@ -800,7 +807,8 @@ static int parse_cgroupfs_options(char *
>>>              for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>>                  ss = subsys[i];
>>>                  if (!strcmp(token, ss->name)) {
>>> -                    set_bit(i, &opts->subsys_bits);
>>> +                    if (!ss->disabled)
>>> +                        set_bit(i, &opts->subsys_bits);
>>>                      break;
>>>                  }
>>>              }
>>> @@ -2604,6 +2612,8 @@ static int proc_cgroupstats_show(struct     
>>> mutex_lock(&cgroup_mutex);
>>>      for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>>          struct cgroup_subsys *ss = subsys[i];
>>> +        if (ss->disabled)
>>> +            continue;
>>>          seq_printf(m, "%s\t%lu\t%d\n",
>>>                 ss->name, ss->root->subsys_bits,
>>>                 ss->root->number_of_cgroups);
>>> @@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct     
>>> spin_unlock(&release_list_lock);
>>>      mutex_unlock(&cgroup_mutex);
>>>  }
>>> +
>>> +static int __init cgroup_disable(char *str)
>>> +{
>>> +    int i;
>>> +    for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
>>> +        struct cgroup_subsys *ss = subsys[i];
>>> +        if (!strcmp(str, ss->name)) {
>>> +            ss->disabled = 1;
>>> +            break;
>>> +        }
>>> +    }
>>> +}
>>> +__setup("cgroup_disable=", cgroup_disable);
>>> diff -puN Documentation/kernel-parameters.txt~cgroup_disable
>>> Documentation/kernel-parameters.txt
>>> ---
>>> linux-2.6.25-rc4/Documentation/kernel-parameters.txt~cgroup_disable   
>>> 2008-03-06 17:57:32.000000000 +0530
>>> +++ linux-2.6.25-rc4-balbir/Documentation/kernel-parameters.txt   
>>> 2008-03-06 18:00:32.000000000 +0530
>>> @@ -383,6 +383,10 @@ and is between 256 and 4096 characters.     
>>> ccw_timeout_log [S390]
>>>              See Documentation/s390/CommonIO for details.
>>>  
>>> +    cgroup_disable= [KNL] Enable disable a particular controller
>>> +            Format: {name of the controller}
>>> +            See /proc/cgroups for a list of compiled controllers
>>> +
>> The changelog of this patch:
>> - foo doesn't show up in /proc/cgroups
>>
>> So a disabled subsystem won't show up in /proc/cgroups. In a previous
>> mail, I asked whether it will be useful to print out the disable bit
>> in /proc/cgroups, so we can distinguish a subsystem from disaled and
>> not-compiled.
> 
> Hi, Li,
> 
> That is a good idea, but can that come in later? We need to get the boot option
> in, so that users can decide at boot time whether they want the page_container
> overhead. I'll send out another set of patches to add that feature or work
> with Paul to see what he thinks about it.
> 

I'm not requiring this to be done in this patch. :)

But my exact meaning here is this sentence is confusing:

	See /proc/cgroups for a list of compiled controllers

It seems it is telling people that /proc/cgroups shows not only enabled
but also disabled cgroup subsystems.

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[KAMEZAWA Hiroyuki]

On Fri, 07 Mar 2008 10:11:17 +0530
Balbir Singh <balbir@linux.vnet.ibm.com> wrote:

> David Rientjes wrote:
> > On Fri, 7 Mar 2008, Balbir Singh wrote:
> > 
> >> @@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct 
> >>  	spin_unlock(&release_list_lock);
> >>  	mutex_unlock(&cgroup_mutex);
> >>  }
> >> +
> >> +static int __init cgroup_disable(char *str)
> >> +{
> >> +	int i;
> >> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> >> +		struct cgroup_subsys *ss = subsys[i];
> >> +		if (!strcmp(str, ss->name)) {
> >> +			ss->disabled = 1;
> >> +			break;
> >> +		}
> >> +	}
> >> +}
> >> +__setup("cgroup_disable=", cgroup_disable);
> > 
> > This doesn't handle spaces very well, so isn't it possible for the name of 
> > a current or future cgroup subsystem to be specified after cgroup_disable= 
> > on the command line and have it disabled by accident?
> > 
> 
Hmm, cmdline like

cgroup_disable=cpu,memory, ...

should be written as

cgroup_disable=cpu cgroup_disable=memory ....

?
Thanks,
-Kame

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[David Rientjes]

On Fri, 7 Mar 2008, Balbir Singh wrote:

> >> +static int __init cgroup_disable(char *str)
> >> +{
> >> +	int i;
> >> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> >> +		struct cgroup_subsys *ss = subsys[i];
> >> +		if (!strcmp(str, ss->name)) {
> >> +			ss->disabled = 1;
> >> +			break;
> >> +		}
> >> +	}
> >> +}
> >> +__setup("cgroup_disable=", cgroup_disable);
> > 
> > This doesn't handle spaces very well, so isn't it possible for the name of 
> > a current or future cgroup subsystem to be specified after cgroup_disable= 
> > on the command line and have it disabled by accident?
> > 
> 
> How do you distinguish that from the user wanting to disable the controller on
> purpose? My understanding is that after parsing cgroup_disable=, the rest of the
> text is passed to cgroup_disable to process further. You'll find that all the
> __setup() code in the kernel is implemented this way.
> 

Since the command line is logically delimited by spaces, you can 
accidently disable a subsystem if its name appears in any of your kernel 
options following your cgroup_disable= option.  So if you're absolutely 
confident that it wouldn't happen (for instance, if there's no logical 
reason that a cgroup subsystem name should appear anywhere besides 
cgroup_disable on the command line), then there's no objection.

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[David Rientjes]

On Fri, 7 Mar 2008, KAMEZAWA Hiroyuki wrote:

> > David Rientjes wrote:
> > > On Fri, 7 Mar 2008, Balbir Singh wrote:
> > > 
> > >> @@ -3010,3 +3020,16 @@ static void cgroup_release_agent(struct 
> > >>  	spin_unlock(&release_list_lock);
> > >>  	mutex_unlock(&cgroup_mutex);
> > >>  }
> > >> +
> > >> +static int __init cgroup_disable(char *str)
> > >> +{
> > >> +	int i;
> > >> +	for (i = 0; i < CGROUP_SUBSYS_COUNT; i++) {
> > >> +		struct cgroup_subsys *ss = subsys[i];
> > >> +		if (!strcmp(str, ss->name)) {
> > >> +			ss->disabled = 1;
> > >> +			break;
> > >> +		}
> > >> +	}
> > >> +}
> > >> +__setup("cgroup_disable=", cgroup_disable);
> > > 
> > > This doesn't handle spaces very well, so isn't it possible for the name of 
> > > a current or future cgroup subsystem to be specified after cgroup_disable= 
> > > on the command line and have it disabled by accident?
> > > 
> > 
> Hmm, cmdline like
> 
> cgroup_disable=cpu,memory, ...
> 
> should be written as
> 
> cgroup_disable=cpu cgroup_disable=memory ....
> 

Or just set the first space following cgroup_disable= to '\0' and you're 
done.  strcmp() will take care of the rest.

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Paul Menage]

On Thu, Mar 6, 2008 at 9:14 PM, David Rientjes <rientjes@google.com> wrote:
>
>  Since the command line is logically delimited by spaces, you can
>  accidently disable a subsystem if its name appears in any of your kernel
>  options following your cgroup_disable= option.

I think that you're confusing this with things like the very early
memory init setup parameters, which do operate on the raw commandline.

By the time anything is passed to a __setup() function, it's already
been split into separate strings at space boundaries.

Paul

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[David Rientjes]

On Fri, 7 Mar 2008, Paul Menage wrote:

> >  Since the command line is logically delimited by spaces, you can
> >  accidently disable a subsystem if its name appears in any of your kernel
> >  options following your cgroup_disable= option.
> 
> I think that you're confusing this with things like the very early
> memory init setup parameters, which do operate on the raw commandline.
> 
> By the time anything is passed to a __setup() function, it's already
> been split into separate strings at space boundaries.
> 

Ok, so the cgroup_disable= parameter should be a list of subsystem names 
delimited by anything other than a space that the user wants disabled.  
That makes more sense, thanks.

Re: [PATCH] Add cgroup support for enabling controllers at boot time

[Paul Menage]

On Fri, Mar 7, 2008 at 12:56 AM, David Rientjes <rientjes@google.com> wrote:
>
>  Ok, so the cgroup_disable= parameter should be a list of subsystem names
>  delimited by anything other than a space that the user wants disabled.
>  That makes more sense, thanks.
>

As the code stands now, it should be just a single name. Disabling
multiple subsystems requires multiple cgroup_disable= options.

Paul