per-task delay accounting taskstats interface: control exit data through cpumasks]

include/linux/taskstats.h +26 -0

+	if (!cpus_subset(mask, cpu_possible_map))
+		return -EINVAL;
+
+	if (isadd == REGISTER) {
+		for_each_cpu_mask(cpu, mask) {
+			s = kmalloc_node(sizeof(struct listener), GFP_KERNEL,
+					 cpu_to_node(cpu));
+			if (!s)
+				return -ENOMEM;
+			s->pid = pid;
+			INIT_LIST_HEAD(&s->list);
+
+			listeners = &per_cpu(listener_array, cpu);
+			down_write(&listeners->sem);
+			list_add(&s->list, &listeners->list);
+			up_write(&listeners->sem);
+		}
+	} else {
+		for_each_cpu_mask(cpu, mask) {
+			struct list_head *tmp;
+
+			listeners = &per_cpu(listener_array, cpu);
+			down_write(&listeners->sem);
+			list_for_each_safe(p, tmp, &listeners->list) {
+				s = list_entry(p, struct listener, list);
+				if (s->pid == pid) {

include/linux/taskstats.h +27 -0

+		rc = add_del_listener(info->snd_pid, &mask, REGISTER);
+		return rc;
+	}
+
+	if (info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK]) {
+		na = info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK];
+		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
+			return -E2BIG;
+		cpulist_parse((char *)nla_data(na), mask);
+		rc = add_del_listener(info->snd_pid, &mask, DEREGISTER);
+		return rc;
+	}
 
...
 
+void taskstats_exit_alloc(struct taskstats **ptidstats, unsigned int *mycpu)
+{
+	struct listener_list *listeners;
+	struct taskstats *tmp;
+	/*
+	 * This is the cpu on which the task is exiting currently and will
+	 * be the one for which the exit event is sent, even if the cpu
+	 * on which this function is running changes later.
+	 */
+	*mycpu = raw_smp_processor_id();
+
+	*ptidstats = NULL;
+	tmp = kmem_cache_zalloc(taskstats_cache, SLAB_KERNEL);
+	if (!tmp)
+		return;

include/linux/taskstats.h

Quoted 21 lines · On Thu, 06 Jul 2006 05:28:35 -0400 Shailabh Nagar <[email protected]> wrote:

On systems with a large number of cpus, with even a modest rate of tasks exiting per cpu, the volume of taskstats data sent on thread exit can overflow a userspace listener's buffers.

One approach to avoiding overflow is to allow listeners to get data for a limited and specific set of cpus. By scaling the number of listeners and/or the cpus they monitor, userspace can handle the statistical data overload more gracefully.

In this patch, each listener registers to listen to a specific set of cpus by specifying a cpumask. The interest is recorded per-cpu. When a task exits on a cpu, its taskstats data is unicast to each listener interested in that cpu.

Thanks to Andrew Morton for pointing out the various scalability and general concerns of previous attempts and for suggesting this design.

...

include/linux/taskstats.h +0 -0

Your email client performs space-stuffing. Fortunately "sed -e 's/^ / /'" is easy.

include/linux/taskstats.h

Quoted 6 lines

  #include <linux/taskstats_kern.h>
  #include <linux/delayacct.h>

include/linux/taskstats.h +2 -0

+#include <linux/cpumask.h>
+#include <linux/percpu.h>
  #include <net/genetlink.h>
  #include <asm/atomic.h>

Like that.

include/linux/taskstats.h

Quoted 11 lines

include/linux/taskstats.h +9 -0

+static int add_del_listener(pid_t pid, cpumask_t *maskp, int isadd)
+{
+	struct listener *s;
+	struct listener_list *listeners;
+	unsigned int cpu;
+	cpumask_t mask;
+	struct list_head *p;
+
+	memcpy(&mask, maskp, sizeof(cpumask_t));

mask = *maskp; ?

Quoted 32 lines

include/linux/taskstats.h +31 -0

+	if (!cpus_subset(mask, cpu_possible_map))
+		return -EINVAL;
+
+	if (isadd == REGISTER) {
+		for_each_cpu_mask(cpu, mask) {
+			s = kmalloc_node(sizeof(struct listener), GFP_KERNEL,
+					 cpu_to_node(cpu));
+			if (!s)
+				return -ENOMEM;
+			s->pid = pid;
+			INIT_LIST_HEAD(&s->list);
+
+			listeners = &per_cpu(listener_array, cpu);
+			down_write(&listeners->sem);
+			list_add(&s->list, &listeners->list);
+			up_write(&listeners->sem);
+		}
+	} else {
+		for_each_cpu_mask(cpu, mask) {
+			struct list_head *tmp;
+
+			listeners = &per_cpu(listener_array, cpu);
+			down_write(&listeners->sem);
+			list_for_each_safe(p, tmp, &listeners->list) {
+				s = list_entry(p, struct listener, list);
+				if (s->pid == pid) {
+					list_del(&s->list);
+					kfree(s);
+					break;
+				}
+			}

include/linux/taskstats.h

Quoted 5 lines

include/linux/taskstats.h +5 -0

+			up_write(&listeners->sem);
+		}
+	}
+	return 0;
+}

You might choose to handle the ENOMEM situation here by backing out and not leaving things half-installed. I suspect that's just a simple `goto'.

include/linux/taskstats.h static int taskstats_send_stats(struct s · line 305

Quoted 11 lines

include/linux/taskstats.h +2 -1

-static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
+static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
  {
  	int rc = 0;
  	struct sk_buff *rep_skb;

@@ -210,6 +302,25 @@ static int taskstats_send_stats(struct s
  	void *reply;
  	size_t size;
  	struct nlattr *na;
+	cpumask_t mask;

When counting add_del_listener(), that's two cpumasks on the stack. How big can these get? 256 bytes? Is it possible to get by with just the one?

include/linux/taskstats.h static int taskstats_send_stats(struct s · line 311

Quoted 7 lines

include/linux/taskstats.h +6 -0

+
+	if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]) {
+		na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
+		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
+			return -E2BIG;
+		cpulist_parse((char *)nla_data(na), mask);

Best check the return value from this function.

Quoted 35 lines

include/linux/taskstats.h +31 -0

+		rc = add_del_listener(info->snd_pid, &mask, REGISTER);
+		return rc;
+	}
+
+	if (info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK]) {
+		na = info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK];
+		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
+			return -E2BIG;
+		cpulist_parse((char *)nla_data(na), mask);
+		rc = add_del_listener(info->snd_pid, &mask, DEREGISTER);
+		return rc;
+	}
 
...
 
+void taskstats_exit_alloc(struct taskstats **ptidstats, unsigned int *mycpu)
+{
+	struct listener_list *listeners;
+	struct taskstats *tmp;
+	/*
+	 * This is the cpu on which the task is exiting currently and will
+	 * be the one for which the exit event is sent, even if the cpu
+	 * on which this function is running changes later.
+	 */
+	*mycpu = raw_smp_processor_id();
+
+	*ptidstats = NULL;
+	tmp = kmem_cache_zalloc(taskstats_cache, SLAB_KERNEL);
+	if (!tmp)
+		return;
+
+	listeners = &per_cpu(listener_array, *mycpu);
+	down_read(&listeners->sem);
+	if (!list_empty(&listeners->list)) {

include/linux/taskstats.h

Quoted 6 lines

include/linux/taskstats.h +6 -0

+		*ptidstats = tmp;
+		tmp = NULL;
+	}
+	up_read(&listeners->sem);
+	if (tmp)
+		kfree(tmp);

kfree(NULL) is legal.

Looks good to me. Does it work?

* Shailabh Nagar <[email protected]> 2006-07-06 07:37

Review static struct nla_policy taskstats_cmd_g · line 54

Quoted 11 lines

(quoted diff) +6 -0

@@ -37,9 +45,26 @@ static struct nla_policy taskstats_cmd_g
 __read_mostly = {
 	[TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_U32 },
 	[TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
+	[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK] = { .type = NLA_STRING },
+	[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK] = { .type = NLA_STRING },};
 
+		na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
+		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
+			return -E2BIG;
+		rc = cpulist_parse((char *)nla_data(na), mask);

This isn't safe, the data in the attribute is not guaranteed to be NUL terminated. Still it's probably me to blame for not making this more obvious in the API.

I've attached a patch below extending the API to make it easier for interfaces using NUL termianted strings, so you'd do:

[TASKSTATS_CMS_ATTR_REGISTER_CPUMASK] = { .type = NLA_NUL_STRING,

                                          .len = TASKSTATS_CPUMASK_MAXLEN }

... and then use (char *) nla_data(str_attr) without any further sanity checks.

[NETLINK]: Improve string attribute validation

Introduces a new attribute type NLA_NUL_STRING to support NUL terminated strings. Attributes of this kind require to carry a terminating NUL within the maximum specified in the policy.

The `old' NLA_STRING which is not required to be NUL terminated is extended to provide means to specify a maximum length of the string.

Aims at easing the pain with using nla_strlcpy() on temporary buffers.

The old minlen' field is renamed to len' for cosmetic purposes which is ok since nobody was using it at this point.

Signed-off-by Thomas Graf <[email protected]>

* Shailabh Nagar <[email protected]> 2006-07-06 07:37

Quoted 6 lines

(quoted diff) +2 -0

@@ -37,9 +45,26 @@ static struct nla_policy taskstats_cmd_g
 __read_mostly = {
 	[TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_U32 },
 	[TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
+	[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK] = { .type = NLA_STRING },
+	[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK] = { .type = NLA_STRING },};

Review static struct nla_policy taskstats_cmd_g · line 53

Quoted 4 lines

(quoted diff) +4 -0

+		na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
+		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
+			return -E2BIG;
+		rc = cpulist_parse((char *)nla_data(na), mask);

This isn't safe, the data in the attribute is not guaranteed to be NUL terminated. Still it's probably me to blame for not making this more obvious in the API.

I've attached a patch below extending the API to make it easier for interfaces using NUL termianted strings,

Quoted 9 lines · Thomas Graf

* Shailabh Nagar <[email protected]> 2006-07-06 07:37

Quoted 6 lines

(quoted diff) +2 -0

@@ -37,9 +45,26 @@ static struct nla_policy taskstats_cmd_g
 __read_mostly = {
 	[TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_U32 },
 	[TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
+	[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK] = { .type = NLA_STRING },
+	[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK] = { .type = NLA_STRING },};

Quoted 9 lines

Review static struct nla_policy taskstats_cmd_g · line 53

Quoted 4 lines

(quoted diff) +4 -0

+		na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
+		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
+			return -E2BIG;
+		rc = cpulist_parse((char *)nla_data(na), mask);

This isn't safe, the data in the attribute is not guaranteed to be NUL terminated. Still it's probably me to blame for not making this more obvious in the API.

Thanks, that was an unpleasant bug.

Quoted 2 lines

I've attached a patch below extending the API to make it easier for interfaces using NUL termianted strings,

In the interests of keeping this work decoupled from netlink enhancements I'd propose the below.

Is it bad to modify the data at nla_data()?

kernel/taskstats.c renamed from kernel/taskstats.c~per-task-delay-accounting-taskstats-interface-control-exit-data-through-cpumasks-fix +25 -18

@@ -299,6 +299,23 @@ cleanup:
 	return 0;
 }
 
+static int parse(struct nlattr *na, cpumask_t *mask)
+{
+	char *data;
+	int len;
+
+	if (na == NULL)
+		return 1;
+	len = nla_len(na);
+	if (len > TASKSTATS_CPUMASK_MAXLEN)
+		return -E2BIG;
+	if (len < 1)
+		return -EINVAL;
+	data = nla_data(na);
+	data[len - 1] = '\0';
+	return cpulist_parse(data, *mask);
+}
+
 static int taskstats_user_cmd(struct sk_buff *skb, struct genl_info *info)
 {
 	int rc = 0;

@@ -309,27 +326,17 @@ static int taskstats_user_cmd(struct sk_
 	struct nlattr *na;
 	cpumask_t mask;
 
-	if (info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK]) {
-		na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
-		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
-			return -E2BIG;
-		rc = cpulist_parse((char *)nla_data(na), mask);
-		if (rc)
-			return rc;
-		rc = add_del_listener(info->snd_pid, &mask, REGISTER);
+	rc = parse(info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK], &mask);
+	if (rc < 0)
 		return rc;
-	}
+	if (rc == 0)
+		return add_del_listener(info->snd_pid, &mask, REGISTER);
 
-	if (info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK]) {
-		na = info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK];
-		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
-			return -E2BIG;
-		rc = cpulist_parse((char *)nla_data(na), mask);
-		if (rc)
-			return rc;
-		rc = add_del_listener(info->snd_pid, &mask, DEREGISTER);
+	rc = parse(info->attrs[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK], &mask);
+	if (rc < 0)
 		return rc;
-	}
+	if (rc == 0)
+		return add_del_listener(info->snd_pid, &mask, DEREGISTER);
 
 	/*
 	 * Size includes space for nested attributes

_

Quoted 29 lines · Andrew Morton

Quoted 9 lines · Thomas Graf

* Shailabh Nagar <[email protected]> 2006-07-06 07:37

Quoted 6 lines

(quoted diff) +2 -0

@@ -37,9 +45,26 @@ static struct nla_policy taskstats_cmd_g
 __read_mostly = {
 	[TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_U32 },
 	[TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
+	[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK] = { .type = NLA_STRING },
+	[TASKSTATS_CMD_ATTR_DEREGISTER_CPUMASK] = { .type = NLA_STRING },};

Quoted 9 lines

Review static struct nla_policy taskstats_cmd_g · line 53

Quoted 4 lines

(quoted diff) +4 -0

+		na = info->attrs[TASKSTATS_CMD_ATTR_REGISTER_CPUMASK];
+		if (nla_len(na) > TASKSTATS_CPUMASK_MAXLEN)
+			return -E2BIG;
+		rc = cpulist_parse((char *)nla_data(na), mask);

This isn't safe, the data in the attribute is not guaranteed to be NUL terminated. Still it's probably me to blame for not making this more obvious in the API.

Thanks, that was an unpleasant bug.

Quoted 2 lines

I've attached a patch below extending the API to make it easier for interfaces using NUL termianted strings,

Quoted 2 lines

In the interests of keeping this work decoupled from netlink enhancements I'd propose the below.

The patch looks good. I was also thinking of simply modifying the input to have the null termination and modify later when netlink provides generic support.

Yup. Thomas, what's the testing status of the netlink patch you sent? Should I queue it up and start plagueing people with it?

In the interests of keeping this work decoupled from netlink enhancements I'd propose the below. Is it bad to modify the data at nla_data()?

kernel/taskstats.c renamed from kernel/taskstats.c~per-task-delay-accounting-taskstats-interface-control-exit-data-through-cpumasks-fix +6 -0

@@ -299,6 +299,23 @@ cleanup:
 	return 0;
 }
 
+static int parse(struct nlattr *na, cpumask_t *mask)
+{
+	char *data;
+	int len;
+
+	if (na == NULL)

* Andrew Morton <[email protected]> 2006-07-06 14:48

Quoted 2 lines

In the interests of keeping this work decoupled from netlink enhancements I'd propose the below. Is it bad to modify the data at nla_data()?

Yes, it points into a skb data buffer which may be shared by sitting on other queues if the message is to be broadcasted. In this case it would be harmless but the policy is to leave it unmodified.

Quoted 17 lines

kernel/taskstats.c renamed from kernel/taskstats.c~per-task-delay-accounting-taskstats-interface-control-exit-data-through-cpumasks-fix +10 -0

@@ -299,6 +299,23 @@ cleanup:
 	return 0;
 }
 
+static int parse(struct nlattr *na, cpumask_t *mask)
+{
+	char *data;
+	int len;
+
+	if (na == NULL)
+		return 1;
+	len = nla_len(na);
+	if (len > TASKSTATS_CPUMASK_MAXLEN)
+		return -E2BIG;

kernel/taskstats.c cleanup: · line 317

Quoted 6 lines

kernel/taskstats.c renamed from kernel/taskstats.c~per-task-delay-accounting-taskstats-interface-control-exit-data-through-cpumasks-fix +6 -0

+	if (len < 1)
+		return -EINVAL;
+	data = nla_data(na);
+	data[len - 1] = '\0';
+	return cpulist_parse(data, *mask);
+}

Usually this is done by using strlcpy:

hm, nla_strlcpy() looks more complex than it needs to be. We really need nla_kstrndup() ;)

Oh well. This?

Your email client performs space-stuffing.