[Patch 4/8] generic netlink utility functions

[Patch 4/8] generic netlink utility functions

[Shailabh Nagar]

genetlink-utils.patch

Two utilities for simplifying usage of NETLINK_GENERIC
interface.

Signed-off-by: Balbir Singh <balbir@in.ibm.com>
Signed-off-by: Shailabh Nagar <nagar@watson.ibm.com>

 include/net/genetlink.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+)

Index: linux-2.6.16/include/net/genetlink.h
===================================================================
--- linux-2.6.16.orig/include/net/genetlink.h	2006-03-29 18:12:54.000000000 -0500
+++ linux-2.6.16/include/net/genetlink.h	2006-03-29 18:13:17.000000000 -0500
@@ -150,4 +150,24 @@ static inline int genlmsg_unicast(struct
 	return nlmsg_unicast(genl_sock, skb, pid);
 }

+/**
+ * gennlmsg_data - head of message payload
+ * @gnlh: genetlink messsage header
+ */
+static inline void *genlmsg_data(const struct genlmsghdr *gnlh)
+{
+       return ((unsigned char *) gnlh + GENL_HDRLEN);
+}
+
+/**
+ * genlmsg_len - length of message payload
+ * @gnlh: genetlink message header
+ */
+static inline int genlmsg_len(const struct genlmsghdr *gnlh)
+{
+       struct nlmsghdr *nlh = (struct nlmsghdr *)((unsigned char *)gnlh -
+                                                   NLMSG_HDRLEN);
+       return (nlh->nlmsg_len - GENL_HDRLEN - NLMSG_HDRLEN);
+}
+
 #endif	/* __NET_GENERIC_NETLINK_H */

[Patch 5/8] generic netlink interface for delay accounting

[Shailabh Nagar]

delayacct-genetlink.patch

Create a generic netlink interface (NETLINK_GENERIC family),
called "taskstats", for getting delay and cpu statistics of
tasks and thread groups during their lifetime and when they exit.


Signed-off-by: Shailabh Nagar <nagar@us.ibm.com>
Signed-off-by: Balbir Singh <balbir@in.ibm.com>

 include/linux/delayacct.h |   11 +
 include/linux/taskstats.h |  113 +++++++++++++++++
 init/Kconfig              |   16 ++
 kernel/Makefile           |    1
 kernel/delayacct.c        |   44 ++++++
 kernel/taskstats.c        |  292 ++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 474 insertions(+), 3 deletions(-)

Index: linux-2.6.16/include/linux/delayacct.h
===================================================================
--- linux-2.6.16.orig/include/linux/delayacct.h	2006-03-29 18:13:13.000000000 -0500
+++ linux-2.6.16/include/linux/delayacct.h	2006-03-29 18:13:18.000000000 -0500
@@ -18,6 +18,7 @@
 #define _LINUX_TASKDELAYS_H

 #include <linux/sched.h>
+#include <linux/taskstats.h>

 #ifdef CONFIG_TASK_DELAY_ACCT
 extern int delayacct_on;	/* Delay accounting turned on/off */
