per-task delay accounting

delayacct-setup.patch

Initialization code related to collection of per-task "delay" statistics which measure how long it had to wait for cpu, sync block io, swapping etc. The collection of statistics and the interface are in other patches. This patch sets up the data structures and allows the statistics collection to be disabled through a kernel boot paramater.

...

kernel/delayacct.c new file +12 -0

@@ -0,0 +1,92 @@
+/* delayacct.c - per-task delay accounting
+ *
+ * Copyright (C) Shailabh Nagar, 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 would be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See

kernel/delayacct.c new file +13 -0

+/*
+ * Finish delay accounting for a statistic using
+ * its timestamps (@start, @end), accumalator (@total) and @count
+ */
+
+static inline void delayacct_end(struct timespec *start, struct timespec *end,
+				u64 *total, u32 *count)
+{
+	struct timespec ts;
+	nsec_t ns;
+
+	do_posix_clock_monotonic_gettime(end);
+	ts.tv_sec = end->tv_sec - start->tv_sec;

delayacct-blkio-swapin.patch

Collect per-task block I/O delay statistics.

Unlike earlier iterations of the delay accounting patches, now delays are only collected for the actual I/O waits rather than try and cover the delays seen in I/O submission paths.

Account separately for block I/O delays
incurred as a result of swapin page faults whose frequency can be affected by the task/process' rss limit. Hence swapin delays can act as feedback for rss limit changes independent of I/O priority changes.

..

+#define PF_SWAPIN 0x02000000 /* I am doing a swap in */

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.

include/linux/taskstats.h new file +9 -0

+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

===================================================================

kernel/taskstats.c new file +15 -0

@@ -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.

kernel/taskstats.c new file +15 -0

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

kernel/taskstats.c new file +15 -0

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

kernel/taskstats.c new file +17 -0

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

kernel/taskstats.c new file +7 -0

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

kernel/taskstats.c new file +13 -0

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

delayacct-schedstats.patch

Make the task-related schedstats functions callable by delay accounting even if schedstats collection isn't turned on. This removes the dependency of delay accounting on schedstats and allows cpu delays to be exported.

..

Index: linux-2.6.16/include/linux/sched.h
===================================================================

include/linux/sched.h +1 -1

@@ -525,7 +525,7 @@ typedef struct prio_array prio_array_t;
 struct backing_dev_info;
 struct reclaim_state;
 
-#ifdef CONFIG_SCHEDSTATS
+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
 struct sched_info {
 	/* cumulative counters */
 	unsigned long	cpu_time,	/* time spent on the cpu */

@@ -537,10 +537,14 @@ struct sched_info {
 			last_queued;	/* when we were last queued to run */
 };

Could you please include the following delay accounting patches
in -mm ?

delayacct-procfs.patch

Export I/O delays seen by a task through /proc/<tgid>/stats for use in top etc.

Note that delays for I/O done for swapping in pages (swapin I/O) is clubbed together with all other I/O here (this is not the case in the netlink interface where the swapin I/O is kept distinct)

...

delayacct-virtcpu.patch

Distinguish between "wall-clock" and "virtual" cpu run times and return both, at per-task and per-tgid granularity.

Some architectures adjust tsk->utime+tsk->stime to reflect the time that the kernel wasn't scheduled in hypervised environments and this is the "wall-clock" cpu run time. "Virtual" cpu run time, on the other hand, does not account for the kernel being descheduled.

This patch allows the most accurate "virtual" cpu run time, collected by the schedstats code (now shared with delay accounting code), to be returned to user space, in addition to the "wall-clock" cpu time that was being exported earlier. Both these times are useful for workload management in different situations.

In a non-virtualized environment, or on architectures which do not adjust tsk->utime/stime, these will effectively be the same value but at different granularities.

...

Index: linux-2.6.16/include/linux/taskstats.h
===================================================================

include/linux/taskstats.h +0 -2

@@ -46,8 +46,14 @@ struct taskstats {
 	__u64	swapin_count;
 	__u64	swapin_delay_total;	/* swapin page fault wait*/
 
-	__u64	cpu_run_total;		/* cpu running time
-					 * no count available/provided */

Quoted 8 lines · 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.

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

include/linux/delayacct.h static inline void delayacct_blkio_start · line 76

Quoted 8 lines

include/linux/delayacct.h +7 -0

+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.

Quoted 15 lines

include/linux/taskstats.h new file +14 -0

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

include/linux/taskstats.h line 51

Quoted 5 lines

include/linux/taskstats.h new file +5 -0

+	__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.

Quoted 4 lines

void __delayacct_tsk_exit(struct task_struct *tsk) {

kernel/delayacct.c +1 -0

+	/*

kernel/delayacct.c void __delayacct_tsk_init(struct task_st · line 67

Quoted 10 lines

kernel/delayacct.c +5 -0

+	 * 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.

kernel/delayacct.c void __delayacct_blkio_end(void) · line 130

Quoted 11 lines

kernel/delayacct.c +10 -0

+#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.

kernel/delayacct.c void __delayacct_blkio_end(void) · line 132

Quoted 3 lines

kernel/delayacct.c +2 -0

+	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 :

kernel/delayacct.c void __delayacct_blkio_end(void) · line 148

Quoted 3 lines

kernel/delayacct.c +2 -0

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

dittos.

Quoted 25 lines

===================================================================

kernel/taskstats.c new file +20 -0

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

kernel/taskstats.c line 25

Quoted 5 lines

kernel/taskstats.c new file +5 -0

+#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.

Quoted 20 lines

kernel/taskstats.c new file +19 -0

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

kernel/taskstats.c line 118

Quoted 6 lines

kernel/taskstats.c new file +6 -0

+	rc = delayacct_add_tsk(stats, tsk);
+	put_task_struct(tsk);
+
+	return rc;
+
+}

Has two callsites, should be uninlined.

Quoted 20 lines

kernel/taskstats.c new file +19 -0

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

kernel/taskstats.c line 144

Quoted 6 lines

kernel/taskstats.c new file +6 -0

+			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?

Quoted 23 lines

kernel/taskstats.c new file +22 -0

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

kernel/taskstats.c line 172

Quoted 5 lines

kernel/taskstats.c new file +5 -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.

kernel/taskstats.c line 180

Quoted 9 lines

kernel/taskstats.c new file +8 -0

+		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.

Quoted 11 lines

kernel/taskstats.c new file +11 -0

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

kernel/taskstats.c line 197

Quoted 6 lines

kernel/taskstats.c new file +6 -0

+	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.

Quoted 20 lines

kernel/taskstats.c new file +19 -0

+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

kernel/taskstats.c line 220

Quoted 4 lines

kernel/taskstats.c new file +4 -0

+	 * 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.

kernel/taskstats.c line 279

Quoted 5 lines

kernel/taskstats.c new file +5 -0

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

EFAULT?

kernel/taskstats.c line 280

Quoted 2 lines

kernel/taskstats.c new file +1 -0

+        family_registered = 1;

whitespace broke.

kernel/taskstats.c line 292

Quoted 13 lines

kernel/taskstats.c new file +12 -0

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

Quoted 2 lines

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.

Quoted 6 lines

kernel/taskstats.c line 172

Quoted 3 lines

kernel/taskstats.c new file +3 -0

+	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?

Quoted 3 lines · Shailabh Nagar

Could you please include the following delay accounting patches
in -mm ?

I'm at a loss to evaluate the suitability of this work, really. I always am when accounting patches come along.

There are various people and various groups working on various different things and there appears to be no coordination and little commonality of aims. I worry that picking one submission basically at random will provide nothing which the other groups can work on to build up their feature.

On the other hand, we don't want to do nothing until some uber-grand all-singing, all-dancing statistics-gathering infrastructure comes along.

So I'm a bit stuck. What I would like to see happen is that there be some coordination between the various stakeholders, and some vague plan which they're all happy with as a basis for the eventual grand solution.

We already have various bits and pieces of statistics gathering in the kernel and it's already a bit ad-hoc. Adding more one-requirement-specific accounting code won't improve that situation.

But then, I said all this a year or two ago and nothing much has happened since then. It's not your fault, but it's a problem.

Perhaps a good starting point would be a one-page bullet-point-form wishlist of all the accounting which people want to get out of the kernel, and a description of what the kernel<->user interface should look like. Right now, I don't think we even have a picture of that.

We need a statistics maintainer, too, to pull together the plan, coordinate, push things forwards. The first step would be to identify the stakeholders, come up with that page of bullet-points.

Then again, maybe the right thing to do is to keep adding low-impact requirement-specific statistics patches as they come along. But if we're going to do it that way, we need an up-front reason for doing so, and I don't know what that would be.

See my problem?

Quoted 9 lines · Balbir Singh

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 8 lines

Quoted 7 lines

kernel/taskstats.c line 172

Quoted 4 lines

kernel/taskstats.c new file +3 -0

+	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.

Quoted 5 lines

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.

Quoted 36 lines · Andrew Morton

Quoted 3 lines · Shailabh Nagar

Could you please include the following delay accounting patches
in -mm ?

I'm at a loss to evaluate the suitability of this work, really. I always am when accounting patches come along.

There are various people and various groups working on various different things and there appears to be no coordination and little commonality of aims. I worry that picking one submission basically at random will provide nothing which the other groups can work on to build up their feature.

On the other hand, we don't want to do nothing until some uber-grand all-singing, all-dancing statistics-gathering infrastructure comes along.

So I'm a bit stuck. What I would like to see happen is that there be some coordination between the various stakeholders, and some vague plan which they're all happy with as a basis for the eventual grand solution.

We already have various bits and pieces of statistics gathering in the kernel and it's already a bit ad-hoc. Adding more one-requirement-specific accounting code won't improve that situation.

But then, I said all this a year or two ago and nothing much has happened since then. It's not your fault, but it's a problem.

Perhaps a good starting point would be a one-page bullet-point-form wishlist of all the accounting which people want to get out of the kernel, and a description of what the kernel<->user interface should look like. Right now, I don't think we even have a picture of that.

We need a statistics maintainer, too, to pull together the plan, coordinate, push things forwards. The first step would be to identify the stakeholders, come up with that page of bullet-points.

Quoted 6 lines

Then again, maybe the right thing to do is to keep adding low-impact requirement-specific statistics patches as they come along. But if we're going to do it that way, we need an up-front reason for doing so, and I don't know what that would be.

See my problem?

One of the issues we have tried to address is the ability to provide some form of a common ground for all the statistics to co-exist. Various methods were discussed for exchanging data between kernel and user space, genetlink was suggested often and the clear winner.

To that end, we have created a taskstats.c file. Any subsystem wanting to add their statistics and sending it to user space can add their own types by extending taskstats.c (changing the version number) and creating their own types using genetlink. They will have to do the following

1. Add statistics gathering in their own subsystem
2. Add a type to taskstats.c, extend it and use data from (1) and send it to user space.

The data from various subsystems can co-exist. I feel that this could serve as the basic common infrastructure to begin with and refined later (depending on the needs of other people).

CSA accounting/PAGG/JOB:
Jay Lan <[email protected]>
Limin Gu <[email protected]>

Sounds fine to me, but I'm not a stakeholder.

Trolling back through lse-tech gives us:

Scalable statistics counters with /proc reporting:
Ravikiran G Thirumalai <[email protected]>
(Kiran feft IBM, but presumably the requirement lives on)

Quoted 9 lines · Shailabh Nagar

delayacct-setup.patch

Initialization code related to collection of per-task "delay" statistics which measure how long it had to wait for cpu, sync block io, swapping etc. The collection of statistics and the interface are in other patches. This patch sets up the data structures and allows the statistics collection to be disabled through a kernel boot paramater.

Documentation/kernel-parameters.txt running once the system is up. · line 420

Quoted 6 lines

...

Documentation/kernel-parameters.txt +2 -0

+	delayacct	[KNL] Enable per-task delay accounting
+

Why does this boot parameter exist?

The code is neat-looking.

Quoted 21 lines

kernel/delayacct.c new file +18 -0

@@ -0,0 +1,92 @@
+/* delayacct.c - per-task delay accounting
+ *
+ * Copyright (C) Shailabh Nagar, 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 would 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/sched.h>
+#include <linux/slab.h>
+#include <linux/time.h>

kernel/delayacct.c line 22

Quoted 6 lines

kernel/delayacct.c new file +4 -0

+#include <linux/sysctl.h>
+#include <linux/delayacct.h>
+
+int delayacct_on __read_mostly = 0;	/* Delay accounting turned on/off */

Yes, it should be __read_mostly. But it shouldn't be initialised to zero.

kernel/delayacct.c line 49

Quoted 12 lines

kernel/delayacct.c new file +8 -0

+void __delayacct_tsk_init(struct task_struct *tsk)
+{
+	tsk->delays = kmem_cache_alloc(delayacct_cache, SLAB_KERNEL);
+	if (tsk->delays) {
+		memset(tsk->delays, 0, sizeof(*tsk->delays));
+		spin_lock_init(&tsk->delays->lock);
+	}
+}

We have kmem_cache_zalloc() now.

kernel/delayacct.c line 57

Quoted 11 lines

kernel/delayacct.c new file +7 -0

+void __delayacct_tsk_exit(struct task_struct *tsk)
+{
+	if (tsk->delays) {
+		kmem_cache_free(delayacct_cache, tsk->delays);
+		tsk->delays = NULL;
+	}
+}

delayacct_tsk_exit() already checked tsk->delays.

Quoted 22 lines

kernel/delayacct.c new file +20 -0

+/*
+ * Finish delay accounting for a statistic using
+ * its timestamps (@start, @end), accumalator (@total) and @count
+ */
+
+static inline void delayacct_end(struct timespec *start, struct timespec *end,
+				u64 *total, u32 *count)
+{
+	struct timespec ts;
+	nsec_t ns;
+
+	do_posix_clock_monotonic_gettime(end);
+	ts.tv_sec = end->tv_sec - start->tv_sec;
+	ts.tv_nsec = end->tv_nsec - start->tv_nsec;
+	ns = timespec_to_ns(&ts);
+	if (ns < 0)
+		return;
+
+	spin_lock(&current->delays->lock);
+	*total += ns;

kernel/delayacct.c line 91

Quoted 5 lines

kernel/delayacct.c new file +3 -0

+	(*count)++;
+	spin_unlock(&current->delays->lock);
+}

It's strange to have a static inline function at the end of a .c file. I guess this gets used in later patches.

It looks rather too big to be inlined.

I'm surprised that we don't already have a timeval_sub() function somewhere.

The code you have there will cause ts.tv_nsec to go negative half the time. It looks like timespec_to_ns() will happily fix that up for us, but it's all a bit fragile.

Quoted 15 lines · Shailabh Nagar

delayacct-blkio-swapin.patch

Collect per-task block I/O delay statistics.

Unlike earlier iterations of the delay accounting patches, now delays are only collected for the actual I/O waits rather than try and cover the delays seen in I/O submission paths.

Account separately for block I/O delays
incurred as a result of swapin page faults whose frequency can be affected by the task/process' rss limit. Hence swapin delays can act as feedback for rss limit changes independent of I/O priority changes.

Quoted 6 lines

..

+#define PF_SWAPIN 0x02000000 /* I am doing a swap in */

Is there really no sane alternative to doing it this way?

Quoted 28 lines · Shailabh Nagar

delayacct-schedstats.patch

Make the task-related schedstats functions callable by delay accounting even if schedstats collection isn't turned on. This removes the dependency of delay accounting on schedstats and allows cpu delays to be exported.

..

Index: linux-2.6.16/include/linux/sched.h
===================================================================

include/linux/sched.h +2 -1

@@ -525,7 +525,7 @@ typedef struct prio_array prio_array_t;
 struct backing_dev_info;
 struct reclaim_state;
 
-#ifdef CONFIG_SCHEDSTATS
+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
 struct sched_info {
 	/* cumulative counters */
 	unsigned long	cpu_time,	/* time spent on the cpu */

@@ -537,10 +537,14 @@ struct sched_info {
 			last_queued;	/* when we were last queued to run */
 };
 
+#ifdef CONFIG_SCHEDSTATS

include/linux/sched.h struct sched_info { · line 549

Quoted 10 lines

include/linux/sched.h +4 -1

 extern struct file_operations proc_schedstat_operations;
-#endif
+#endif /* CONFIG_SCHEDSTATS */
+#endif /* defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT) */
 
 #ifdef CONFIG_TASK_DELAY_ACCT
+extern int delayacct_on;
+
 
 

This was already declared in delayacct.h and nothing in sched.h needs it. So it's better if .c files pick this declaration up from delayacct.h.

include/linux/sched.h struct sched_info {

Quoted 10 lines

include/linux/sched.h +8 -0

+static inline void rq_sched_info_arrive(struct runqueue *rq,
+						unsigned long diff)
+{
+	if (rq) {
+		rq->rq_sched_info.run_delay += diff;
+		rq->rq_sched_info.pcnt++;
+	}
+}

The nonatomic updates need locking. I assume it's runqueue.lock, but a comment describing the rules would be good.

include/linux/sched.h

Quoted 8 lines

include/linux/sched.h +6 -0

+static inline void rq_sched_info_depart(struct runqueue *rq,
+						unsigned long diff)
+{
+	if (rq)
+		rq->rq_sched_info.cpu_time += diff;
+}

Ditto.

Quoted 9 lines

static inline void sched_info_queued(task_t *t) {

include/linux/sched.h +3 -2

-	if (!t->sched_info.last_queued)
-		t->sched_info.last_queued = jiffies;
+	if (unlikely(sched_info_on()))
+		if (!t->sched_info.last_queued)
+			t->sched_info.last_queued = jiffies;

Quoted 3 lines

}

It might be better to put the unlikely() into sched_info_on() itself.

Quoted 8 lines · Shailabh Nagar

kernel/sched.c +3 -2

-#ifdef CONFIG_SCHEDSTATS
-       memset(&p->sched_info, 0, sizeof(p->sched_info));
+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
+       if (unlikely(sched_info_on()))
+               memset(&p->sched_info, 0, sizeof(p->sched_info));

#endif

If you unconditionally define sched_info_on(), you can get get rid of this #ifdef.

kernel/sched.c +12 -0

+#if defined(CONFIG_SCHEDSTATS) || defined(CONFIG_TASK_DELAY_ACCT)
+
+static inline int sched_info_on(void)
+{
+ #ifdef CONFIG_SCHEDSTATS
+       return 1;
+#elif defined(CONFIG_TASK_DELAY_ACCT)
+       return delayacct_on;
+#else
+	return 0;
+#endif
+}

Might as well hide the junk in a header.

-- Dave

Quoted 30 lines · Shailabh Nagar

delayacct-virtcpu.patch

Distinguish between "wall-clock" and "virtual" cpu run times and return both, at per-task and per-tgid granularity.

Some architectures adjust tsk->utime+tsk->stime to reflect the time that the kernel wasn't scheduled in hypervised environments and this is the "wall-clock" cpu run time. "Virtual" cpu run time, on the other hand, does not account for the kernel being descheduled.

This patch allows the most accurate "virtual" cpu run time, collected by the schedstats code (now shared with delay accounting code), to be returned to user space, in addition to the "wall-clock" cpu time that was being exported earlier. Both these times are useful for workload management in different situations.

In a non-virtualized environment, or on architectures which do not adjust tsk->utime/stime, these will effectively be the same value but at different granularities.

...

Index: linux-2.6.16/include/linux/taskstats.h
===================================================================

include/linux/taskstats.h +0 -0

@@ -46,8 +46,14 @@ struct taskstats {
 	__u64	swapin_count;
 	__u64	swapin_delay_total;	/* swapin page fault wait*/
 

Quoted 4 lines

include/linux/taskstats.h +1 -2

-	__u64	cpu_run_total;		/* cpu running time
-					 * no count available/provided */
+	__u64	cpu_run_real_total;	/* cpu "wall-clock" running time

include/linux/taskstats.h struct taskstats { · line 59

Quoted 10 lines

include/linux/taskstats.h +7 -0

+					 * Potentially accounts for cpu
+					 * virtualization, on some arches
+					 */
+	__u64	cpu_run_virtual_total;	/* cpu "virtual" running time
+					 * Uses time intervals as seen by
+					 * the kernel
+					 */
 };
 
 

Again, the reader of this struct wants to know what the atomicity rules are.

include/linux/taskstats.h

Quoted 5 lines

include/linux/taskstats.h +1 -0

+	d->cpu_run_real_total = (tmp < (nsec_t)d->cpu_run_real_total)? 0: tmp;

	lval = expr1 ? expr2 : expr3;

include/linux/taskstats.h

Quoted 6 lines

include/linux/taskstats.h +2 -0

+	tmp = (nsec_t)d->cpu_run_virtual_total
+		+ (nsec_t)jiffies_to_usecs(t3) * 1000;

umm, Linux doesn't have nsec_t any more.

Hi, Andrew

Quoted 13 lines · Andrew Morton

Quoted 10 lines · 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.

Quoted 10 lines

kernel/taskstats.c line 279

Quoted 7 lines

kernel/taskstats.c new file +5 -0

+
+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.

Quoted 25 lines · Balbir Singh

Quoted 22 lines · Andrew Morton

Quoted 4 lines · Shailabh Nagar

Could you please include the following delay accounting patches in -mm ?

I'm at a loss to evaluate the suitability of this work, really. I always am when accounting patches come along.

There are various people and various groups working on various different things and there appears to be no coordination and little commonality of aims. I worry that picking one submission basically at random will provide nothing which the other groups can work on to build up their feature.

On the other hand, we don't want to do nothing until some uber-grand all-singing, all-dancing statistics-gathering infrastructure comes along.

So I'm a bit stuck. What I would like to see happen is that there be some coordination between the various stakeholders, and some vague plan which they're all happy with as a basis for the eventual grand solution.

Quoted 8 lines

Quoted 8 lines

We already have various bits and pieces of statistics gathering in the kernel and it's already a bit ad-hoc. Adding more one-requirement-specific accounting code won't improve that situation.

But then, I said all this a year or two ago and nothing much has happened since then. It's not your fault, but it's a problem.

Quoted 4 lines

Quoted 4 lines

Perhaps a good starting point would be a one-page bullet-point-form
wishlist of all the accounting which people want to get out of the kernel,
and a description of what the kernel<->user interface should look like. Right now, I don't think we even have a picture of that.

Quoted 8 lines

Quoted 8 lines

We need a statistics maintainer, too, to pull together the plan, coordinate, push things forwards. The first step would be to identify the stakeholders, come up with that page of bullet-points.

Then again, maybe the right thing to do is to keep adding low-impact requirement-specific statistics patches as they come along.

Quoted 18 lines

Quoted 7 lines

But if we're
going to do it that way, we need an up-front reason for doing so, and I don't know what that would be.

See my problem?

One of the issues we have tried to address is the ability to provide some form of a common ground for all the statistics to co-exist. Various methods were discussed for exchanging data between kernel and user space, genetlink was suggested often and the clear winner.

To that end, we have created a taskstats.c file. Any subsystem wanting to add their statistics and sending it to user space can add their own types by extending taskstats.c (changing the version number) and creating their own types using genetlink. They will have to do the following

Quoted 10 lines

1. Add statistics gathering in their own subsystem
2. Add a type to taskstats.c, extend it and use data from (1) and send it to user space.

The data from various subsystems can co-exist. I feel that this could serve as the basic common infrastructure to begin with and refined later (depending on the needs of other people).

Sounds fine to me, but I'm not a stakeholder.

Trolling back through lse-tech gives us:

pnotify:
Erik Jacobson <[email protected]>

CSA accounting/PAGG/JOB:
Jay Lan <[email protected]>
Limin Gu <[email protected]>

per-process IO statistics:
Levent Serinol <[email protected]>

ELSA:
Guillaume Thouvenin <[email protected]>

per-cpu time statistics:
Erich Focht <[email protected]>

Scalable statistics counters with /proc reporting:
Ravikiran G Thirumalai <[email protected]>
(Kiran feft IBM, but presumably the requirement lives on)

There was a long thread "A common layer for Accounting packages". Did it come to a conclusion?

Anyway, if mostly everyone is mostly happy with what you propose then that it good news.

Quoted 3 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Shailabh" == Shailabh Nagar <[email protected]> writes:

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

"Shailabh" == Shailabh Nagar <[email protected]> writes:

Shailabh> To this list we can also add

Shailabh> Microstate accounting Peter Chubb
Shailabh> <[email protected]> I don't know if Peter is still Shailabh> interested in pursuing this or it was rejected.

It's still maintained in a sporadic sort of way --- I update it when either I need it for something, or someone's downloaded it and asks why it doesn't work agains kernel X.Y.Z. I see a few downloads a month.

My microstate accounting patch overlaps the delay accounting patch quite a lot in functionality, (but I thnk mine is cleaner except for interrupt time accounting... which the delay accounting patch doesn't do. I wanted to know how much time a thread *really* had on the processor, subtracting off the time spent in interrupt handlers for some other process).

Following Andrew's suggestion, here's my quick overview of the various other accounting packages that have been proposed on lse-tech with a focus on whether they can utilize the netlink-based taskstats interface being proposed by the delay accounting patches.

Please note that unification of statistics *collection* is not being discussed since that kind of merger can be done as these patches get accepted, if at all, into the kernel. To try and unify right away would hold every patch (esp. delay accounting !) hostage to the problems in every other patch unnecessarily. As long as the interface can be unified, the merger of the collection bits can always happen without affecting user space.

Stakeholders of each of these patches, on cc, are requested to please correct any misunderstandings of what their patches do.

Also, please comment on the observations about their patch's ability to use the netlink-based taskstats interface, code for which was posted at

http://www.uwsg.indiana.edu/hypermail/linux/kernel/0603.3/1787.html

5. Enhanced Linux System Accounting (Guillaume Thouvenine)

----------------------------------------------------------

- Group tasks at a user level into "jobs" and aggregate, at user level, per-task statistics collected by CSA and/or BSD process accounting.

- ELSA does not introduce any new requirement for either collection or export of statistics from the kernel. It can use either BSD and/or CSA's method of using an accounting file.

- ELSA needs notification of forks and exits which it can already get through the process events connector in the kernel.

Hence ELSA's needs are either met by the kernel today or are a strict subset of CSA (since BSD accounting is already there).

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

"Shailabh" == Shailabh Nagar <[email protected]> writes:

Quoted 6 lines

It's still maintained in a sporadic sort of way --- I update it
when either I need it for something, or someone's downloaded it and asks why it doesn't work agains kernel X.Y.Z. I see a few downloads a month.

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

"Shailabh" == Shailabh Nagar <[email protected]> writes:

Shailabh> Peter Chubb wrote:
(microstate accounting patch)

Quoted 8 lines

Quoted 8 lines

It's still maintained in a sporadic sort of way --- I update it when either I need it for something, or someone's downloaded it and asks why it doesn't work agains kernel X.Y.Z. I see a few downloads a month.

Shailabh> So do you intend to pursue acceptance ? If so, do you think Shailabh> the netlink-based taskstats interface provided by the delay Shailabh> accounting patches could be an acceptable substitute for the Shailabh> interfaces you had (from an old lkml post, they appear to be Shailabh> /proc/tgid/msa and a syscall based one) ?

I'd have to take a close look.

The syscall interface is modelled on
getrusage(), and only lets you get your own or your children's data; I'm not too worried about trashing it, as it should be possible to emulate in terms of netlink (albeit at a cost; system calls are relatively cheap)

/proc/<pid>/task/<tid>/msa lets you get at anything you own. I use awk scripts to process the msa file in /proc/... and pipe it into gnuplot at n second intervals; a netlink interface would need to have an auxiliary program to read it and then squirt it into the scripts, I think --- or is there a way to get ASCII out on demand?

I quite often
use cat to do quick checks on whats going on too --- so overall I think the /proc interface is desirable.

Quoted 25 lines · On Fri, 31 Mar 2006 01:42:28 -0500 Shailabh Nagar <[email protected]> wrote:

Following Andrew's suggestion, here's my quick overview of the various other accounting packages that have been proposed on lse-tech with a focus on whether they can utilize the netlink-based taskstats interface being proposed by the delay accounting patches.

Please note that unification of statistics *collection* is not being discussed since that kind of merger can be done as these patches get accepted, if at all, into the kernel. To try and unify right away would hold every patch (esp. delay accounting !) hostage to the problems in every other patch unnecessarily. As long as the interface can be unified, the merger of the collection bits can always happen without affecting user space.

Stakeholders of each of these patches, on cc, are requested to please correct any misunderstandings of what their patches do.

Also, please comment on the observations about their patch's ability to use the netlink-based taskstats interface, code for which was posted at

http://www.uwsg.indiana.edu/hypermail/linux/kernel/0603.3/1787.html

[...]

Quoted 3 lines

5. Enhanced Linux System Accounting (Guillaume Thouvenine)

                                                ^^^^^^^^^^
                                                Thouvenin

Quoted 10 lines

----------------------------------------------------------

- Group tasks at a user level into "jobs" and aggregate, at user level, per-task statistics collected by CSA and/or BSD process accounting.

- ELSA does not introduce any new requirement for either collection or export of statistics from the kernel. It can use either BSD and/or CSA's method of using an accounting file.

Quoted 7 lines

- ELSA needs notification of forks and exits which it can already get through the process events connector in the kernel.

Hence ELSA's needs are either met by the kernel today or are a strict subset of CSA (since BSD accounting is already there).

The overview is very interesting and you have a very good comprehension of ELSA. As you said ELSA is a group tasks at a user level and everything is already in the kernel so your patches don't generate troubles to ELSA. As you said in the delay accounting documentation, delay statistics can also be collected for all tasks and a tool like ELSA can aggregate results for groups of processes.

Chears,
Guillaume

Quoted 15 lines

Following Andrew's suggestion, here's my quick overview of the various other accounting packages that have been proposed on lse-tech with a focus on whether they can utilize the netlink-based taskstats interface being proposed by the delay accounting patches.

Please note that unification of statistics *collection* is not being discussed since that kind of merger can be done as these patches get accepted, if at all, into the kernel. To try and unify right away would hold every patch (esp. delay accounting !) hostage to the problems in every other patch unnecessarily. As long as the interface can be unified, the merger of the collection bits can always happen without affecting user space.

Quoted 2 lines

Stakeholders of each of these patches, on cc, are requested to please correct any misunderstandings of what their patches do.

To me, data collection and formation before sending down to userspace is very important part. What this taskstats netlink interface does is just to provide an interface to send "already formatted" data to userspace. In other words, it will replace "writing accounting records to an accounting file" step currently performed in BSD accouting and in CSA. If i understand it correctly, you have delayacct.c sitting on top of taskstats interface, and all other accounting methods should build their own layer on top of taskstats as well. For example, potentially BSD acct.c can replace fput() (and other statements dealing with acctounting file) with this interface. Same for CSA.

This approach sounds right to me. Actually i am very glad that you made effort to provide a common ground here. Yet, this is only one step. I will apply your patchset on top of 2.6.16-mm to see what i get and give more feedback later.

This taskstats thing is much more complicated than what Guillaume used to have when he put up a prototype of doing ELSA over netlink. One confusing point is the struct taskstats. If it is to be used as the big data struct to contain all accounting data everybody needs (as Shailabh suggested on his CSA analysis section), then if at do_exit() every accounting methods are to be invoked to handle their netlink transmission (as currently implemented in delayed accounting), would it be a lot of overhead sending "grand data" too many times? Maybe each layer should just format data of their interest when invoked from do_exit, and then we do one call to genetlink to deliver formated struct taskstats data?

Also, as you pointed out, CSA only retrieve data at end of task
but delayed accounting needs to retrieve data during the process.
So, i think we need more than one record types, not just the
struct taskstats, so that the user space delayed accounting application can specify to get only delayed accounting record.

Honestly, this taskstats.c layer looks more like something
extracted from delayed accounting than a carefully designed common ground to me. Patch 8/8 is about documentation of delayed accounting than the common ground for various accounting methods.
Can you please present us a documentation of design concept of
such a common layer? That would help me. I guess i also need to catch up on genetlink to better understand taskstats code.

Regards.
- jay

I made two feedback on 3/31 only to see them bounced back over the weekend. :(

Here was my first feedback:

Quoted 24 lines · Shailabh Nagar

Quoted 15 lines

Following Andrew's suggestion, here's my quick overview of the various other accounting packages that have been proposed on lse-tech with a focus on whether they can utilize the netlink-based taskstats interface being proposed by the delay accounting patches.

Please note that unification of statistics *collection* is not being discussed since that kind of merger can be done as these patches get accepted, if at all, into the kernel. To try and unify right away would hold every patch (esp. delay accounting !) hostage to the problems in every other patch unnecessarily. As long as the interface can be unified, the merger of the collection bits can always happen without affecting user space.

Quoted 2 lines

Stakeholders of each of these patches, on cc, are requested to please correct any misunderstandings of what their patches do.

To me, data collection and formation before sending down to userspace is very important part. What this taskstats netlink interface does is just to provide an interface to send "already formatted" data to userspace. In other words, it will replace "writing accounting records to an accounting file" step currently performed in BSD accouting and in CSA.

If i understand it correctly,

Quoted 3 lines

you have delayacct.c sitting on top of taskstats interface, and all other accounting methods should build their own layer on top of taskstats as well.

For example, potentially BSD acct.c can replace

Quoted 2 lines

fput() (and other statements dealing with acctounting file) with this interface. Same for CSA.

Quoted 5 lines

This approach sounds right to me. Actually i am very glad that you made effort to provide a common ground here. Yet, this is only one step. I will apply your patchset on top of 2.6.16-mm to see what i get and give more feedback later.

And, here is the second one:

Quoted 13 lines

This taskstats thing is much more complicated than what Guillaume used to have when he put up a prototype of doing ELSA over netlink. One confusing point is the struct taskstats. If it is to be used as the big data struct to contain all accounting data everybody needs (as Shailabh suggested on his CSA analysis section), then if at do_exit() every accounting methods are to be invoked to handle their netlink transmission (as currently implemented in delayed accounting), would it be a lot of overhead sending "grand data" too many times? Maybe each layer should just format data of their interest when invoked from do_exit, and then we do one call to genetlink to deliver formated struct taskstats data?

Quoted 5 lines

Also, as you pointed out, CSA only retrieve data at end of task
but delayed accounting needs to retrieve data during the process.
So, i think we need more than one record types, not just the
struct taskstats, so that the user space delayed accounting application can specify to get only delayed accounting record.

Quoted 2 lines

Honestly, this taskstats.c layer looks more like something extracted from delayed accounting than a carefully designed common ground to me.

Quoted 2 lines

Patch 8/8 is about documentation of delayed
accounting than the common ground for various accounting methods.

Quoted 2 lines

Can you please present us a documentation of design concept of such a common layer ?

Quoted 1 line

That would help me. I guess i also need to catch up on genetlink to better understand taskstats code.

Quoted 4 lines

Regards.
- jay

Jay Lan wrote:

Quoted 13 lines

Quoted 11 lines

This taskstats thing is much more complicated than what Guillaume used to have when he put up a prototype of doing ELSA over netlink. One confusing point is the struct taskstats. If it is to be used as the big data struct to contain all accounting data everybody needs (as Shailabh suggested on his CSA analysis section), then if at do_exit() every accounting methods are to be invoked to handle their netlink transmission (as currently implemented in delayed accounting), would it be a lot of overhead sending "grand data" too many times? Maybe each layer should just format data of their interest when invoked from do_exit, and then we do one call to genetlink to deliver formated struct taskstats data?

Good idea. One can already do this in the code we submitted by adding functions similar to delayacct_add_tsk() within the fill_pid() and fill_tgid() parts of the taskstats code. Then the delayacct_tsk_exit() routine will serve as the "one call" to deliver formatted data.

However, using delayacct_tsk_exit (which does have delay accounting specific bits too) as the data delivery call isn't intuitive. So I'll separate out the taskstats_exit_pid as a separate call directly made within do_exit(). Will require some refactoring but it can be done.

Quoted 9 lines

Quoted 7 lines

Also, as you pointed out, CSA only retrieve data at end of task
but delayed accounting needs to retrieve data during the process.
So, i think we need more than one record types, not just the
struct taskstats, so that the user space delayed accounting application can specify to get only delayed accounting record.

A separate record type isn't needed, atleast for now. For delay accounting, the data obtained during a process' lifetime is the same as the one expected at the end. So by itself, it has no need to distinguish records generated during the lifetime and those generated after a process exits.

Yes, the additional fields added to the taskstats struct by CSA will be "unnecessary" for delay accounting users but they will have to be able to deal with that anyway (for the process exit records where CSA and delay will share a common exit record).

So creating a separate record structure for the "during lifetime" records trades off transmission of a larger structure (relatively cheap) vs. the added complexity of tracking two types of records. At this point, the tradeoff isn't worth it for us.

Quoted 4 lines

Quoted 2 lines

Honestly, this taskstats.c layer looks more like something extracted from delayed accounting than a carefully designed common ground to me.

If you have other specific suggestions about the interface and why it doesn't meet CSA's needs, we can work to fix them.

Quoted 4 lines

Quoted 2 lines

Patch 8/8 is about documentation of delayed
accounting than the common ground for various accounting methods.

True. Patch 8/8 was meant to document delay accounting alone. I'll extract the taskstats specific parts out.

Quoted 4 lines

Quoted 2 lines

Can you please present us a documentation of design concept of such a common layer ?

Well, the design is fairly straightforward and is probably apparent by now. A common per-task accounting structure called taskstats exists. Userspace can use a NETLINK_GENERIC interface to send queries for statistics of a particular pid or tgid during the lifetime of a process. Specifying the pid gives the stats for just that pid. Specifying the tgid returns the sum of stats for all threads of the tgid.

Userspace can also choose to open the NETLINK_GENERIC socket in multicast and
listen for per-pid and per-tgid statistics that are automatically sent from the kernel using a whenever a task exits. These stats are sent
whenever there is any listener on the genetlink socket. The per-pid and per-tgid
data are exactly the same as what you would get if a query could be done just before
a task exited. Sending the per-tgid data at the exit of each pid/tid is necessary since
there is no well-defined "tgid exit" point in the kernel (we do not define a thread group to
cease existence when the thread group leader exits...rather it ceases to exist when the
last thread of the thread group exits). Also, per-tgid accumalation is only done dynamically in the kernel, not maintained as a separate statistic (to avoid wasting time and space). So each time a tid from a tgid exits, one needs to collect and send the whole tgid's data in case userspace is trying to track the stats at a per-tgid level.

The statistic structure contents are documented in include/linux/taskstats.h and by the accounting subsystem which fills in the fields. Currently delay accounting is the only user so all the fields are of the form
XXX_count and XXX_delay_total

where the former is a count of number of values added in the latter. Latter is the cumulative "delay", in nanoseconds, seen by a pid waiting for the resource XXX. e.g. cpu_delay_total is the total time spent waiting for a cpu to run on, blkio_delay_total is the time spent waiting for sync block I/O to complete etc.

As more per-task accounting packages get added to the kernel, they can define additional fields following the instructions in include/linux/taskstats.h and define their own userspace utilities similar to getdelays.c
Querying for data during a task's lifetime is done completely independently by all the utilities (using unicast queries and replies) - responses to queries by one are not seen by the others. The stats sent on task exit are common and multicast to all listening utilities.

Will add this to a separate taskstats doc in Documentation/.

Quoted 3 lines

Quoted 1 line

That would help me. I guess i also need to catch up on genetlink to better understand taskstats code.

Please do so soon. The usage of genetlink for taskstats has gone through a detailed review by Jamal etc. so there shouldn't be any genetlink issues that are pertinent to the potential CSA usage of taskstats.

--Shailabh

Quoted 4 lines

Quoted 4 lines

Regards.
- jay

-------------------------------------------------------
This SF.Net email is sponsored by xPML, a groundbreaking scripting language that extends applications into web and mobile media. Attend the live webcast and join the prime developer group breaking into this new coding territory!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=110944&bid=241720&dat=121642
_______________________________________________
Lse-tech mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/lse-tech