LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups
@ 2008-02-21 21:28 menage
  2008-02-21 21:28 ` [PATCH 1/2] cgroup map files: Add cgroup map data type menage
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: menage @ 2008-02-21 21:28 UTC (permalink / raw)
  To: kamezawa.hiroyu, yamamoto, akpm; +Cc: linux-kernel, linux-mm, balbir, xemul

[ Updated from the previous version to remove the colon from the map output ]

These patches add a new cgroup control file output type - a map from
strings to u64 values - and make use of it for the memory controller
"stat" file.

It is intended for use when the subsystem wants to return a collection
of values that are related in some way, for which a separate control
file for each value would make the reporting unwieldy.

The advantages of this are:

- more standardized output from control files that report
similarly-structured data that needs to be parsed programmatically

- less boilerplate required in cgroup subsystems

- simplifies transition to a future efficient cgroups binary API

Signed-off-by: Paul Menage <menage@google.com>


--

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] cgroup map files: Add cgroup map data type
  2008-02-21 21:28 [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups menage
@ 2008-02-21 21:28 ` menage
  2008-02-22  3:51   ` YAMAMOTO Takashi
  2008-02-23  8:04   ` Andrew Morton
  2008-02-21 21:28 ` [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file menage
  2008-02-23  8:04 ` [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups Andrew Morton
  2 siblings, 2 replies; 9+ messages in thread
From: menage @ 2008-02-21 21:28 UTC (permalink / raw)
  To: kamezawa.hiroyu, yamamoto, akpm; +Cc: linux-kernel, linux-mm, balbir, xemul

[-- Attachment #1: cgroup_read_map.patch --]
[-- Type: text/plain, Size: 3934 bytes --]

Adds a new type of supported control file representation, a map from
strings to u64 values.

The map type is printed in a similar format to /proc/meminfo or
/proc/<pid>/status, i.e. "$key: $value\n"

Signed-off-by: Paul Menage <menage@google.com>

---
 include/linux/cgroup.h |   19 +++++++++++++++
 kernel/cgroup.c        |   59 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 77 insertions(+), 1 deletion(-)

Index: cgroupmap-2.6.25-rc2-mm1/include/linux/cgroup.h
===================================================================
--- cgroupmap-2.6.25-rc2-mm1.orig/include/linux/cgroup.h
+++ cgroupmap-2.6.25-rc2-mm1/include/linux/cgroup.h
@@ -166,6 +166,16 @@ struct css_set {
 
 };
 
+/*
+ * cgroup_map_cb is an abstract callback API for reporting map-valued
+ * control files
+ */
+
+struct cgroup_map_cb {
+	int (*fill)(struct cgroup_map_cb *cb, const char *key, u64 value);
+	void *state;
+};
+
 /* struct cftype:
  *
  * The files in the cgroup filesystem mostly have a very simple read/write
@@ -194,6 +204,15 @@ struct cftype {
 	 * single integer. Use it in place of read()
 	 */
 	u64 (*read_uint) (struct cgroup *cont, struct cftype *cft);
+	/*
+	 * read_map() is used for defining a map of key/value
+	 * pairs. It should call cb->fill(cb, key, value) for each
+	 * entry. The key/value pairs (and their ordering) should not
+	 * change between reboots.
+	 */
+	int (*read_map) (struct cgroup *cont, struct cftype *cft,
+			 struct cgroup_map_cb *cb);
+
 	ssize_t (*write) (struct cgroup *cont, struct cftype *cft,
 			  struct file *file,
 			  const char __user *buf, size_t nbytes, loff_t *ppos);
Index: cgroupmap-2.6.25-rc2-mm1/kernel/cgroup.c
===================================================================
--- cgroupmap-2.6.25-rc2-mm1.orig/kernel/cgroup.c
+++ cgroupmap-2.6.25-rc2-mm1/kernel/cgroup.c
@@ -1487,6 +1487,46 @@ static ssize_t cgroup_file_read(struct f
 	return -EINVAL;
 }
 
+/*
+ * seqfile ops/methods for returning structured data. Currently just
+ * supports string->u64 maps, but can be extended in future.
+ */
+
+struct cgroup_seqfile_state {
+	struct cftype *cft;
+	struct cgroup *cgroup;
+};
+
+static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
+{
+	struct seq_file *sf = cb->state;
+	return seq_printf(sf, "%s %llu\n", key, value);
+}
+
+static int cgroup_seqfile_show(struct seq_file *m, void *arg)
+{
+	struct cgroup_seqfile_state *state = m->private;
+	struct cftype *cft = state->cft;
+	if (cft->read_map) {
+		struct cgroup_map_cb cb = {
+			.fill = cgroup_map_add,
+			.state = m,
+		};
+		return cft->read_map(state->cgroup, cft, &cb);
+	} else {
+		BUG();
+	}
+}
+
+int cgroup_seqfile_release(struct inode *inode, struct file *file)
+{
+	struct seq_file *seq = file->private_data;
+	kfree(seq->private);
+	return single_release(inode, file);
+}
+
+static struct file_operations cgroup_seqfile_operations;
+
 static int cgroup_file_open(struct inode *inode, struct file *file)
 {
 	int err;
@@ -1499,7 +1539,18 @@ static int cgroup_file_open(struct inode
 	cft = __d_cft(file->f_dentry);
 	if (!cft)
 		return -ENODEV;
-	if (cft->open)
+	if (cft->read_map) {
+		struct cgroup_seqfile_state *state =
+			kzalloc(sizeof(*state), GFP_USER);
+		if (!state)
+			return -ENOMEM;
+		state->cft = cft;
+		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
+		file->f_op = &cgroup_seqfile_operations;
+		err = single_open(file, cgroup_seqfile_show, state);
+		if (err < 0)
+			kfree(state);
+	} else if (cft->open)
 		err = cft->open(inode, file);
 	else
 		err = 0;
@@ -1538,6 +1589,12 @@ static struct file_operations cgroup_fil
 	.release = cgroup_file_release,
 };
 
+static struct file_operations cgroup_seqfile_operations = {
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = cgroup_seqfile_release,
+};
+
 static struct inode_operations cgroup_dir_inode_operations = {
 	.lookup = simple_lookup,
 	.mkdir = cgroup_mkdir,

--

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file
  2008-02-21 21:28 [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups menage
  2008-02-21 21:28 ` [PATCH 1/2] cgroup map files: Add cgroup map data type menage
@ 2008-02-21 21:28 ` menage
  2008-02-23  8:04   ` Andrew Morton
  2008-02-23  8:04 ` [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups Andrew Morton
  2 siblings, 1 reply; 9+ messages in thread
From: menage @ 2008-02-21 21:28 UTC (permalink / raw)
  To: kamezawa.hiroyu, yamamoto, akpm; +Cc: linux-kernel, linux-mm, balbir, xemul

[-- Attachment #1: memcontrol_use_cgroup_map.patch --]
[-- Type: text/plain, Size: 2382 bytes --]

Remove the seq_file boilerplate used to construct the memcontrol stats
map, and instead use the new map representation for cgroup control
files

Signed-off-by: Paul Menage <menage@google.com>

---
 mm/memcontrol.c |   30 ++++++------------------------
 1 file changed, 6 insertions(+), 24 deletions(-)

Index: cgroupmap-2.6.25-rc2-mm1/mm/memcontrol.c
===================================================================
--- cgroupmap-2.6.25-rc2-mm1.orig/mm/memcontrol.c
+++ cgroupmap-2.6.25-rc2-mm1/mm/memcontrol.c
@@ -974,9 +974,9 @@ static const struct mem_cgroup_stat_desc
 	[MEM_CGROUP_STAT_RSS] = { "rss", PAGE_SIZE, },
 };
 
-static int mem_control_stat_show(struct seq_file *m, void *arg)
+static int mem_control_stat_show(struct cgroup *cont, struct cftype *cft,
+				 struct cgroup_map_cb *cb)
 {
-	struct cgroup *cont = m->private;
 	struct mem_cgroup *mem_cont = mem_cgroup_from_cont(cont);
 	struct mem_cgroup_stat *stat = &mem_cont->stat;
 	int i;
@@ -986,8 +986,7 @@ static int mem_control_stat_show(struct 
 
 		val = mem_cgroup_read_stat(stat, i);
 		val *= mem_cgroup_stat_desc[i].unit;
-		seq_printf(m, "%s %lld\n", mem_cgroup_stat_desc[i].msg,
-				(long long)val);
+		cb->fill(cb, mem_cgroup_stat_desc[i].msg, val);
 	}
 	/* showing # of active pages */
 	{
@@ -997,29 +996,12 @@ static int mem_control_stat_show(struct 
 						MEM_CGROUP_ZSTAT_INACTIVE);
 		active = mem_cgroup_get_all_zonestat(mem_cont,
 						MEM_CGROUP_ZSTAT_ACTIVE);
-		seq_printf(m, "active %ld\n", (active) * PAGE_SIZE);
-		seq_printf(m, "inactive %ld\n", (inactive) * PAGE_SIZE);
+		cb->fill(cb, "active", (active) * PAGE_SIZE);
+		cb->fill(cb, "inactive", (inactive) * PAGE_SIZE);
 	}
 	return 0;
 }
 
-static const struct file_operations mem_control_stat_file_operations = {
-	.read = seq_read,
-	.llseek = seq_lseek,
-	.release = single_release,
-};
-
-static int mem_control_stat_open(struct inode *unused, struct file *file)
-{
-	/* XXX __d_cont */
-	struct cgroup *cont = file->f_dentry->d_parent->d_fsdata;
-
-	file->f_op = &mem_control_stat_file_operations;
-	return single_open(file, mem_control_stat_show, cont);
-}
-
-
-
 static struct cftype mem_cgroup_files[] = {
 	{
 		.name = "usage_in_bytes",
@@ -1044,7 +1026,7 @@ static struct cftype mem_cgroup_files[] 
 	},
 	{
 		.name = "stat",
-		.open = mem_control_stat_open,
+		.read_map = mem_control_stat_show,
 	},
 };
 

--

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] cgroup map files: Add cgroup map data type
  2008-02-21 21:28 ` [PATCH 1/2] cgroup map files: Add cgroup map data type menage
@ 2008-02-22  3:51   ` YAMAMOTO Takashi
  2008-02-23  8:04   ` Andrew Morton
  1 sibling, 0 replies; 9+ messages in thread
From: YAMAMOTO Takashi @ 2008-02-22  3:51 UTC (permalink / raw)
  To: menage; +Cc: kamezawa.hiroyu, akpm, linux-kernel, linux-mm, balbir, xemul

> The map type is printed in a similar format to /proc/meminfo or
> /proc/<pid>/status, i.e. "$key: $value\n"

this description doesn't seem to match with the code.

YAMAMOTO Takashi

> +static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
> +{
> +	struct seq_file *sf = cb->state;
> +	return seq_printf(sf, "%s %llu\n", key, value);
> +}

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups
  2008-02-21 21:28 [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups menage
  2008-02-21 21:28 ` [PATCH 1/2] cgroup map files: Add cgroup map data type menage
  2008-02-21 21:28 ` [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file menage
@ 2008-02-23  8:04 ` Andrew Morton
  2 siblings, 0 replies; 9+ messages in thread
From: Andrew Morton @ 2008-02-23  8:04 UTC (permalink / raw)
  To: menage; +Cc: kamezawa.hiroyu, yamamoto, linux-kernel, linux-mm, balbir, xemul

On Thu, 21 Feb 2008 13:28:54 -0800 menage@google.com wrote:

> These patches add a new cgroup control file output type - a map from
> strings to u64 values - and make use of it for the memory controller
> "stat" file.

Can we document the moderately obscure kernel->userspace interface
somewhere please?

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] cgroup map files: Add cgroup map data type
  2008-02-21 21:28 ` [PATCH 1/2] cgroup map files: Add cgroup map data type menage
  2008-02-22  3:51   ` YAMAMOTO Takashi
@ 2008-02-23  8:04   ` Andrew Morton
  2008-02-23 15:22     ` Paul Menage
  1 sibling, 1 reply; 9+ messages in thread
From: Andrew Morton @ 2008-02-23  8:04 UTC (permalink / raw)
  To: menage; +Cc: kamezawa.hiroyu, yamamoto, linux-kernel, linux-mm, balbir, xemul

On Thu, 21 Feb 2008 13:28:55 -0800 menage@google.com wrote:

> Adds a new type of supported control file representation, a map from
> strings to u64 values.
> 
> The map type is printed in a similar format to /proc/meminfo or
> /proc/<pid>/status, i.e. "$key: $value\n"
> 
> Signed-off-by: Paul Menage <menage@google.com>
> 
> ---
>  include/linux/cgroup.h |   19 +++++++++++++++
>  kernel/cgroup.c        |   59 ++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 77 insertions(+), 1 deletion(-)
> 
> Index: cgroupmap-2.6.25-rc2-mm1/include/linux/cgroup.h
> ===================================================================
> --- cgroupmap-2.6.25-rc2-mm1.orig/include/linux/cgroup.h
> +++ cgroupmap-2.6.25-rc2-mm1/include/linux/cgroup.h
> @@ -166,6 +166,16 @@ struct css_set {
>  
>  };
>  
> +/*
> + * cgroup_map_cb is an abstract callback API for reporting map-valued
> + * control files
> + */
> +
> +struct cgroup_map_cb {
> +	int (*fill)(struct cgroup_map_cb *cb, const char *key, u64 value);
> +	void *state;
> +};
> +
>  /* struct cftype:
>   *
>   * The files in the cgroup filesystem mostly have a very simple read/write
> @@ -194,6 +204,15 @@ struct cftype {
>  	 * single integer. Use it in place of read()
>  	 */
>  	u64 (*read_uint) (struct cgroup *cont, struct cftype *cft);
> +	/*
> +	 * read_map() is used for defining a map of key/value
> +	 * pairs. It should call cb->fill(cb, key, value) for each
> +	 * entry. The key/value pairs (and their ordering) should not
> +	 * change between reboots.
> +	 */
> +	int (*read_map) (struct cgroup *cont, struct cftype *cft,
> +			 struct cgroup_map_cb *cb);
> +
>  	ssize_t (*write) (struct cgroup *cont, struct cftype *cft,
>  			  struct file *file,
>  			  const char __user *buf, size_t nbytes, loff_t *ppos);
> Index: cgroupmap-2.6.25-rc2-mm1/kernel/cgroup.c
> ===================================================================
> --- cgroupmap-2.6.25-rc2-mm1.orig/kernel/cgroup.c
> +++ cgroupmap-2.6.25-rc2-mm1/kernel/cgroup.c
> @@ -1487,6 +1487,46 @@ static ssize_t cgroup_file_read(struct f
>  	return -EINVAL;
>  }
>  
> +/*
> + * seqfile ops/methods for returning structured data. Currently just
> + * supports string->u64 maps, but can be extended in future.
> + */
> +
> +struct cgroup_seqfile_state {
> +	struct cftype *cft;
> +	struct cgroup *cgroup;
> +};
> +
> +static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
> +{
> +	struct seq_file *sf = cb->state;
> +	return seq_printf(sf, "%s %llu\n", key, value);
> +}

We don't know what type the architecture uses to implement u64.  This will
warn on powerpc, sparc64, maybe others.

> +static int cgroup_seqfile_show(struct seq_file *m, void *arg)
> +{
> +	struct cgroup_seqfile_state *state = m->private;
> +	struct cftype *cft = state->cft;
> +	if (cft->read_map) {
> +		struct cgroup_map_cb cb = {
> +			.fill = cgroup_map_add,
> +			.state = m,
> +		};
> +		return cft->read_map(state->cgroup, cft, &cb);
> +	} else {
> +		BUG();

That's not really needed.  Just call cft->read_map unconditionally.  if
it's zero we'll get a null-pointer deref which will have just the same
effect as a BUG.

> +	}
> +}
> +
> +int cgroup_seqfile_release(struct inode *inode, struct file *file)
> +{
> +	struct seq_file *seq = file->private_data;
> +	kfree(seq->private);
> +	return single_release(inode, file);
> +}
> +
> +static struct file_operations cgroup_seqfile_operations;

afaict you can just move the definition of cgroup_seqfile_operations here
and avoid the forward decl.

>  static int cgroup_file_open(struct inode *inode, struct file *file)
>  {
>  	int err;
> @@ -1499,7 +1539,18 @@ static int cgroup_file_open(struct inode
>  	cft = __d_cft(file->f_dentry);
>  	if (!cft)
>  		return -ENODEV;
> -	if (cft->open)
> +	if (cft->read_map) {

But above a NULL value is illegal.  Why are we testing it here?

> +		struct cgroup_seqfile_state *state =
> +			kzalloc(sizeof(*state), GFP_USER);
> +		if (!state)
> +			return -ENOMEM;
> +		state->cft = cft;
> +		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
> +		file->f_op = &cgroup_seqfile_operations;
> +		err = single_open(file, cgroup_seqfile_show, state);
> +		if (err < 0)
> +			kfree(state);
> +	} else if (cft->open)
>  		err = cft->open(inode, file);
>  	else
>  		err = 0;
> @@ -1538,6 +1589,12 @@ static struct file_operations cgroup_fil
>  	.release = cgroup_file_release,
>  };
>  
> +static struct file_operations cgroup_seqfile_operations = {
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = cgroup_seqfile_release,
> +};
> +
>  static struct inode_operations cgroup_dir_inode_operations = {
>  	.lookup = simple_lookup,
>  	.mkdir = cgroup_mkdir,
> 
> --

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file
  2008-02-21 21:28 ` [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file menage
@ 2008-02-23  8:04   ` Andrew Morton
  0 siblings, 0 replies; 9+ messages in thread
From: Andrew Morton @ 2008-02-23  8:04 UTC (permalink / raw)
  To: menage; +Cc: kamezawa.hiroyu, yamamoto, linux-kernel, linux-mm, balbir, xemul

On Thu, 21 Feb 2008 13:28:56 -0800 menage@google.com wrote:

> Remove the seq_file boilerplate used to construct the memcontrol stats
> map, and instead use the new map representation for cgroup control
> files
> 
> Signed-off-by: Paul Menage <menage@google.com>
> 
> ---
>  mm/memcontrol.c |   30 ++++++------------------------
>  1 file changed, 6 insertions(+), 24 deletions(-)
> 
> Index: cgroupmap-2.6.25-rc2-mm1/mm/memcontrol.c
> ===================================================================
> --- cgroupmap-2.6.25-rc2-mm1.orig/mm/memcontrol.c
> +++ cgroupmap-2.6.25-rc2-mm1/mm/memcontrol.c
> @@ -974,9 +974,9 @@ static const struct mem_cgroup_stat_desc
>  	[MEM_CGROUP_STAT_RSS] = { "rss", PAGE_SIZE, },
>  };
>  
> -static int mem_control_stat_show(struct seq_file *m, void *arg)
> +static int mem_control_stat_show(struct cgroup *cont, struct cftype *cft,
> +				 struct cgroup_map_cb *cb)
>  {
> -	struct cgroup *cont = m->private;
>  	struct mem_cgroup *mem_cont = mem_cgroup_from_cont(cont);
>  	struct mem_cgroup_stat *stat = &mem_cont->stat;
>  	int i;
> @@ -986,8 +986,7 @@ static int mem_control_stat_show(struct 
>  
>  		val = mem_cgroup_read_stat(stat, i);
>  		val *= mem_cgroup_stat_desc[i].unit;
> -		seq_printf(m, "%s %lld\n", mem_cgroup_stat_desc[i].msg,
> -				(long long)val);
> +		cb->fill(cb, mem_cgroup_stat_desc[i].msg, val);
>  	}
>  	/* showing # of active pages */
>  	{
> @@ -997,29 +996,12 @@ static int mem_control_stat_show(struct 
>  						MEM_CGROUP_ZSTAT_INACTIVE);
>  		active = mem_cgroup_get_all_zonestat(mem_cont,
>  						MEM_CGROUP_ZSTAT_ACTIVE);
> -		seq_printf(m, "active %ld\n", (active) * PAGE_SIZE);
> -		seq_printf(m, "inactive %ld\n", (inactive) * PAGE_SIZE);
> +		cb->fill(cb, "active", (active) * PAGE_SIZE);
> +		cb->fill(cb, "inactive", (inactive) * PAGE_SIZE);

umm, afacit this might be a prior bug in mem_control_stat_show().  On a
32-bit machine can [in]active*PAGE_SIZE overflow 32-bits?


>  	}
>  	return 0;
>  }
>  
> -static const struct file_operations mem_control_stat_file_operations = {
> -	.read = seq_read,
> -	.llseek = seq_lseek,
> -	.release = single_release,
> -};
> -
> -static int mem_control_stat_open(struct inode *unused, struct file *file)
> -{
> -	/* XXX __d_cont */
> -	struct cgroup *cont = file->f_dentry->d_parent->d_fsdata;
> -
> -	file->f_op = &mem_control_stat_file_operations;
> -	return single_open(file, mem_control_stat_show, cont);
> -}
> -
> -
> -
>  static struct cftype mem_cgroup_files[] = {
>  	{
>  		.name = "usage_in_bytes",
> @@ -1044,7 +1026,7 @@ static struct cftype mem_cgroup_files[] 
>  	},
>  	{
>  		.name = "stat",
> -		.open = mem_control_stat_open,
> +		.read_map = mem_control_stat_show,
>  	},
>  };
>  
> 
> --

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH 1/2] cgroup map files: Add cgroup map data type
  2008-02-23  8:04   ` Andrew Morton
@ 2008-02-23 15:22     ` Paul Menage
  0 siblings, 0 replies; 9+ messages in thread
From: Paul Menage @ 2008-02-23 15:22 UTC (permalink / raw)
  To: Andrew Morton
  Cc: kamezawa.hiroyu, yamamoto, linux-kernel, linux-mm, balbir, xemul

On Sat, Feb 23, 2008 at 12:04 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
>  > +static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
>  > +{
>  > +     struct seq_file *sf = cb->state;
>  > +     return seq_printf(sf, "%s %llu\n", key, value);
>  > +}
>
>  We don't know what type the architecture uses to implement u64.  This will
>  warn on powerpc, sparc64, maybe others.

OK, I'll add an (unsigned long long) cast.

>
>
>  > +static int cgroup_seqfile_show(struct seq_file *m, void *arg)
>  > +{
>  > +     struct cgroup_seqfile_state *state = m->private;
>  > +     struct cftype *cft = state->cft;
>  > +     if (cft->read_map) {
>  > +             struct cgroup_map_cb cb = {
>  > +                     .fill = cgroup_map_add,
>  > +                     .state = m,
>  > +             };
>  > +             return cft->read_map(state->cgroup, cft, &cb);
>  > +     } else {
>  > +             BUG();
>
>  That's not really needed.  Just call cft->read_map unconditionally.  if
>  it's zero we'll get a null-pointer deref which will have just the same
>  effect as a BUG.

OK. The long-term plan is to have other kinds of files also handled by
this function, so eventually it would look something like:

if (cft->read_map) {
...
} else if (cft->read_something_else) {
...
}
...
} else {
  BUG();
}

But I guess I can save that for the future.

>  >  static int cgroup_file_open(struct inode *inode, struct file *file)
>  >  {
>  >       int err;
>  > @@ -1499,7 +1539,18 @@ static int cgroup_file_open(struct inode
>  >       cft = __d_cft(file->f_dentry);
>  >       if (!cft)
>  >               return -ENODEV;
>  > -     if (cft->open)
>  > +     if (cft->read_map) {
>
>  But above a NULL value is illegal.  Why are we testing it here?
>
>

The existence of cft->read_map causes us to open a seq_file. Otherwise
we do nothing special and carry on down the normal open path.

Paul

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH 1/2] cgroup map files: Add cgroup map data type
  2008-02-20  5:15 menage
@ 2008-02-20  5:15 ` menage
  0 siblings, 0 replies; 9+ messages in thread
From: menage @ 2008-02-20  5:15 UTC (permalink / raw)
  To: kamezawa.hiroyu, akpm; +Cc: linux-kernel, linux-mm, balbir, xemul, yamamoto

[-- Attachment #1: cgroup_read_map.patch --]
[-- Type: text/plain, Size: 3824 bytes --]

Adds a new type of supported control file representation, a map from
strings to u64 values.

Signed-off-by: Paul Menage <menage@google.com>

---
 include/linux/cgroup.h |   19 +++++++++++++++
 kernel/cgroup.c        |   59 ++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 77 insertions(+), 1 deletion(-)

Index: cgroupmap-2.6.25-rc2-mm1/include/linux/cgroup.h
===================================================================
--- cgroupmap-2.6.25-rc2-mm1.orig/include/linux/cgroup.h
+++ cgroupmap-2.6.25-rc2-mm1/include/linux/cgroup.h
@@ -166,6 +166,16 @@ struct css_set {
 
 };
 
+/*
+ * cgroup_map_cb is an abstract callback API for reporting map-valued
+ * control files
+ */
+
+struct cgroup_map_cb {
+	int (*fill)(struct cgroup_map_cb *cb, const char *key, u64 value);
+	void *state;
+};
+
 /* struct cftype:
  *
  * The files in the cgroup filesystem mostly have a very simple read/write
@@ -194,6 +204,15 @@ struct cftype {
 	 * single integer. Use it in place of read()
 	 */
 	u64 (*read_uint) (struct cgroup *cont, struct cftype *cft);
+	/*
+	 * read_map() is used for defining a map of key/value
+	 * pairs. It should call cb->fill(cb, key, value) for each
+	 * entry. The key/value pairs (and their ordering) should not
+	 * change between reboots.
+	 */
+	int (*read_map) (struct cgroup *cont, struct cftype *cft,
+			 struct cgroup_map_cb *cb);
+
 	ssize_t (*write) (struct cgroup *cont, struct cftype *cft,
 			  struct file *file,
 			  const char __user *buf, size_t nbytes, loff_t *ppos);
Index: cgroupmap-2.6.25-rc2-mm1/kernel/cgroup.c
===================================================================
--- cgroupmap-2.6.25-rc2-mm1.orig/kernel/cgroup.c
+++ cgroupmap-2.6.25-rc2-mm1/kernel/cgroup.c
@@ -1487,6 +1487,46 @@ static ssize_t cgroup_file_read(struct f
 	return -EINVAL;
 }
 
+/*
+ * seqfile ops/methods for returning structured data. Currently just
+ * supports string->u64 maps, but can be extended in future.
+ */
+
+struct cgroup_seqfile_state {
+	struct cftype *cft;
+	struct cgroup *cgroup;
+};
+
+static int cgroup_map_add(struct cgroup_map_cb *cb, const char *key, u64 value)
+{
+	struct seq_file *sf = cb->state;
+	return seq_printf(sf, "%s: %llu\n", key, value);
+}
+
+static int cgroup_seqfile_show(struct seq_file *m, void *arg)
+{
+	struct cgroup_seqfile_state *state = m->private;
+	struct cftype *cft = state->cft;
+	struct cgroup_map_cb cb = {
+		.fill = cgroup_map_add,
+		.state = m,
+	};
+	if (cft->read_map) {
+		return cft->read_map(state->cgroup, cft, &cb);
+	} else {
+		BUG();
+	}
+}
+
+int cgroup_seqfile_release(struct inode *inode, struct file *file)
+{
+	struct seq_file *seq = file->private_data;
+	kfree(seq->private);
+	return single_release(inode, file);
+}
+
+static struct file_operations cgroup_seqfile_operations;
+
 static int cgroup_file_open(struct inode *inode, struct file *file)
 {
 	int err;
@@ -1499,7 +1539,18 @@ static int cgroup_file_open(struct inode
 	cft = __d_cft(file->f_dentry);
 	if (!cft)
 		return -ENODEV;
-	if (cft->open)
+	if (cft->read_map) {
+		struct cgroup_seqfile_state *state =
+			kzalloc(sizeof(*state), GFP_USER);
+		if (!state)
+			return -ENOMEM;
+		state->cft = cft;
+		state->cgroup = __d_cgrp(file->f_dentry->d_parent);
+		file->f_op = &cgroup_seqfile_operations;
+		err = single_open(file, cgroup_seqfile_show, state);
+		if (err < 0)
+			kfree(state);
+	} else if (cft->open)
 		err = cft->open(inode, file);
 	else
 		err = 0;
@@ -1538,6 +1589,12 @@ static struct file_operations cgroup_fil
 	.release = cgroup_file_release,
 };
 
+static struct file_operations cgroup_seqfile_operations = {
+	.read = seq_read,
+	.llseek = seq_lseek,
+	.release = cgroup_seqfile_release,
+};
+
 static struct inode_operations cgroup_dir_inode_operations = {
 	.lookup = simple_lookup,
 	.mkdir = cgroup_mkdir,

--

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2008-02-23 15:22 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2008-02-21 21:28 [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups menage
2008-02-21 21:28 ` [PATCH 1/2] cgroup map files: Add cgroup map data type menage
2008-02-22  3:51   ` YAMAMOTO Takashi
2008-02-23  8:04   ` Andrew Morton
2008-02-23 15:22     ` Paul Menage
2008-02-21 21:28 ` [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file menage
2008-02-23  8:04   ` Andrew Morton
2008-02-23  8:04 ` [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups Andrew Morton
  -- strict thread matches above, loose matches on Subject: below --
2008-02-20  5:15 menage
2008-02-20  5:15 ` [PATCH 1/2] cgroup map files: Add cgroup map data type menage

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).