@@ -27,6 +28,7 @@ extern void __delayacct_tsk_init(struct
 extern void __delayacct_tsk_exit(struct task_struct *);
 extern void __delayacct_blkio_start(void);
 extern void __delayacct_blkio_end(void);
+extern int __delayacct_add_tsk(struct taskstats *, struct task_struct *);

 static inline void delayacct_tsk_init(struct task_struct *tsk)
 {
@@ -64,4 +66,13 @@ static inline void delayacct_blkio_start
 static inline void delayacct_blkio_end(void)
 {}
 #endif /* CONFIG_TASK_DELAY_ACCT */
+#ifdef CONFIG_TASKSTATS
+static inline int delayacct_add_tsk(struct taskstats *d,
+				    struct task_struct *tsk)
+{
+	if (!tsk->delays)
+		return -EINVAL;
+	return __delayacct_add_tsk(d, tsk);
+}
+#endif
 #endif /* _LINUX_TASKDELAYS_H */
Index: linux-2.6.16/include/linux/taskstats.h
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.16/include/linux/taskstats.h	2006-03-29 18:13:18.000000000 -0500
@@ -0,0 +1,113 @@
+/* taskstats.h - exporting per-task statistics
+ *
+ * Copyright (C) Shailabh Nagar, IBM Corp. 2006
+ *           (C) Balbir Singh,   IBM Corp. 2006
+ *
+ * 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_TASKSTATS_H
+#define _LINUX_TASKSTATS_H
+
+/* Format for per-task data returned to userland when
+ *	- a task exits
+ *	- listener requests stats for a task
+ *
+ * The struct is versioned. Newer versions should only add fields to
+ * the bottom of the struct to maintain backward compatibility.
+ *
+ * To create the next version, bump up the taskstats_version variable
+ * and delineate the start of newly added fields with a comment indicating
+ * the version number.
+ */
+
+#define TASKSTATS_VERSION	1
+#define TASKSTATS_NOPID	-1
+
+struct taskstats {
+	/* Maintain 64-bit alignment while extending */
+	/* Version 1 */
+
+	/* XXX_count is number of delay values recorded.
+	 * XXX_total is corresponding cumulative delay in nanoseconds
+	 */
+
+#define TASKSTATS_NOCPUSTATS	1
+	__u64	cpu_count;
+	__u64	cpu_delay_total;	/* wait, while runnable, for cpu */
+	__u64	blkio_count;
+	__u64	blkio_delay_total;	/* sync,block io completion wait*/
+	__u64	swapin_count;
+	__u64	swapin_delay_total;	/* swapin page fault wait*/
+
+	__u64	cpu_run_total;		/* cpu running time
+					 * no count available/provided */
+};
+
+
+#define TASKSTATS_LISTEN_GROUP	0x1
+
+/*
+ * Commands sent from userspace
+ * Not versioned. New commands should only be inserted at the enum's end
+ */
+
+enum {
+	TASKSTATS_CMD_UNSPEC = 0,	/* Reserved */
+	TASKSTATS_CMD_GET,		/* user->kernel request/get-response */
+	TASKSTATS_CMD_NEW,		/* kernel->user event */
+	__TASKSTATS_CMD_MAX,
+};
+
+#define TASKSTATS_CMD_MAX (__TASKSTATS_CMD_MAX - 1)
+
+enum {
+	TASKSTATS_TYPE_UNSPEC = 0,	/* Reserved */
+	TASKSTATS_TYPE_PID,		/* Process id */
+	TASKSTATS_TYPE_TGID,		/* Thread group id */
+	TASKSTATS_TYPE_STATS,		/* taskstats structure */
+	TASKSTATS_TYPE_AGGR_PID,	/* contains pid + stats */
+	TASKSTATS_TYPE_AGGR_TGID,	/* contains tgid + stats */
+	__TASKSTATS_TYPE_MAX,
+};
+
+#define TASKSTATS_TYPE_MAX (__TASKSTATS_TYPE_MAX - 1)
+
+enum {
+	TASKSTATS_CMD_ATTR_UNSPEC = 0,
+	TASKSTATS_CMD_ATTR_PID,
+	TASKSTATS_CMD_ATTR_TGID,
+	__TASKSTATS_CMD_ATTR_MAX,
+};
+
+#define TASKSTATS_CMD_ATTR_MAX (__TASKSTATS_CMD_ATTR_MAX - 1)
+
+/* NETLINK_GENERIC related info */
+
+#define TASKSTATS_GENL_NAME	"TASKSTATS"
+#define TASKSTATS_GENL_VERSION	0x1
+
+#ifdef __KERNEL__
+
+#include <linux/sched.h>
+
+enum {
+	TASKSTATS_MSG_UNICAST,		/* send data only to requester */
+	TASKSTATS_MSG_MULTICAST,	/* send data to a group */
+};
+
+#ifdef CONFIG_TASKSTATS
+extern void taskstats_exit_pid(struct task_struct *);
+#else
+static inline void taskstats_exit_pid(struct task_struct *tsk)
+{}
+#endif
+
+#endif /* __KERNEL__ */
+#endif /* _LINUX_TASKSTATS_H */
Index: linux-2.6.16/init/Kconfig
===================================================================
--- linux-2.6.16.orig/init/Kconfig	2006-03-29 18:12:57.000000000 -0500
+++ linux-2.6.16/init/Kconfig	2006-03-29 18:13:18.000000000 -0500
@@ -158,11 +158,21 @@ config TASK_DELAY_ACCT
 	  in pages. Such statistics can help in setting a task's priorities
 	  relative to other tasks for cpu, io, rss limits etc.

-	  Unlike BSD process accounting, this information is available
-	  continuously during the lifetime of a task.
-
 	  Say N if unsure.

+config TASKSTATS
+	bool "Export task/process statistics through netlink (EXPERIMENTAL)"
+	depends on TASK_DELAY_ACCT
+	default y
+	help
+	  Export selected statistics for tasks/processes through the
+	  generic netlink interface. Unlike BSD process accounting, the
+	  statistics are available during the lifetime of tasks/processes as
+	  responses to commands. Like BSD accounting, they are sent to user
+	  space on task exit.
+
+	  Say Y if unsure.
+
 config SYSCTL
 	bool "Sysctl support"
 	---help---
Index: linux-2.6.16/kernel/delayacct.c
===================================================================
--- linux-2.6.16.orig/kernel/delayacct.c	2006-03-29 18:13:13.000000000 -0500
+++ linux-2.6.16/kernel/delayacct.c	2006-03-29 18:13:18.000000000 -0500
@@ -1,6 +1,7 @@
 /* delayacct.c - per-task delay accounting
  *
  * Copyright (C) Shailabh Nagar, IBM Corp. 2006
+ * Copyright (C) Balbir Singh, IBM Corp. 2006
  *
  * This program is free software;  you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -18,9 +19,12 @@
 #include <linux/time.h>
 #include <linux/sysctl.h>
 #include <linux/delayacct.h>
+#include <linux/taskstats.h>
+#include <linux/mutex.h>

 int delayacct_on __read_mostly = 0;	/* Delay accounting turned on/off */
 kmem_cache_t *delayacct_cache;
+static DEFINE_MUTEX(delayacct_exit_mutex);

 static int __init delayacct_setup_enable(char *str)
 {
@@ -50,10 +54,16 @@ void __delayacct_tsk_init(struct task_st

 void __delayacct_tsk_exit(struct task_struct *tsk)
 {
+	/*
+	 * Protect against racing thread group exits
+	 */
+	mutex_lock(&delayacct_exit_mutex);
+	taskstats_exit_pid(tsk);
 	if (tsk->delays) {
 		kmem_cache_free(delayacct_cache, tsk->delays);
 		tsk->delays = NULL;
 	}
+	mutex_unlock(&delayacct_exit_mutex);
 }

 /*
@@ -108,3 +118,37 @@ void __delayacct_blkio_end(void)
 			      &current->delays->blkio_delay,
 			      &current->delays->blkio_count);
 }
+#ifdef CONFIG_TASKSTATS
+int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
+{
+	nsec_t tmp;
+	struct timespec ts;
+	unsigned long t1,t2;
+
+	/* zero XXX_total,non-zero XXX_count implies XXX stat overflowed */
+
+	tmp = (nsec_t)d->cpu_run_total ;
+	tmp += (u64)(tsk->utime+tsk->stime)*TICK_NSEC;
+	d->cpu_run_total = (tmp < (nsec_t)d->cpu_run_total)? 0: tmp;
+
+	/* No locking available for sched_info. Take snapshot first. */
+	t1 = tsk->sched_info.pcnt;
+	t2 = tsk->sched_info.run_delay;
+
+	d->cpu_count += t1;
+
+	jiffies_to_timespec(t2, &ts);
+	tmp = (nsec_t)d->cpu_delay_total + timespec_to_ns(&ts);
+	d->cpu_delay_total = (tmp < (nsec_t)d->cpu_delay_total)? 0: tmp;
+
+	spin_lock(&tsk->delays->lock);
+	tmp = d->blkio_delay_total + tsk->delays->blkio_delay;
+	d->blkio_delay_total = (tmp < d->blkio_delay_total)? 0: tmp;
+	tmp = d->swapin_delay_total + tsk->delays->swapin_delay;
+	d->swapin_delay_total = (tmp < d->swapin_delay_total)? 0: tmp;
+	d->blkio_count += tsk->delays->blkio_count;
+	d->swapin_count += tsk->delays->swapin_count;
+	spin_unlock(&tsk->delays->lock);
+	return 0;
+}
+#endif /* CONFIG_TASKSTATS */
Index: linux-2.6.16/kernel/Makefile
===================================================================
--- linux-2.6.16.orig/kernel/Makefile	2006-03-29 18:12:57.000000000 -0500
+++ linux-2.6.16/kernel/Makefile	2006-03-29 18:13:18.000000000 -0500
@@ -35,6 +35,7 @@ obj-$(CONFIG_GENERIC_HARDIRQS) += irq/
 obj-$(CONFIG_SECCOMP) += seccomp.o
 obj-$(CONFIG_RCU_TORTURE_TEST) += rcutorture.o
 obj-$(CONFIG_TASK_DELAY_ACCT) += delayacct.o
+obj-$(CONFIG_TASKSTATS) += taskstats.o

 ifneq ($(CONFIG_SCHED_NO_NO_OMIT_FRAME_POINTER),y)
 # According to Alan Modra <alan@linuxcare.com.au>, the -fno-omit-frame-pointer is
Index: linux-2.6.16/kernel/taskstats.c
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ linux-2.6.16/kernel/taskstats.c	2006-03-29 18:13:18.000000000 -0500
@@ -0,0 +1,292 @@
+/*
+ * taskstats.c - Export per-task statistics to userland
+ *
+ * Copyright (C) Shailabh Nagar, IBM Corp. 2006
+ *           (C) Balbir Singh,   IBM Corp. 2006
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/taskstats.h>
+#include <linux/delayacct.h>
+#include <net/genetlink.h>
+#include <asm/atomic.h>
+
+const int taskstats_version = TASKSTATS_VERSION;
+static DEFINE_PER_CPU(__u32, taskstats_seqnum) = { 0 };
+static int family_registered = 0;
+
+static struct genl_family family = {
+	.id             = GENL_ID_GENERATE,
+	.name           = TASKSTATS_GENL_NAME,
+	.version        = TASKSTATS_GENL_VERSION,
+	.maxattr        = TASKSTATS_CMD_ATTR_MAX,
+};
+
+static struct nla_policy taskstats_cmd_get_policy[TASKSTATS_CMD_ATTR_MAX+1] __read_mostly = {
+	[TASKSTATS_CMD_ATTR_PID]  = { .type = NLA_U32 },
+	[TASKSTATS_CMD_ATTR_TGID] = { .type = NLA_U32 },
+};
+
+
+static int prepare_reply(struct genl_info *info, u8 cmd, struct sk_buff **skbp,
+			 void **replyp, size_t size)
+{
+	struct sk_buff *skb;
+	void *reply;
+
+	/*
+	 * If new attributes are added, please revisit this allocation
+	 */
+	skb = nlmsg_new(size);
+	if (!skb)
+		return -ENOMEM;
+
+	if (!info) {
+		int seq = get_cpu_var(taskstats_seqnum)++;
+		put_cpu_var(taskstats_seqnum);
+
+		reply = genlmsg_put(skb, 0, seq,
+				    family.id, 0, 0,
+				    cmd, family.version);
+	} else
+		reply = genlmsg_put(skb, info->snd_pid, info->snd_seq,
+				    family.id, 0, 0,
+				    cmd, family.version);
+	if (reply == NULL) {
+		nlmsg_free(skb);
+		return -EINVAL;
+	}
+
+	*skbp = skb;
+	*replyp = reply;
+	return 0;
+}
+
+static int send_reply(struct sk_buff *skb, pid_t pid, int event)
+{
+	struct genlmsghdr *genlhdr = nlmsg_data((struct nlmsghdr *)skb->data);
+	void *reply;
+	int rc;
+
+	reply = genlmsg_data(genlhdr);
+
+	rc = genlmsg_end(skb, reply);
+	if (rc < 0) {
+		nlmsg_free(skb);
+		return rc;
+	}
+
+	if (event == TASKSTATS_MSG_MULTICAST)
+		return genlmsg_multicast(skb, pid, TASKSTATS_LISTEN_GROUP);
+	return genlmsg_unicast(skb, pid);
+}
+
+static inline int fill_pid(pid_t pid, struct task_struct *pidtsk,
+			   struct taskstats *stats)
+{
+	int rc;
+	struct task_struct *tsk = pidtsk;
+
+	if (!pidtsk) {
+		read_lock(&tasklist_lock);
+		tsk = find_task_by_pid(pid);
+		if (!tsk) {
+			read_unlock(&tasklist_lock);
+			return -ESRCH;
+		}
+		get_task_struct(tsk);
+		read_unlock(&tasklist_lock);
+	} else
+		get_task_struct(tsk);
+
+	rc = delayacct_add_tsk(stats, tsk);
+	put_task_struct(tsk);
+
+	return rc;
+
+}
+
+static inline int fill_tgid(pid_t tgid, struct task_struct *tgidtsk,
+			    struct taskstats *stats)
+{
+	int rc;
+	struct task_struct *tsk, *first;
+
+	first = tgidtsk;
+	read_lock(&tasklist_lock);
+	if (!first) {
+		first = find_task_by_pid(tgid);
+		if (!first) {
+			read_unlock(&tasklist_lock);
+			return -ESRCH;
+		}
+	}
+	tsk = first;
+	do {
+		rc = delayacct_add_tsk(stats, tsk);
+		if (rc)
+			break;
+	} while_each_thread(first, tsk);
+	read_unlock(&tasklist_lock);
+
+	return rc;
+}
+
+static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
+{
+	int rc = 0;
+	struct sk_buff *rep_skb;
+	struct taskstats stats;
+	void *reply;
+	size_t size;
+	struct nlattr *na;
+
+	/*
+	 * Size includes space for nested attribute as well
+	 * The returned data is of the format
+	 * TASKSTATS_TYPE_AGGR_PID/TGID
+	 * --> TASKSTATS_TYPE_PID/TGID
+	 * --> TASKSTATS_TYPE_STATS
+	 */
+	size = nla_total_size(sizeof(u32)) +
+		nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
+
+	memset(&stats, 0, sizeof(stats));
+	rc = prepare_reply(info, TASKSTATS_CMD_NEW, &rep_skb, &reply, size);
+	if (rc < 0)
+		return rc;
+
+	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
+		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
+		rc = fill_pid((pid_t)pid, NULL, &stats);
+		if (rc < 0)
+			goto err;
+
+		na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_PID);
+		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, pid);
+	} else if (info->attrs[TASKSTATS_CMD_ATTR_TGID]) {
+		u32 tgid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_TGID]);
+		rc = fill_tgid((pid_t)tgid, NULL, &stats);
+		if (rc < 0)
+			goto err;
+
+		na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_TGID);
+		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, tgid);
+	} else {
+		rc = -EINVAL;
+		goto err;
+	}
+
+	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
+	nla_nest_end(rep_skb, na);
+
+	return send_reply(rep_skb, info->snd_pid, TASKSTATS_MSG_UNICAST);
+
+nla_put_failure:
+	return  genlmsg_cancel(rep_skb, reply);
+err:
+	nlmsg_free(rep_skb);
+	return rc;
+}
+
+
+/* Send pid data out on exit */
+void taskstats_exit_pid(struct task_struct *tsk)
+{
+	int rc = 0;
+	struct sk_buff *rep_skb;
+	void *reply;
+	struct taskstats stats;
+	size_t size;
+	int is_thread_group = !thread_group_empty(tsk);
+	struct nlattr *na;
+
+	/*
+	 * tasks can start to exit very early. Ensure that the family
+	 * is registered before notifications are sent out
+	 */
+	if (!family_registered)
+		return;
+
+	/*
+	 * Size includes space for nested attributes
+	 */
+	size = nla_total_size(sizeof(u32)) +
+		nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
+
+	if (is_thread_group)
+		size = 2 * size;	// PID + STATS + TGID + STATS
+
+	memset(&stats, 0, sizeof(stats));
+	rc = prepare_reply(NULL, TASKSTATS_CMD_NEW, &rep_skb, &reply, size);
+	if (rc < 0)
+		return;
+
+	rc = fill_pid(tsk->pid, tsk, &stats);
+	if (rc < 0)
+		goto err;
+
+	na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_PID);
+	NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, (u32)tsk->pid);
+	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
+	nla_nest_end(rep_skb, na);
+
+	if (!is_thread_group) {
+		send_reply(rep_skb, 0, TASKSTATS_MSG_MULTICAST);
+		return;
+	}
+
+	memset(&stats, 0, sizeof(stats));
+	rc = fill_tgid(tsk->tgid, tsk, &stats);
+	if (rc < 0)
+		goto err;
+
+	na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_TGID);
+	NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, (u32)tsk->tgid);
+	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
+	nla_nest_end(rep_skb, na);
+
+	send_reply(rep_skb, 0, TASKSTATS_MSG_MULTICAST);
+	return;
+
+nla_put_failure:
+	genlmsg_cancel(rep_skb, reply);
+	return;
+err:
+	nlmsg_free(rep_skb);
+}
+
+static struct genl_ops taskstats_ops = {
+	.cmd            = TASKSTATS_CMD_GET,
+	.doit           = taskstats_send_stats,
+	.policy		= taskstats_cmd_get_policy,
+};
+
+static int __init taskstats_init(void)
+{
+	if (genl_register_family(&family))
+		return -EFAULT;
+        family_registered = 1;
+
+	if (genl_register_ops(&family, &taskstats_ops))
+		goto err;
+
+	return 0;
+err:
+	genl_unregister_family(&family);
+	family_registered = 0;
+	return -EFAULT;
+}
+
+late_initcall(taskstats_init);

