[0/1] [patch -mm] Add containerstats (v3)

[0/1] [patch -mm] Add containerstats (v3)

[Balbir Singh]

Changelog (v3)

1. Use the container iterator (Paul Menage)
2. delayacct_set_flag() has now been moved to delayacct_blkio_start
				(Andrew Morton)

Changelog (v2)

1. Instead of parsing long container path's use the dentry to match the
   container for which stats are required. The user space application
   opens the container directory and passes the file descriptor, which
   is used to determine the container for which stats are required.
   This approach was suggested by Paul Menage

This patch is inspired by the discussion at http://lkml.org/lkml/2007/4/11/187
and implements per container statistics as suggested by Andrew Morton
in http://lkml.org/lkml/2007/4/11/263. The patch is on top of 2.6.21-mm1
with Paul's containers v9 patches (forward ported)

This patch implements per container statistics infrastructure and re-uses
code from the taskstats interface. A new set of container operations are
registered with commands and attributes. It should be very easy to
*extend* per container statistics, by adding members to the containerstats
structure.

The current model for containerstats is a pull, a push model (to post
statistics on interesting events), should be very easy to add. Currently
user space requests for statistics by passing the container file descriptor.
Statistics about the state of all the tasks in the container is returned to
user space.

TODO's/NOTE:

This patch provides an infrastructure for implementing container statistics.
Based on the needs of each controller, we can incrementally add more statistics,
event based support for notification of statistics, accumulation of taskstats
into container statistics in the future.

Sample output

# ./containerstats -C /container/a
sleeping 2, blocked 0, running 1, stopped 0, uninterruptible 0

# ./containerstats -C /container/
sleeping 154, blocked 0, running 0, stopped 0, uninterruptible 0

If the approach looks good, I'll enhance and post the user space utility for
the same

Feedback, comments, test results are always welcome!



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

 Documentation/accounting/containerstats.txt |   27 ++++++++++
 include/linux/Kbuild                        |    1 
 include/linux/container.h                   |    8 +++
 include/linux/containerstats.h              |   70 ++++++++++++++++++++++++++++
 include/linux/delayacct.h                   |   13 +++++
 kernel/container.c                          |   54 +++++++++++++++++++++
 kernel/taskstats.c                          |   66 ++++++++++++++++++++++++++
 7 files changed, 239 insertions(+)

diff -puN /dev/null Documentation/accounting/containerstats.txt
--- /dev/null	2007-06-01 20:42:04.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/Documentation/accounting/containerstats.txt	2007-06-08 23:11:03.000000000 +0530
@@ -0,0 +1,27 @@
+Containerstats is inspired by the discussion at
+http://lkml.org/lkml/2007/4/11/187 and implements per container statistics as
+suggested by Andrew Morton in http://lkml.org/lkml/2007/4/11/263.
+
+Per container statistics infrastructure re-uses code from the taskstats
+interface. A new set of container operations are registered with commands
+and attributes specific to containers. It should be very easy to
+extend per container statistics, by adding members to the containerstats
+structure.
+
+The current model for containerstats is a pull, a push model (to post
+statistics on interesting events), should be very easy to add. Currently
+user space requests for statistics by passing the container path.
+Statistics about the state of all the tasks in the container is returned to
+user space.
+
+NOTE: We currently rely on delay accounting for extracting information
+about tasks blocked on I/O. If CONFIG_TASK_DELAY_ACCT is disabled, this
+information will not be available.
+
+To extract container statistics a utility very similar to getdelays.c
+has been developed, the sample output of the utility is shown below
+
+~/balbir/containerstats # ./getdelays  -C "/container/a"
+sleeping 1, blocked 0, running 1, stopped 0, uninterruptible 0
+~/balbir/containerstats # ./getdelays  -C "/container"
+sleeping 155, blocked 0, running 1, stopped 0, uninterruptible 2
diff -puN include/linux/container.h~containers-taskstats include/linux/container.h
--- linux-2.6.22-rc24-mm2/include/linux/container.h~containers-taskstats	2007-06-08 23:11:03.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/include/linux/container.h	2007-06-08 23:11:25.000000000 +0530
@@ -13,6 +13,7 @@
 #include <linux/cpumask.h>
 #include <linux/nodemask.h>
 #include <linux/rcupdate.h>
