[PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[menage]

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


--

[PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[menage]

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


--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] cgroup map files: Add cgroup map data type

[menage]

[-- 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,

--

[PATCH 1/2] cgroup map files: Add cgroup map data type

[menage]

[-- Attachment #1: cgroup_read_map.patch --]
[-- Type: text/plain, Size: 4160 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,

--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] cgroup map files: Add cgroup map data type

[YAMAMOTO Takashi]

> 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);
> +}

Re: [PATCH 1/2] cgroup map files: Add cgroup map data type

[YAMAMOTO Takashi]

> 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);
> +}

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] cgroup map files: Add cgroup map data type

[Andrew Morton]

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

Re: [PATCH 1/2] cgroup map files: Add cgroup map data type

[Andrew Morton]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] cgroup map files: Add cgroup map data type

[Paul Menage]

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

Re: [PATCH 1/2] cgroup map files: Add cgroup map data type

[Paul Menage]

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file

[menage]

[-- 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,
 	},
 };
 

--

[PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file

[menage]

[-- Attachment #1: memcontrol_use_cgroup_map.patch --]
[-- Type: text/plain, Size: 2608 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,
 	},
 };
 

--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file

[Andrew Morton]

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,
>  	},
>  };
>  
> 
> --

Re: [PATCH 2/2] cgroup map files: Use cgroup map for memcontrol stats file

[Andrew Morton]

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,
>  	},
>  };
>  
> 
> --

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[Andrew Morton]

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?

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[Andrew Morton]

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?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[menage]

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

- less boilerplate required in cgroup subsystems

- simplifies transition to a future efficient cgroups binary API

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

--

[PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[menage]

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

- less boilerplate required in cgroup subsystems

- simplifies transition to a future efficient cgroups binary API

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

--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[YAMAMOTO Takashi]

> 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
> 
> - less boilerplate required in cgroup subsystems
> 
> - simplifies transition to a future efficient cgroups binary API
> 
> Signed-off-by: Paul Menage <menage@google.com>

it changes the format from "%s %lld" to "%s: %llu", right?
why?

YAMAMOTO Takashi

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[YAMAMOTO Takashi]

> 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
> 
> - less boilerplate required in cgroup subsystems
> 
> - simplifies transition to a future efficient cgroups binary API
> 
> Signed-off-by: Paul Menage <menage@google.com>

it changes the format from "%s %lld" to "%s: %llu", right?
why?

YAMAMOTO Takashi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[Paul Menage]

On Feb 19, 2008 9:48 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
>
> it changes the format from "%s %lld" to "%s: %llu", right?
> why?
>

The colon for consistency with maps in /proc. I think it also makes it
slightly more readable.

For %lld versus %llu - I think that cgroup resource APIs are much more
likely to need to report unsigned rather than signed values. In the
case of the memory.stat file, that's certainly the case.

But I guess there's an argument to be made that nothing's likely to
need the final 64th bit of an unsigned value, whereas the ability to
report negative numbers could potentially be useful for some cgroups.

Paul

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[Paul Menage]

On Feb 19, 2008 9:48 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
>
> it changes the format from "%s %lld" to "%s: %llu", right?
> why?
>

The colon for consistency with maps in /proc. I think it also makes it
slightly more readable.

For %lld versus %llu - I think that cgroup resource APIs are much more
likely to need to report unsigned rather than signed values. In the
case of the memory.stat file, that's certainly the case.

But I guess there's an argument to be made that nothing's likely to
need the final 64th bit of an unsigned value, whereas the ability to
report negative numbers could potentially be useful for some cgroups.

Paul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[YAMAMOTO Takashi]

> On Feb 19, 2008 9:48 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
> >
> > it changes the format from "%s %lld" to "%s: %llu", right?
> > why?
> >
> 
> The colon for consistency with maps in /proc. I think it also makes it
> slightly more readable.

can you be a little more specific?

i object against the colon because i want to use the same parser for
/proc/vmstat, which doesn't have colons.

btw, when making ABI changes like this, can you please mention it
explicitly in the patch descriptions?

> For %lld versus %llu - I think that cgroup resource APIs are much more
> likely to need to report unsigned rather than signed values. In the
> case of the memory.stat file, that's certainly the case.
> 
> But I guess there's an argument to be made that nothing's likely to
> need the final 64th bit of an unsigned value, whereas the ability to
> report negative numbers could potentially be useful for some cgroups.
> 
> Paul

i don't have any strong opinions about signedness.

YAMAMOTO Takashi

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[YAMAMOTO Takashi]

> On Feb 19, 2008 9:48 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
> >
> > it changes the format from "%s %lld" to "%s: %llu", right?
> > why?
> >
> 
> The colon for consistency with maps in /proc. I think it also makes it
> slightly more readable.

can you be a little more specific?

i object against the colon because i want to use the same parser for
/proc/vmstat, which doesn't have colons.

btw, when making ABI changes like this, can you please mention it
explicitly in the patch descriptions?

> For %lld versus %llu - I think that cgroup resource APIs are much more
> likely to need to report unsigned rather than signed values. In the
> case of the memory.stat file, that's certainly the case.
> 
> But I guess there's an argument to be made that nothing's likely to
> need the final 64th bit of an unsigned value, whereas the ability to
> report negative numbers could potentially be useful for some cgroups.
> 
> Paul

i don't have any strong opinions about signedness.

YAMAMOTO Takashi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[Paul Menage]

On Feb 19, 2008 10:14 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
> > On Feb 19, 2008 9:48 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
> > >
> > > it changes the format from "%s %lld" to "%s: %llu", right?
> > > why?
> > >
> >
> > The colon for consistency with maps in /proc. I think it also makes it
> > slightly more readable.
>
> can you be a little more specific?
>
> i object against the colon because i want to use the same parser for
> /proc/vmstat, which doesn't have colons.

Ah. This /proc behaviour of having multiple formats for reporting the
same kind of data (compare with /proc/meminfo, which does use colons)
is the kind of thing that I want to avoid with cgroups. i.e. if two
cgroup subsystems are both reporting the same kind of structured data,
then they should both use the same output format.

I guess since /proc has both styles, and memory.stat is the first file
reporting key/value pairs in cgroups, you get to call the format. OK,
I'll zap the colon.

Paul

Re: [PATCH 0/2] cgroup map files: Add a key/value map file type to cgroups

[Paul Menage]

On Feb 19, 2008 10:14 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
> > On Feb 19, 2008 9:48 PM, YAMAMOTO Takashi <yamamoto@valinux.co.jp> wrote:
> > >
> > > it changes the format from "%s %lld" to "%s: %llu", right?
> > > why?
> > >
> >
> > The colon for consistency with maps in /proc. I think it also makes it
> > slightly more readable.
>
> can you be a little more specific?
>
> i object against the colon because i want to use the same parser for
> /proc/vmstat, which doesn't have colons.

Ah. This /proc behaviour of having multiple formats for reporting the
same kind of data (compare with /proc/meminfo, which does use colons)
is the kind of thing that I want to avoid with cgroups. i.e. if two
cgroup subsystems are both reporting the same kind of structured data,
then they should both use the same output format.

I guess since /proc has both styles, and memory.stat is the first file
reporting key/value pairs in cgroups, you get to call the format. OK,
I'll zap the colon.

Paul

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>