Re: [Patch 5/8] generic netlink interface for delay accounting

[Andrew Morton]

Shailabh Nagar <nagar@watson.ibm.com> wrote:
>
> delayacct-genetlink.patch
> 
> Create a generic netlink interface (NETLINK_GENERIC family),
> called "taskstats", for getting delay and cpu statistics of
> tasks and thread groups during their lifetime and when they exit.
> 
> 

It's be best to have a netlink person review the netlinkisms here.

> +static inline int delayacct_add_tsk(struct taskstats *d,
> +				    struct task_struct *tsk)
> +{
> +	if (!tsk->delays)
> +		return -EINVAL;
> +	return __delayacct_add_tsk(d, tsk);
> +}

hm.  It's a worry that this can return an error if delay accounting simply
isn't enabled.

> +struct taskstats {
> +	/* Maintain 64-bit alignment while extending */
> +	/* Version 1 */
> +
> +	/* XXX_count is number of delay values recorded.
> +	 * XXX_total is corresponding cumulative delay in nanoseconds
> +	 */
> +
> +#define TASKSTATS_NOCPUSTATS	1
> +	__u64	cpu_count;
> +	__u64	cpu_delay_total;	/* wait, while runnable, for cpu */
> +	__u64	blkio_count;
> +	__u64	blkio_delay_total;	/* sync,block io completion wait*/
> +	__u64	swapin_count;
> +	__u64	swapin_delay_total;	/* swapin page fault wait*/
> +
> +	__u64	cpu_run_total;		/* cpu running time
> +					 * no count available/provided */
> +};