+#include <linux/containerstats.h>
 
 #ifdef CONFIG_CONTAINERS
 
@@ -27,6 +28,8 @@ extern void container_unlock(void);
 extern void container_fork(struct task_struct *p);
 extern void container_fork_callbacks(struct task_struct *p);
 extern void container_exit(struct task_struct *p, int run_callbacks);
+extern int containerstats_build(struct containerstats *stats,
+				struct dentry *dentry);
 
 extern struct file_operations proc_container_operations;
 
@@ -288,6 +291,11 @@ static inline void container_exit(struct
 
 static inline void container_lock(void) {}
 static inline void container_unlock(void) {}
+static inline int containerstats_build(struct containerstats *stats,
+					struct dentry *dentry)
+{
+	return -EINVAL;
+}
 
 #endif /* !CONFIG_CONTAINERS */
 
diff -puN /dev/null include/linux/containerstats.h
--- /dev/null	2007-06-01 20:42:04.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/include/linux/containerstats.h	2007-06-08 23:11:03.000000000 +0530
@@ -0,0 +1,70 @@
+/* containerstats.h - exporting per-container statistics
+ *
+ * Copyright IBM Corporation, 2007
+ * Author Balbir Singh <balbir@linux.vnet.ibm.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of version 2.1 of the GNU Lesser General Public License
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ */
+
+#ifndef _LINUX_CONTAINERSTATS_H
+#define _LINUX_CONTAINERSTATS_H
+
+#include <linux/taskstats.h>
+
+/*
+ * Data shared between user space and kernel space on a per container
+ * basis. This data is shared using taskstats.
+ *
+ * Most of these states are derived by looking at the task->state value
+ * For the nr_io_wait state, a flag in the delay accounting structure
+ * indicates that the task is waiting on IO
+ *
+ * Each member is aligned to a 8 byte boundary.
+ */
+struct containerstats {
+	__u64	nr_sleeping;		/* Number of tasks sleeping */
+	__u64	nr_running;		/* Number of tasks running */
+	__u64	nr_stopped;		/* Number of tasks in stopped state */
+	__u64	nr_uninterruptible;	/* Number of tasks in uninterruptible */
+					/* state */
+	__u64	nr_io_wait;		/* Number of tasks waiting on IO */
+};
+
+/*
+ * Commands sent from userspace
+ * Not versioned. New commands should only be inserted at the enum's end
+ * prior to __CONTAINERSTATS_CMD_MAX
+ */
+
+enum {
+	CONTAINERSTATS_CMD_UNSPEC = __TASKSTATS_CMD_MAX,	/* Reserved */
+	CONTAINERSTATS_CMD_GET,		/* user->kernel request/get-response */
+	CONTAINERSTATS_CMD_NEW,		/* kernel->user event */
+	__CONTAINERSTATS_CMD_MAX,
+};
+
+#define CONTAINERSTATS_CMD_MAX (__CONTAINERSTATS_CMD_MAX - 1)
+
+enum {
+	CONTAINERSTATS_TYPE_UNSPEC = 0,	/* Reserved */
+	CONTAINERSTATS_TYPE_CONTAINER_STATS,	/* contains name + stats */
+	__CONTAINERSTATS_TYPE_MAX,
+};
+
+#define CONTAINERSTATS_TYPE_MAX (__CONTAINERSTATS_TYPE_MAX - 1)
+
+enum {
+	CONTAINERSTATS_CMD_ATTR_UNSPEC = 0,
+	CONTAINERSTATS_CMD_ATTR_FD,
+	__CONTAINERSTATS_CMD_ATTR_MAX,
+};
+
+#define CONTAINERSTATS_CMD_ATTR_MAX (__CONTAINERSTATS_CMD_ATTR_MAX - 1)
+
+#endif /* _LINUX_CONTAINERSTATS_H */
diff -puN include/linux/delayacct.h~containers-taskstats include/linux/delayacct.h
--- linux-2.6.22-rc24-mm2/include/linux/delayacct.h~containers-taskstats	2007-06-08 23:11:03.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/include/linux/delayacct.h	2007-06-08 23:11:03.000000000 +0530
@@ -26,6 +26,7 @@
  * Used to set current->delays->flags
  */
 #define DELAYACCT_PF_SWAPIN	0x00000001	/* I am doing a swapin */
+#define DELAYACCT_PF_BLKIO	0x00000002	/* I am waiting on IO */
 
 #ifdef CONFIG_TASK_DELAY_ACCT
 
@@ -39,6 +40,14 @@ extern void __delayacct_blkio_end(void);
 extern int __delayacct_add_tsk(struct taskstats *, struct task_struct *);
 extern __u64 __delayacct_blkio_ticks(struct task_struct *);
 
+static inline int delayacct_is_task_waiting_on_io(struct task_struct *p)
+{
+	if (p->delays)
+		return (p->delays->flags & DELAYACCT_PF_BLKIO);
+	else
+		return 0;
+}
+
 static inline void delayacct_set_flag(int flag)
 {
 	if (current->delays)
@@ -71,6 +80,7 @@ static inline void delayacct_tsk_free(st
 
 static inline void delayacct_blkio_start(void)
 {
+	delayacct_set_flag(DELAYACCT_PF_BLKIO);
 	if (current->delays)
 		__delayacct_blkio_start();
 }
@@ -79,6 +89,7 @@ static inline void delayacct_blkio_end(v
 {
 	if (current->delays)
 		__delayacct_blkio_end();
+	delayacct_clear_flag(DELAYACCT_PF_BLKIO);
 }
 
 static inline int delayacct_add_tsk(struct taskstats *d,
@@ -116,6 +127,8 @@ static inline int delayacct_add_tsk(stru
 { return 0; }
 static inline __u64 delayacct_blkio_ticks(struct task_struct *tsk)
 { return 0; }
+static inline int delayacct_is_task_waiting_on_io(struct task_struct *p)
+{ return 0; }
 #endif /* CONFIG_TASK_DELAY_ACCT */
 
 #endif
diff -puN include/linux/Kbuild~containers-taskstats include/linux/Kbuild
--- linux-2.6.22-rc24-mm2/include/linux/Kbuild~containers-taskstats	2007-06-08 23:11:03.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/include/linux/Kbuild	2007-06-08 23:11:03.000000000 +0530
@@ -47,6 +47,7 @@ header-y += coff.h
 header-y += comstats.h
 header-y += consolemap.h
 header-y += const.h
+header-y += containerstats.h
 header-y += cycx_cfm.h
 header-y += dlm_device.h
 header-y += dlm_netlink.h
diff -puN kernel/container.c~containers-taskstats kernel/container.c
--- linux-2.6.22-rc24-mm2/kernel/container.c~containers-taskstats	2007-06-08 23:11:03.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/kernel/container.c	2007-06-08 23:11:44.000000000 +0530
@@ -58,6 +58,8 @@
 #include <linux/time.h>
 #include <linux/backing-dev.h>
 #include <linux/sort.h>
+#include <linux/delayacct.h>
+#include <linux/containerstats.h>
 
 #include <asm/atomic.h>
 
@@ -1603,6 +1605,58 @@ static int pid_array_load(pid_t *pidarra
 	return n;
 }
 
+/**
+ * Build and fill containerstats so that taskstats can export it to user
+ * space.
+ *
+ * @stats: containerstats to fill information into
+ * @dentry: A dentry entry belonging to the container for which stats have
+ * been requested.
+ */
+int containerstats_build(struct containerstats *stats, struct dentry *dentry)
+{
+	int ret = -EINVAL;
+	struct container *cont;
+	struct container_iter it;
+	struct task_struct *tsk;
+	/*
+	 * Validate dentry by checking the superblock operations
+	 */
+	if (dentry->d_sb->s_op != &container_ops)
+		 goto err;
+
+	ret = 0;
+	cont = dentry->d_fsdata;
+	rcu_read_lock();
+
+	container_iter_start(cont, &it);
+	while ((tsk = container_iter_next(cont, &it))) {
+		switch (tsk->state) {
+		case TASK_RUNNING:
+			stats->nr_running++;
+			break;
+		case TASK_INTERRUPTIBLE:
+			stats->nr_sleeping++;
+			break;
+		case TASK_UNINTERRUPTIBLE:
+			stats->nr_uninterruptible++;
+			break;
+		case TASK_STOPPED:
+			stats->nr_stopped++;
+			break;
+		default:
+			if (delayacct_is_task_waiting_on_io(tsk))
+				stats->nr_io_wait++;
+			break;
+		}
+	}
+	container_iter_end(cont, &it);
+
+	rcu_read_unlock();
+err:
+	return ret;
+}
+
 static int cmppid(const void *a, const void *b)
 {
 	return *(pid_t *)a - *(pid_t *)b;
diff -puN kernel/sched.c~containers-taskstats kernel/sched.c
diff -puN kernel/taskstats.c~containers-taskstats kernel/taskstats.c
--- linux-2.6.22-rc24-mm2/kernel/taskstats.c~containers-taskstats	2007-06-08 23:11:03.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/kernel/taskstats.c	2007-06-08 23:11:03.000000000 +0530
@@ -23,6 +23,9 @@
 #include <linux/tsacct_kern.h>
 #include <linux/cpumask.h>
 #include <linux/percpu.h>
+#include <linux/containerstats.h>
+#include <linux/container.h>
+#include <linux/file.h>
 #include <net/genetlink.h>
 #include <asm/atomic.h>
 
@@ -50,6 +53,11 @@ __read_mostly = {
 	[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK] = { .type = NLA_STRING },
 	[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK] = { .type = NLA_STRING },};
 
+static struct nla_policy
+containerstats_cmd_get_policy[CONTAINERSTATS_CMD_ATTR_MAX+1] __read_mostly = {
+	[CONTAINERSTATS_CMD_ATTR_FD] = { .type = NLA_U32 },
+};
+
 struct listener {
 	struct list_head list;
 	pid_t pid;
@@ -373,6 +381,51 @@ err:
 	return NULL;
 }
 
+static int containerstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
+{
+	int rc = 0;
+	struct sk_buff *rep_skb;
+	struct containerstats *stats;
+	struct nlattr *na;
+	size_t size;
+	u32 fd;
+	struct file *file;
+	int fput_needed;
+
+	na = info->attrs[CONTAINERSTATS_CMD_ATTR_FD];
+	if (!na)
+		return -EINVAL;
+
+	fd = nla_get_u32(info->attrs[CONTAINERSTATS_CMD_ATTR_FD]);
+	file = fget_light(fd, &fput_needed);
+	if (file) {
+		size = nla_total_size(sizeof(struct containerstats));
+
+		rc = prepare_reply(info, CONTAINERSTATS_CMD_NEW, &rep_skb,
+					size);
+		if (rc < 0)
+			goto err;
+
+		na = nla_reserve(rep_skb, CONTAINERSTATS_TYPE_CONTAINER_STATS,
+					sizeof(struct containerstats));
+		stats = nla_data(na);
+		memset(stats, 0, sizeof(*stats));
+
+		rc = containerstats_build(stats, file->f_dentry);
+		if (rc < 0)
+			goto err;
+
+		fput_light(file, fput_needed);
+		return send_reply(rep_skb, info->snd_pid);
+	}
+
+err:
+	if (file)
+		fput_light(file, fput_needed);
+	nlmsg_free(rep_skb);
+	return rc;
+}
+
 static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
 {
 	int rc = 0;
@@ -523,6 +576,12 @@ static struct genl_ops taskstats_ops = {
 	.policy		= taskstats_cmd_get_policy,
 };
 
+static struct genl_ops containerstats_ops = {
+	.cmd		= CONTAINERSTATS_CMD_GET,
+	.doit		= containerstats_user_cmd,
+	.policy		= containerstats_cmd_get_policy,
+};
+
 /* Needed early in initialization */
 void __init taskstats_init_early(void)
 {
@@ -547,8 +606,15 @@ static int __init taskstats_init(void)
 	if (rc < 0)
 		goto err;
 
+	rc = genl_register_ops(&family, &containerstats_ops);
+	if (rc < 0)
+		goto err_container_ops;
+
 	family_registered = 1;
+	printk("registered taskstats version %d\n", TASKSTATS_GENL_VERSION);
 	return 0;
+err_container_ops:
+	genl_unregister_ops(&family, &taskstats_ops);
 err:
 	genl_unregister_family(&family);
 	return rc;
diff -puN kernel/cpuset.c~containers-taskstats kernel/cpuset.c
diff -puN kernel/delayacct.c~containers-taskstats kernel/delayacct.c
_

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

[1/1] [PATCH -mm] Update getdelays to become containerstats aware

[Balbir Singh]

Update the getdelays utility to become containerstats aware. A new -C option
has been added. It takes in a container path and prints out a summary of
task states in the container.


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

 Documentation/accounting/getdelays.c |   43 ++++++++++++++++++++++++++++++++++-
 1 file changed, 42 insertions(+), 1 deletion(-)

diff -puN Documentation/accounting/getdelays.c~containerstats-update-getdelays Documentation/accounting/getdelays.c
--- linux-2.6.22-rc24-mm2/Documentation/accounting/getdelays.c~containerstats-update-getdelays	2007-06-08 23:11:56.000000000 +0530
+++ linux-2.6.22-rc24-mm2-balbir/Documentation/accounting/getdelays.c	2007-06-08 23:12:47.000000000 +0530
@@ -26,6 +26,7 @@
 
 #include <linux/genetlink.h>
 #include <linux/taskstats.h>
+#include <linux/containerstats.h>
 
 /*
  * Generic macros for dealing with netlink sockets. Might be duplicated
@@ -79,6 +80,7 @@ static void usage(void)
 	fprintf(stderr, "  -i: print IO accounting (works only with -p)\n");
 	fprintf(stderr, "  -l: listen forever\n");
 	fprintf(stderr, "  -v: debug on\n");
+	fprintf(stderr, "  -C: container path\n");
 }
 
 /*
@@ -213,6 +215,14 @@ void task_context_switch_counts(struct t
 	       t->nvcsw, t->nivcsw);
 }
 
+void print_containerstats(struct containerstats *c)
+{
+	printf("sleeping %llu, blocked %llu, running %llu, stopped %llu, "
+		"uninterruptible %llu\n", c->nr_sleeping, c->nr_io_wait,
+		c->nr_running, c->nr_stopped, c->nr_uninterruptible);
+}
+
+
 void print_ioacct(struct taskstats *t)
 {
 	printf("%s: read=%llu, write=%llu, cancelled_write=%llu\n",
@@ -240,11 +250,14 @@ int main(int argc, char *argv[])
 	int maskset = 0;
 	char *logfile = NULL;
 	int loop = 0;
+	int containerset = 0;
+	char containerpath[1024];
+	int cfd = 0;
 
 	struct msgtemplate msg;
 
 	while (1) {
-		c = getopt(argc, argv, "qdiw:r:m:t:p:vl");
+		c = getopt(argc, argv, "qdiw:r:m:t:p:vlC:");
 		if (c < 0)
 			break;
 
@@ -261,6 +274,10 @@ int main(int argc, char *argv[])
 			printf("printing task/process context switch rates\n");
 			print_task_context_switch_counts = 1;
 			break;
+		case 'C':
+			containerset = 1;
+			strncpy(containerpath, optarg, strlen(optarg) + 1);
+			break;
 		case 'w':
 			logfile = strdup(optarg);
 			printf("write to file %s\n", logfile);
@@ -335,6 +352,11 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (tid && containerset) {
+		fprintf(stderr, "Select either -t or -C, not both\n");
+		goto err;
+	}
+
 	if (tid) {
 		rc = send_cmd(nl_sd, id, mypid, TASKSTATS_CMD_GET,
 			      cmd_type, &tid, sizeof(__u32));
@@ -345,6 +367,20 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (containerset) {
+		cfd = open(containerpath, O_RDONLY);
+		if (cfd < 0) {
+			perror("error opening container file");
+			goto err;
+		}
+		rc = send_cmd(nl_sd, id, mypid, CONTAINERSTATS_CMD_GET,
+			      CONTAINERSTATS_CMD_ATTR_FD, &cfd, sizeof(__u32));
+		if (rc < 0) {
+			perror("error sending containerstats command");
+			goto err;
+		}
+	}
+
 	do {
 		int i;
 
@@ -423,6 +459,9 @@ int main(int argc, char *argv[])
 				}
 				break;
 
+			case CONTAINERSTATS_TYPE_CONTAINER_STATS:
+				print_containerstats(NLA_DATA(na));
+				break;
 			default:
 				fprintf(stderr, "Unknown nla_type %d\n",
 					na->nla_type);
@@ -444,5 +483,7 @@ err:
 	close(nl_sd);
 	if (fd)
 		close(fd);
+	if (cfd)
+		close(cfd);
 	return 0;
 }
_

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

Re: [0/1] [patch -mm] Add containerstats (v3)

[Balbir Singh]

Andrew Morton wrote:
> On Fri, 08 Jun 2007 23:43:46 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> 
>> This patch implements per container statistics infrastructure and re-uses
>> code from the taskstats interface.
> 
> boggle.
> 
> Symbol: CONTAINERS [=y]
>   Selected by: CONTAINER_DEBUG || CPUSETS && SMP || CONTAINER_CPUACCT
> 
> Paul, that's just bizarre.  How come it was done this way?
> 
> <struggles for a while, works out how to make CONFIG_CONTAINERS go away>
> 
> OK, so taskstats.c still compiles, but I'm surprised.  Shouldn't all that
> newly-added container stuff in taskstats.c be inside CONFIG_CONTAINERS?

Hi, Andrew,

I've added a definition of containerstats_build() even when CONFIG_CONTAINERS
is turned off, it simply returns -EINVAL. That's why taskstats.c compiles,
I could add an #ifdef and move the containers commands under CONFIG_CONTAINERS,
but I felt taskstats.c could be clean without any #ifdef's hanging around
(as much as possible).

In the next iteration, I could move out all the code to containerstats.c
and make containerstats.c depend on CONFIG_CONTAINERS

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

Re: [0/1] [patch -mm] Add containerstats (v3)

[Paul Menage]

On 6/8/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Fri, 08 Jun 2007 23:43:46 +0530
> Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
>
> > This patch implements per container statistics infrastructure and re-uses
> > code from the taskstats interface.
>
> boggle.
>
> Symbol: CONTAINERS [=y]
>   Selected by: CONTAINER_DEBUG || CPUSETS && SMP || CONTAINER_CPUACCT
>
> Paul, that's just bizarre.  How come it was done this way?

Containers on their own without any subsystems aren't hugely useful.
So the plan was that any container subsystem would "select CONTAINERS"
to cause the container framework to be built. I'm happy to change it
to invert the dependencies if you'd prefer.

Paul

Re: [0/1] [patch -mm] Add containerstats (v3)

[Andrew Morton]

On Sat, 9 Jun 2007 00:09:55 -0700 "Paul Menage" <menage@google.com> wrote:

> On 6/8/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> > On Fri, 08 Jun 2007 23:43:46 +0530
> > Balbir Singh <balbir@linux.vnet.ibm.com> wrote:
> >
> > > This patch implements per container statistics infrastructure and re-uses
> > > code from the taskstats interface.
> >
> > boggle.
> >
> > Symbol: CONTAINERS [=y]
> >   Selected by: CONTAINER_DEBUG || CPUSETS && SMP || CONTAINER_CPUACCT
> >
> > Paul, that's just bizarre.  How come it was done this way?
> 
> Containers on their own without any subsystems aren't hugely useful.
> So the plan was that any container subsystem would "select CONTAINERS"
> to cause the container framework to be built. I'm happy to change it
> to invert the dependencies if you'd prefer.
> 

- CONTAINER_DEBUG should depend on CONTAINERS

- the CPUSETS && SMP is weird and should be deleted, unless I'm missing
  something

- CONTAINERS should depend on CPUSETS

- That leaves CONTAINER_CPUACCT.  One _could_ `select' CONTAINERS if
  CONTAINER_CPUACCT but I suspect that'll screw up the above (select does
  odd things).  I think it's reasonable to have a CONTAINERS menu and under
  that, all the clients of it: CONTAINER_CPUACCT, CONTAINER_MEMORY_CONTROL,
  etc.

Re: [0/1] [patch -mm] Add containerstats (v3)

[Paul Menage]

On 6/9/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> - CONTAINER_DEBUG should depend on CONTAINERS

CONTAINER_DEBUG is actually a container subsystem whose sole purpose
is to provide debugging information about any hierarchy that it's
mounted as a part of. So in some senses it's in the same boat as
something like cpusets or the RSS controller. CONFIG_CONTAINER_DEBUG
doesn't affect any of the container framework code.

>
> - the CPUSETS && SMP is weird and should be deleted, unless I'm missing
>   something

Cpusets depends on SMP in the vanilla tree, so that's not anything new
that I added.

>
> - CONTAINERS should depend on CPUSETS

You mean the other way around?

>
> - That leaves CONTAINER_CPUACCT.

Really, CONTAINER_CPUACCT should have the same relationship to
CONTAINERS as CPUSETS does.

Paul

Re: [0/1] [patch -mm] Add containerstats (v3)

[Andrew Morton]

On Sat, 9 Jun 2007 01:07:53 -0700 "Paul Menage" <menage@google.com> wrote:

> On 6/9/07, Andrew Morton <akpm@linux-foundation.org> wrote:
> >
> > - CONTAINER_DEBUG should depend on CONTAINERS
> 
> CONTAINER_DEBUG is actually a container subsystem whose sole purpose
> is to provide debugging information about any hierarchy that it's
> mounted as a part of. So in some senses it's in the same boat as
> something like cpusets or the RSS controller. CONFIG_CONTAINER_DEBUG
> doesn't affect any of the container framework code.

Oh, that's right.

But it still should depend on CONTAINERS

> >
> > - the CPUSETS && SMP is weird and should be deleted, unless I'm missing
> >   something
> 
> Cpusets depends on SMP in the vanilla tree, so that's not anything new
> that I added.

Oh, OK, so CPUSETS is nor a client of CONTAINERS: so it depneds on CONTAINERS

> >
> > - CONTAINERS should depend on CPUSETS
> 
> You mean the other way around?

yup

> >
> > - That leaves CONTAINER_CPUACCT.
> 
> Really, CONTAINER_CPUACCT should have the same relationship to
> CONTAINERS as CPUSETS does.
> 

Would it not be simplest to have CONTAINERS as the top-level
user-configurable item and to then have everything else depend on it?

select is a nasty thing - we repeatedly have problems when using it.

Re: [0/1] [patch -mm] Add containerstats (v3)

[Paul Menage]

On 6/9/07, Andrew Morton <akpm@linux-foundation.org> wrote:
>
> Would it not be simplest to have CONTAINERS as the top-level
> user-configurable item and to then have everything else depend on it?
>

Yes, OK - it can go that way around too. I guess my thought was that
people would be more interested in enabling/disabling the subsystems
themselves (cpusets, RSS, etc) than the underlying framework. But if
select has problems then it's no trouble to invert it.

Paul