What locking is used to make updates to these u64's appear to be atomic? 
Maybe it's deliberately nonatomic.  Either way, it needs a comment.

>  void __delayacct_tsk_exit(struct task_struct *tsk)
>  {
> +	/*
> +	 * Protect against racing thread group exits
> +	 */
> +	mutex_lock(&delayacct_exit_mutex);
> +	taskstats_exit_pid(tsk);
>  	if (tsk->delays) {
>  		kmem_cache_free(delayacct_cache, tsk->delays);
>  		tsk->delays = NULL;
>  	}
> +	mutex_unlock(&delayacct_exit_mutex);
>  }

hm, I wonder how contended that lock is likely to be.

The kmem_cache_free() can happen outside the lock.

> +#ifdef CONFIG_TASKSTATS
> +int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
> +{
> +	nsec_t tmp;
> +	struct timespec ts;
> +	unsigned long t1,t2;
> +
> +	/* zero XXX_total,non-zero XXX_count implies XXX stat overflowed */
> +
> +	tmp = (nsec_t)d->cpu_run_total ;

stray space before semicolon.

> +	tmp += (u64)(tsk->utime+tsk->stime)*TICK_NSEC;
> +	d->cpu_run_total = (tmp < (nsec_t)d->cpu_run_total)? 0: tmp;

Missing space before ?, missing space before :

> +	d->blkio_delay_total = (tmp < d->blkio_delay_total)? 0: tmp;
> +	d->swapin_delay_total = (tmp < d->swapin_delay_total)? 0: tmp;

dittos.

> ===================================================================
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ linux-2.6.16/kernel/taskstats.c	2006-03-29 18:13:18.000000000 -0500
> @@ -0,0 +1,292 @@
> +/*
> + * taskstats.c - Export per-task statistics to userland
> + *
> + * Copyright (C) Shailabh Nagar, IBM Corp. 2006
> + *           (C) Balbir Singh,   IBM Corp. 2006
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/taskstats.h>
> +#include <linux/delayacct.h>
> +#include <net/genetlink.h>
> +#include <asm/atomic.h>
> +
> +const int taskstats_version = TASKSTATS_VERSION;

Odd.  It'd be better to put TASKSTATS_VERSION in the header file, use that
directly.

> +
> +static inline int fill_pid(pid_t pid, struct task_struct *pidtsk,
> +			   struct taskstats *stats)
> +{
> +	int rc;
> +	struct task_struct *tsk = pidtsk;
> +
> +	if (!pidtsk) {
> +		read_lock(&tasklist_lock);
> +		tsk = find_task_by_pid(pid);
> +		if (!tsk) {
> +			read_unlock(&tasklist_lock);
> +			return -ESRCH;
> +		}
> +		get_task_struct(tsk);
> +		read_unlock(&tasklist_lock);
> +	} else
> +		get_task_struct(tsk);
> +
> +	rc = delayacct_add_tsk(stats, tsk);
> +	put_task_struct(tsk);
> +
> +	return rc;
> +
> +}

Has two callsites, should be uninlined.

> +static inline int fill_tgid(pid_t tgid, struct task_struct *tgidtsk,
> +			    struct taskstats *stats)
> +{
> +	int rc;
> +	struct task_struct *tsk, *first;
> +
> +	first = tgidtsk;
> +	read_lock(&tasklist_lock);
> +	if (!first) {
> +		first = find_task_by_pid(tgid);
> +		if (!first) {
> +			read_unlock(&tasklist_lock);
> +			return -ESRCH;
> +		}
> +	}
> +	tsk = first;
> +	do {
> +		rc = delayacct_add_tsk(stats, tsk);
> +		if (rc)
> +			break;
> +	} while_each_thread(first, tsk);
> +	read_unlock(&tasklist_lock);
> +
> +	return rc;
> +}

Ditto.

It's somewhat similar to fill_pid() - perhaps they can be combined, halving
the overhead?

> +static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
> +{
> +	int rc = 0;
> +	struct sk_buff *rep_skb;
> +	struct taskstats stats;
> +	void *reply;
> +	size_t size;
> +	struct nlattr *na;
> +
> +	/*
> +	 * Size includes space for nested attribute as well
> +	 * The returned data is of the format
> +	 * TASKSTATS_TYPE_AGGR_PID/TGID
> +	 * --> TASKSTATS_TYPE_PID/TGID
> +	 * --> TASKSTATS_TYPE_STATS
> +	 */
> +	size = nla_total_size(sizeof(u32)) +
> +		nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
> +
> +	memset(&stats, 0, sizeof(stats));
> +	rc = prepare_reply(info, TASKSTATS_CMD_NEW, &rep_skb, &reply, size);
> +	if (rc < 0)
> +		return rc;
> +
> +	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> +		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> +		rc = fill_pid((pid_t)pid, NULL, &stats);

We shouldn't have a typecast here.  If it generates a warning then we need
to get in there and find out why.

> +		if (rc < 0)
> +			goto err;
> +
> +		na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_PID);
> +		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, pid);
> +	} else if (info->attrs[TASKSTATS_CMD_ATTR_TGID]) {
> +		u32 tgid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_TGID]);
> +		rc = fill_tgid((pid_t)tgid, NULL, &stats);

Ditto.

> +		if (rc < 0)
> +			goto err;
> +
> +		na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_TGID);
> +		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, tgid);
> +	} else {
> +		rc = -EINVAL;
> +		goto err;
> +	}
> +
> +	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> +	nla_nest_end(rep_skb, na);
> +
> +	return send_reply(rep_skb, info->snd_pid, TASKSTATS_MSG_UNICAST);
> +
> +nla_put_failure:
> +	return  genlmsg_cancel(rep_skb, reply);

Extra space.

> +err:
> +	nlmsg_free(rep_skb);
> +	return rc;
> +}
> +
> +
> +/* Send pid data out on exit */
> +void taskstats_exit_pid(struct task_struct *tsk)
> +{
> +	int rc = 0;
> +	struct sk_buff *rep_skb;
> +	void *reply;
> +	struct taskstats stats;
> +	size_t size;
> +	int is_thread_group = !thread_group_empty(tsk);
> +	struct nlattr *na;
> +
> +	/*
> +	 * tasks can start to exit very early. Ensure that the family
> +	 * is registered before notifications are sent out
> +	 */
> +	if (!family_registered)
> +		return;

This code risks evaluating thread_group_empty() even if !family_registered.
The compiler will most likely sort that out in this case, but it's a risk
when using these initialisers.

> +
> +static int __init taskstats_init(void)
> +{
> +	if (genl_register_family(&family))
> +		return -EFAULT;

EFAULT?

> +        family_registered = 1;

whitespace broke.

> +
> +	if (genl_register_ops(&family, &taskstats_ops))
> +		goto err;
> +
> +	return 0;
> +err:
> +	genl_unregister_family(&family);
> +	family_registered = 0;
> +	return -EFAULT;
> +}
> +
> +late_initcall(taskstats_init);

Why late_initcall()?  (A comment would be appropriate)

Re: [Patch 5/8] generic netlink interface for delay accounting

[Balbir Singh]

Hi, Andrew


On Wed, Mar 29, 2006 at 09:04:06PM -0800, Andrew Morton wrote:
> Shailabh Nagar <nagar@watson.ibm.com> wrote:
> >
> > delayacct-genetlink.patch
> > 
> > Create a generic netlink interface (NETLINK_GENERIC family),
> > called "taskstats", for getting delay and cpu statistics of
> > tasks and thread groups during their lifetime and when they exit.
> > 
> > 
> 
> It's be best to have a netlink person review the netlinkisms here.

Jamal did review this code, his comments are available at
http://lkml.org/lkml/2006/3/26/71. His comments were very helpful in
doing the correct thing and understanding the design and usage of
genetlink.

> 
> > +static inline int delayacct_add_tsk(struct taskstats *d,
> > +				    struct task_struct *tsk)
> > +{
> > +	if (!tsk->delays)
> > +		return -EINVAL;
> > +	return __delayacct_add_tsk(d, tsk);
> > +}
> 
> hm.  It's a worry that this can return an error if delay accounting simply
> isn't enabled.

Yes, if CONFIG_TASKSTATS is enabled and the kernel is booted without
delayacct. A user space utility trying to extract statistics will get
an error.

> 
> > +struct taskstats {
> > +	/* Maintain 64-bit alignment while extending */
> > +	/* Version 1 */
> > +
> > +	/* XXX_count is number of delay values recorded.
> > +	 * XXX_total is corresponding cumulative delay in nanoseconds
> > +	 */
> > +
> > +#define TASKSTATS_NOCPUSTATS	1
> > +	__u64	cpu_count;
> > +	__u64	cpu_delay_total;	/* wait, while runnable, for cpu */
> > +	__u64	blkio_count;
> > +	__u64	blkio_delay_total;	/* sync,block io completion wait*/
> > +	__u64	swapin_count;
> > +	__u64	swapin_delay_total;	/* swapin page fault wait*/
> > +
> > +	__u64	cpu_run_total;		/* cpu running time
> > +					 * no count available/provided */
> > +};
> 
> What locking is used to make updates to these u64's appear to be atomic? 
> Maybe it's deliberately nonatomic.  Either way, it needs a comment.

These fields are protected by tsk->delays->lock. We will add comments
to indicate the same.

> 
> >  void __delayacct_tsk_exit(struct task_struct *tsk)
> >  {
> > +	/*
> > +	 * Protect against racing thread group exits
> > +	 */
> > +	mutex_lock(&delayacct_exit_mutex);
> > +	taskstats_exit_pid(tsk);
> >  	if (tsk->delays) {
> >  		kmem_cache_free(delayacct_cache, tsk->delays);
> >  		tsk->delays = NULL;
> >  	}
> > +	mutex_unlock(&delayacct_exit_mutex);
> >  }
> 
> hm, I wonder how contended that lock is likely to be.
>

It is taken for every exiting task. We did not measure the contention
using any tool.
 
> The kmem_cache_free() can happen outside the lock.


kmem_cache_free() and setting to NULL outside the lock is prone to
race conditions. Consider the following scenario

A thread group T1 has exiting processes P1 and P2

P1 is exiting, finishes the delay accounting by calling taskstats_exit_pid()
and gives up the mutex and calls kmem_cache_free(), but before it can set
tsk->delays to NULL, we try to get statistics for the entire thread group.
This task will show up in the thread group with a dangling tsk->delays.

> 
> > +#ifdef CONFIG_TASKSTATS
> > +int __delayacct_add_tsk(struct taskstats *d, struct task_struct *tsk)
> > +{
> > +	nsec_t tmp;
> > +	struct timespec ts;
> > +	unsigned long t1,t2;
> > +
> > +	/* zero XXX_total,non-zero XXX_count implies XXX stat overflowed */
> > +
> > +	tmp = (nsec_t)d->cpu_run_total ;
> 
> stray space before semicolon.

Will fix it.

> 
> > +	tmp += (u64)(tsk->utime+tsk->stime)*TICK_NSEC;
> > +	d->cpu_run_total = (tmp < (nsec_t)d->cpu_run_total)? 0: tmp;
> 
> Missing space before ?, missing space before :

Will fix it.

> 
> > +	d->blkio_delay_total = (tmp < d->blkio_delay_total)? 0: tmp;
> > +	d->swapin_delay_total = (tmp < d->swapin_delay_total)? 0: tmp;
> 
> dittos.

Will fix it.

> 
> > ===================================================================
> > --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> > +++ linux-2.6.16/kernel/taskstats.c	2006-03-29 18:13:18.000000000 -0500
> > @@ -0,0 +1,292 @@
> > +/*
> > + * taskstats.c - Export per-task statistics to userland
> > + *
> > + * Copyright (C) Shailabh Nagar, IBM Corp. 2006
> > + *           (C) Balbir Singh,   IBM Corp. 2006
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/taskstats.h>
> > +#include <linux/delayacct.h>
> > +#include <net/genetlink.h>
> > +#include <asm/atomic.h>
> > +
> > +const int taskstats_version = TASKSTATS_VERSION;
> 
> Odd.  It'd be better to put TASKSTATS_VERSION in the header file, use that
> directly.

We will fix this.

> 
> > +
> > +static inline int fill_pid(pid_t pid, struct task_struct *pidtsk,
> > +			   struct taskstats *stats)
> > +{
> > +	int rc;
> > +	struct task_struct *tsk = pidtsk;
> > +
> > +	if (!pidtsk) {
> > +		read_lock(&tasklist_lock);
> > +		tsk = find_task_by_pid(pid);
> > +		if (!tsk) {
> > +			read_unlock(&tasklist_lock);
> > +			return -ESRCH;
> > +		}
> > +		get_task_struct(tsk);
> > +		read_unlock(&tasklist_lock);
> > +	} else
> > +		get_task_struct(tsk);
> > +
> > +	rc = delayacct_add_tsk(stats, tsk);
> > +	put_task_struct(tsk);
> > +
> > +	return rc;
> > +
> > +}
> 
> Has two callsites, should be uninlined.

Will do.

> 
> > +static inline int fill_tgid(pid_t tgid, struct task_struct *tgidtsk,
> > +			    struct taskstats *stats)
> > +{
> > +	int rc;
> > +	struct task_struct *tsk, *first;
> > +
> > +	first = tgidtsk;
> > +	read_lock(&tasklist_lock);
> > +	if (!first) {
> > +		first = find_task_by_pid(tgid);
> > +		if (!first) {
> > +			read_unlock(&tasklist_lock);
> > +			return -ESRCH;
> > +		}
> > +	}
> > +	tsk = first;
> > +	do {
> > +		rc = delayacct_add_tsk(stats, tsk);
> > +		if (rc)
> > +			break;
> > +	} while_each_thread(first, tsk);
> > +	read_unlock(&tasklist_lock);
> > +
> > +	return rc;
> > +}
> 
> Ditto.

Will do.

> 
> It's somewhat similar to fill_pid() - perhaps they can be combined, halving
> the overhead?

Yes, there is a lot of synergy between the two. The main difference is
the way we lock (using get/put_task_struct for pids and tasklist_lock
for tgids). Using a flag to do the correct thing looked a bit ugly,
so we split the functions.

> 
> > +static int taskstats_send_stats(struct sk_buff *skb, struct genl_info *info)
> > +{
> > +	int rc = 0;
> > +	struct sk_buff *rep_skb;
> > +	struct taskstats stats;
> > +	void *reply;
> > +	size_t size;
> > +	struct nlattr *na;
> > +
> > +	/*
> > +	 * Size includes space for nested attribute as well
> > +	 * The returned data is of the format
> > +	 * TASKSTATS_TYPE_AGGR_PID/TGID
> > +	 * --> TASKSTATS_TYPE_PID/TGID
> > +	 * --> TASKSTATS_TYPE_STATS
> > +	 */
> > +	size = nla_total_size(sizeof(u32)) +
> > +		nla_total_size(sizeof(struct taskstats)) + nla_total_size(0);
> > +
> > +	memset(&stats, 0, sizeof(stats));
> > +	rc = prepare_reply(info, TASKSTATS_CMD_NEW, &rep_skb, &reply, size);
> > +	if (rc < 0)
> > +		return rc;
> > +
> > +	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> > +		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> > +		rc = fill_pid((pid_t)pid, NULL, &stats);
> 
> We shouldn't have a typecast here.  If it generates a warning then we need
> to get in there and find out why.

The reason for a typecast is that pid is passed as a u32 from userspace.
genetlink currently supports most unsigned types with little or no
support for signed types. We exchange data as u32 and do the correct
thing in the kernel. Would you like us to move away from this?

> 
> > +		if (rc < 0)
> > +			goto err;
> > +
> > +		na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_PID);
> > +		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_PID, pid);
> > +	} else if (info->attrs[TASKSTATS_CMD_ATTR_TGID]) {
> > +		u32 tgid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_TGID]);
> > +		rc = fill_tgid((pid_t)tgid, NULL, &stats);
> 
> Ditto.
> 
> > +		if (rc < 0)
> > +			goto err;
> > +
> > +		na = nla_nest_start(rep_skb, TASKSTATS_TYPE_AGGR_TGID);
> > +		NLA_PUT_U32(rep_skb, TASKSTATS_TYPE_TGID, tgid);
> > +	} else {
> > +		rc = -EINVAL;
> > +		goto err;
> > +	}
> > +
> > +	NLA_PUT_TYPE(rep_skb, struct taskstats, TASKSTATS_TYPE_STATS, stats);
> > +	nla_nest_end(rep_skb, na);
> > +
> > +	return send_reply(rep_skb, info->snd_pid, TASKSTATS_MSG_UNICAST);
> > +
> > +nla_put_failure:
> > +	return  genlmsg_cancel(rep_skb, reply);
> 
> Extra space.

Will fix.

> 
> > +err:
> > +	nlmsg_free(rep_skb);
> > +	return rc;
> > +}
> > +
> > +
> > +/* Send pid data out on exit */
> > +void taskstats_exit_pid(struct task_struct *tsk)
> > +{
> > +	int rc = 0;
> > +	struct sk_buff *rep_skb;
> > +	void *reply;
> > +	struct taskstats stats;
> > +	size_t size;
> > +	int is_thread_group = !thread_group_empty(tsk);
> > +	struct nlattr *na;
> > +
> > +	/*
> > +	 * tasks can start to exit very early. Ensure that the family
> > +	 * is registered before notifications are sent out
> > +	 */
> > +	if (!family_registered)
> > +		return;
> 
> This code risks evaluating thread_group_empty() even if !family_registered.
> The compiler will most likely sort that out in this case, but it's a risk
> when using these initialisers.
> 

Yes, this can be optimized and we can initialize it after the check for
!family_registerd

> > +
> > +static int __init taskstats_init(void)
> > +{
> > +	if (genl_register_family(&family))
> > +		return -EFAULT;
> 
> EFAULT?

It shouldn't be (Shailabh please comment). We will fix it.

> 
> > +        family_registered = 1;
> 
> whitespace broke.

Will fix

> 
> > +
> > +	if (genl_register_ops(&family, &taskstats_ops))
> > +		goto err;
> > +
> > +	return 0;
> > +err:
> > +	genl_unregister_family(&family);
> > +	family_registered = 0;
> > +	return -EFAULT;
> > +}
> > +
> > +late_initcall(taskstats_init);
> 
> Why late_initcall()?  (A comment would be appropriate)

We will add a comment.

Thanks for your detailed review,
Balbir

Re: [Patch 5/8] generic netlink interface for delay accounting

[Andrew Morton]

Balbir Singh <balbir@in.ibm.com> wrote:
>
> > The kmem_cache_free() can happen outside the lock.
> 
> 
> kmem_cache_free() and setting to NULL outside the lock is prone to
> race conditions. Consider the following scenario
> 
> A thread group T1 has exiting processes P1 and P2
> 
> P1 is exiting, finishes the delay accounting by calling taskstats_exit_pid()
> and gives up the mutex and calls kmem_cache_free(), but before it can set
> tsk->delays to NULL, we try to get statistics for the entire thread group.
> This task will show up in the thread group with a dangling tsk->delays.

Yes, the `tsk->delays = NULL;' needs to happen inside the lock.  But the
kmem_cache_free() does not.  It pointlessly increases the lock hold time.

> > > +	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> > > +		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> > > +		rc = fill_pid((pid_t)pid, NULL, &stats);
> > 
> > We shouldn't have a typecast here.  If it generates a warning then we need
> > to get in there and find out why.
> 
> The reason for a typecast is that pid is passed as a u32 from userspace.
> genetlink currently supports most unsigned types with little or no
> support for signed types. We exchange data as u32 and do the correct
> thing in the kernel. Would you like us to move away from this?
> 

I think it's best to avoid the cast unless it's actually needed to avoid a
warning or compile error, or to do special things with sign extension. 
Because casts clutter up the code and can hide real bugs.  In this case the
compiler should silently perform the conversion.

Re: [Patch 5/8] generic netlink interface for delay accounting

[Balbir Singh]

On Wed, Mar 29, 2006 at 10:26:29PM -0800, Andrew Morton wrote:
> Balbir Singh <balbir@in.ibm.com> wrote:
> >
> > > The kmem_cache_free() can happen outside the lock.
> > 
> > 
> > kmem_cache_free() and setting to NULL outside the lock is prone to
> > race conditions. Consider the following scenario
> > 
> > A thread group T1 has exiting processes P1 and P2
> > 
> > P1 is exiting, finishes the delay accounting by calling taskstats_exit_pid()
> > and gives up the mutex and calls kmem_cache_free(), but before it can set
> > tsk->delays to NULL, we try to get statistics for the entire thread group.
> > This task will show up in the thread group with a dangling tsk->delays.
> 
> Yes, the `tsk->delays = NULL;' needs to happen inside the lock.  But the
> kmem_cache_free() does not.  It pointlessly increases the lock hold time.

Understood will fix it

> 
> > > > +	if (info->attrs[TASKSTATS_CMD_ATTR_PID]) {
> > > > +		u32 pid = nla_get_u32(info->attrs[TASKSTATS_CMD_ATTR_PID]);
> > > > +		rc = fill_pid((pid_t)pid, NULL, &stats);
> > > 
> > > We shouldn't have a typecast here.  If it generates a warning then we need
> > > to get in there and find out why.
> > 
> > The reason for a typecast is that pid is passed as a u32 from userspace.
> > genetlink currently supports most unsigned types with little or no
> > support for signed types. We exchange data as u32 and do the correct
> > thing in the kernel. Would you like us to move away from this?
> > 
> 
> I think it's best to avoid the cast unless it's actually needed to avoid a
> warning or compile error, or to do special things with sign extension. 
> Because casts clutter up the code and can hide real bugs.  In this case the
> compiler should silently perform the conversion.

Yep, the compiler was doing it for me, but I tried to be smart and cast
things around. Will fix it.

Thanks,
Balbir

Re: [Patch 5/8] generic netlink interface for delay accounting

[Shailabh Nagar]

Balbir Singh wrote:

>Hi, Andrew
>
>
>On Wed, Mar 29, 2006 at 09:04:06PM -0800, Andrew Morton wrote:
>  
>
>>Shailabh Nagar <nagar@watson.ibm.com> wrote:
>>    
>>
>>>delayacct-genetlink.patch
>>>
>>>Create a generic netlink interface (NETLINK_GENERIC family),
>>>called "taskstats", for getting delay and cpu statistics of
>>>tasks and thread groups during their lifetime and when they exit.
>>>
>>>
>>>
>>>      
>>>
<snip>

>>>+
>>>+static int __init taskstats_init(void)
>>>+{
>>>+	if (genl_register_family(&family))
>>>+		return -EFAULT;
>>>      
>>>
>>EFAULT?
>>    
>>
>
>It shouldn't be (Shailabh please comment). We will fix it.
>  
>
Sorry, it should return the  value returned by genl_register_family.

I copied the code from net/tipc/netlink.c
where a similarly erroneous errno is being used. We'll submit a fix
for that as well.

--Shailabh