add basic accounting fields to taskstats

PATCH In review Jay Lan 5 participants 14 messages Jul 31, 2006 – Aug 8, 2006 Taskstats statistics interface

Share & tools

Jay Lan

author

2006-07-31 19:20 UTC #0 lore raw

PATCH 1/3 add basic accounting fields to taskstats

This patch adds a few basic accounting fields to the taskstat struct and a bacct_add_tsk() routine to fill the data on exit.

Signed-off-by Jay Lan <[email protected]>

include/linux/taskstats.h +21 -2

Index: linux/include/linux/taskstats.h
===================================================================
@@ -2,6 +2,7 @@
  *
  * Copyright (C) Shailabh Nagar, IBM Corp. 2006
  *           (C) Balbir Singh,   IBM Corp. 2006
+ *           (C) Jay Lan,        SGI, 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

@@ -29,13 +30,18 @@
  *	c) add new fields after version comment; maintain 64-bit alignment
  */
 
-#define TASKSTATS_VERSION	1
+#define TASKSTATS_VERSION	2
+#define TASK_COMM_LEN		16
 
 struct taskstats {
 
 	/* Version 1 */
 	__u16	version;
-	__u16	padding[3];	/* Userspace should not interpret the padding
+	__u8	ac_flag;	/* Record flags */
+	__u8	ac_nice;	/* task_nice */
+	__u8	ac_sched;	/* Scheduling discipline */
+	__u8	ac_pad;
+	__u16	padding;	/* Userspace should not interpret the padding
 				 * field which can be replaced by useful
 				 * fields if struct taskstats is extended.
 				 */

@@ -88,6 +94,19 @@ struct taskstats {
 	__u64	cpu_run_virtual_total;
 	/* Delay accounting fields end */
 	/* version 1 ends here */
+
+	/* Basic Accounting Fields start */
+	char	ac_comm[TASK_COMM_LEN];	/* Command name */
+	__u32	ac_exitcode;		/* Exit status */
+	__u32	ac_uid;			/* User ID */
+	__u32	ac_gid;			/* Group ID */
+	__u32	ac_pid;			/* Process ID */
+	__u32	ac_ppid;		/* Parent process ID */
+	__u32	ac_btime;		/* Begin time [sec sinec 1970] */
+	__u64	ac_etime;		/* Elapsed time [usec] */
+	__u64	ac_utime;		/* User CPU time [usec] */
+	__u64	ac_stime;		/* SYstem CPU time [usec] */
+	/* Basic Accounting Fields end */
 };

kernel/taskstats.c +52 -0

76 lines not on this page · raw message

Shailabh Nagar

2006-07-31 20:23 UTC #1 lore raw

Quoted 29 lines · Jay Lan

This patch adds a few basic accounting fields to the taskstat struct and a bacct_add_tsk() routine to fill the data on exit.
Signed-off-by Jay Lan <[email protected]>
------------------------------------------------------------------------
Index: linux/include/linux/taskstats.h
===================================================================
include/linux/taskstats.h +3 -1
@@ -2,6 +2,7 @@
  *
  * Copyright (C) Shailabh Nagar, IBM Corp. 2006
  *           (C) Balbir Singh,   IBM Corp. 2006
+ *           (C) Jay Lan,        SGI, 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
@@ -29,13 +30,18 @@
  *	c) add new fields after version comment; maintain 64-bit alignment
  */
 
-#define TASKSTATS_VERSION	1
+#define TASKSTATS_VERSION	2
+#define TASK_COMM_LEN		16

include/linux/taskstats.h line 43

Quoted 10 lines

include/linux/taskstats.h +4 -1

 
 struct taskstats {
 
 	/* Version 1 */
 	__u16	version;
-	__u16	padding[3];	/* Userspace should not interpret the padding
+	__u8	ac_flag;	/* Record flags */
+	__u8	ac_nice;	/* task_nice */
+	__u8	ac_sched;	/* Scheduling discipline */
+	__u8	ac_pad;

[Minor comment] The choice of fields to fill the padding is visually separate from the delay accounting fields which follow. Can a single field of the same length, say ac_uid, be put here instead ?

Also, is the ac_ prefix needed for the common fields like comm, uid, gid, pid that are pretty much coming straight out of task_struct ?

include/linux/taskstats.h line 44

Quoted 1 line

include/linux/taskstats.h +1 -0

+	__u16	padding;	/* Userspace should not interpret the padding

Combine the padding's ?

Quoted 6 lines

include/linux/taskstats.h +0 -0

 				 * field which can be replaced by useful
 				 * fields if struct taskstats is extended.
 				 */

@@ -88,6 +94,19 @@ struct taskstats {
 	__u64	cpu_run_virtual_total;
 	/* Delay accounting fields end */

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

Quoted 10 lines

include/linux/taskstats.h +9 -0

 	/* version 1 ends here */
+
+	/* Basic Accounting Fields start */
+	char	ac_comm[TASK_COMM_LEN];	/* Command name */
+	__u32	ac_exitcode;		/* Exit status */
+	__u32	ac_uid;			/* User ID */
+	__u32	ac_gid;			/* Group ID */
+	__u32	ac_pid;			/* Process ID */
+	__u32	ac_ppid;		/* Parent process ID */
+	__u32	ac_btime;		/* Begin time [sec sinec 1970] */

s/sinec/since

Quoted 42 lines

include/linux/taskstats.h +4 -0

+	__u64	ac_etime;		/* Elapsed time [usec] */
+	__u64	ac_utime;		/* User CPU time [usec] */
+	__u64	ac_stime;		/* SYstem CPU time [usec] */
+	/* Basic Accounting Fields end */
 };

Index: linux/kernel/taskstats.c
===================================================================

kernel/taskstats.c +13 -0

@@ -3,6 +3,7 @@
  *
  * Copyright (C) Shailabh Nagar, IBM Corp. 2006
  *           (C) Balbir Singh,   IBM Corp. 2006
+ *           (C) Jay Lan,        SGI, 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,6 +19,7 @@
 
 #include <linux/kernel.h>
 #include <linux/taskstats_kern.h>
+#include <linux/acct.h>
 #include <linux/delayacct.h>
 #include <linux/cpumask.h>
 #include <linux/percpu.h>

@@ -172,6 +174,53 @@ static void send_cpu_listeners(struct sk
 	up_write(&listeners->sem);
 }
 
+
+#define USEC_PER_TICK	(USEC_PER_SEC/HZ)
+/*
+ * fill in basic accounting fields
+ */
+static void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk)
+{
+	u64	run_time;
+	struct timespec uptime;
+
+	/* calculate run_time in nsec */

kernel/taskstats.c static void send_cpu_listeners(struct sk · line 197

Quoted 10 lines

kernel/taskstats.c +8 -0

+	do_posix_clock_monotonic_gettime(&uptime);
 
 
+	run_time = (u64)uptime.tv_sec*NSEC_PER_SEC + uptime.tv_nsec;
+	run_time -= (u64)current->group_leader->start_time.tv_sec * NSEC_PER_SEC
+			+ current->group_leader->start_time.tv_nsec;
+	do_div(run_time, NSEC_PER_USEC);	/* rebase run_time to usec */
+	stats->ac_etime = run_time;
+	do_div(run_time, USEC_PER_SEC);		/* rebase run_time to sec */
+	stats->ac_btime = xtime.tv_sec - run_time;

Above chunk can be written more succintly using the new timespec_sub function added.

ts = timespec_sub(&current->group_leader->start_time, &uptime); stats->ac_etime = timespec_to_ns(&ts); stats->ac_btime = xtime.tv_sec - ts.tv_sec;

Also, if there's any chance of run_time going negative due to overflow, that needs to be handled in both original and succinct code (in latter you can check for ts->tv_sec being negative).

Quoted 32 lines

kernel/taskstats.c +28 -0

+	if (thread_group_leader(tsk)) {
+		stats->ac_exitcode = tsk->exit_code;
+		if (tsk->flags & PF_FORKNOEXEC)
+			stats->ac_flag |= AFORK;
+	}
+	if (tsk->flags & PF_SUPERPRIV)
+		stats->ac_flag |= ASU;
+	if (tsk->flags & PF_DUMPCORE)
+		stats->ac_flag |= ACORE;
+	if (tsk->flags & PF_SIGNALED)
+		stats->ac_flag |= AXSIG;
+	stats->ac_nice	= task_nice(tsk);
+	stats->ac_sched	= tsk->policy;
+	stats->ac_uid	= tsk->uid;
+	stats->ac_gid	= tsk->gid;
+	stats->ac_pid	= tsk->pid;
+	stats->ac_ppid	= (tsk->parent) ? tsk->parent->pid : 0;
+	stats->ac_utime	= tsk->utime * USEC_PER_TICK;
+	stats->ac_stime	= tsk->stime * USEC_PER_TICK;
+	/* Each process gets a minimum of a half tick cpu time */
+	if ((stats->ac_utime == 0) && (stats->ac_stime == 0)) {
+		stats->ac_stime = USEC_PER_TICK/2;
+	}
+
+	strncpy(stats->ac_comm, tsk->comm, sizeof(stats->ac_comm));
+}
+
+
 static int fill_pid(pid_t pid, struct task_struct *pidtsk,

kernel/taskstats.c +0 -0

 		struct taskstats *stats)
 {

kernel/taskstats.c static int fill_pid(pid_t pid, struct ta · line 254

Quoted 6 lines

kernel/taskstats.c +3 -0

@@ -200,6 +249,9 @@ static int fill_pid(pid_t pid, struct ta
 	delayacct_add_tsk(stats, tsk);
 	stats->version = TASKSTATS_VERSION;
 
+	/* fill in basic acct fields */
+	bacct_add_tsk(stats, tsk);
+

Should be added before stats->version is filled since that is a generic field, not delay accounting specific.

Quoted 4 lines

kernel/taskstats.c +0 -0

 	/* Define err: label here if needed */
 	put_task_struct(tsk);
 	return rc;

Jay Lan

author

2006-07-31 21:50 UTC #2 lore raw

Quoted 34 lines · Shailabh Nagar

Quoted 32 lines · Jay Lan
This patch adds a few basic accounting fields to the taskstat struct and a bacct_add_tsk() routine to fill the data on exit.
Signed-off-by Jay Lan <[email protected]>
------------------------------------------------------------------------
Index: linux/include/linux/taskstats.h
===================================================================
include/linux/taskstats.h +3 -1
@@ -2,6 +2,7 @@
 *
 * Copyright (C) Shailabh Nagar, IBM Corp. 2006
 *           (C) Balbir Singh,   IBM Corp. 2006
+ *           (C) Jay Lan,        SGI, 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
@@ -29,13 +30,18 @@
 *	c) add new fields after version comment; maintain 64-bit alignment
 */
 
-#define TASKSTATS_VERSION	1
+#define TASKSTATS_VERSION	2
+#define TASK_COMM_LEN		16
 
 struct taskstats {
 

Quoted 12 lines

include/linux/taskstats.h line 43
Quoted 7 lines
include/linux/taskstats.h +4 -1
 	/* Version 1 */
 	__u16	version;
-	__u16	padding[3];	/* Userspace should not interpret the padding
+	__u8	ac_flag;	/* Record flags */
+	__u8	ac_nice;	/* task_nice */
+	__u8	ac_sched;	/* Scheduling discipline */
+	__u8	ac_pad;
[Minor comment] The choice of fields to fill the padding is visually separate from the delay accounting fields which follow. Can a single field of the same length, say ac_uid, be put here instead ?

You are right. I will replace those __u8 fields with the exitcode.

Quoted 4 lines

Also, is the ac_ prefix needed for the common fields like comm, uid, gid, pid that are pretty much coming straight out of task_struct ?

And the GNU accounting use the ac_ prefix as well...
I lean to keep ac_ prefix so that people can link those fields to those in the task_struct. However, i am fine with either way. Let's see if there is other opinions on the prefix.

Quoted 19 lines

include/linux/taskstats.h line 44

Quoted 3 lines

include/linux/taskstats.h +1 -0

+	__u16	padding;	/* Userspace should not interpret the padding

Combine the padding's ?

Quoted 11 lines

include/linux/taskstats.h +4 -0

 				 * field which can be replaced by useful
 				 * fields if struct taskstats is extended.
 				 */

@@ -88,6 +94,19 @@ struct taskstats {
 	__u64	cpu_run_virtual_total;
 	/* Delay accounting fields end */
 	/* version 1 ends here */
+
+	/* Basic Accounting Fields start */
+	char	ac_comm[TASK_COMM_LEN];	/* Command name */
+	__u32	ac_exitcode;		/* Exit status */

Quoted 8 lines

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

Quoted 5 lines

include/linux/taskstats.h +5 -0

+	__u32	ac_uid;			/* User ID */
+	__u32	ac_gid;			/* Group ID */
+	__u32	ac_pid;			/* Process ID */
+	__u32	ac_ppid;		/* Parent process ID */
+	__u32	ac_btime;		/* Begin time [sec sinec 1970] */

s/sinec/since

Good eyes! :)

Quoted 60 lines

Quoted 45 lines

include/linux/taskstats.h +4 -0

+	__u64	ac_etime;		/* Elapsed time [usec] */
+	__u64	ac_utime;		/* User CPU time [usec] */
+	__u64	ac_stime;		/* SYstem CPU time [usec] */
+	/* Basic Accounting Fields end */
 };

Index: linux/kernel/taskstats.c
===================================================================

kernel/taskstats.c +14 -0

@@ -3,6 +3,7 @@
 *
 * Copyright (C) Shailabh Nagar, IBM Corp. 2006
 *           (C) Balbir Singh,   IBM Corp. 2006
+ *           (C) Jay Lan,        SGI, 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,6 +19,7 @@
 
 #include <linux/kernel.h>
 #include <linux/taskstats_kern.h>
+#include <linux/acct.h>
 #include <linux/delayacct.h>
 #include <linux/cpumask.h>
 #include <linux/percpu.h>

@@ -172,6 +174,53 @@ static void send_cpu_listeners(struct sk
 	up_write(&listeners->sem);
 }
 
+
+#define USEC_PER_TICK	(USEC_PER_SEC/HZ)
+/*
+ * fill in basic accounting fields
+ */
+static void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk)
+{
+	u64	run_time;
+	struct timespec uptime;
+
+	/* calculate run_time in nsec */
+	do_posix_clock_monotonic_gettime(&uptime);

kernel/taskstats.c static void send_cpu_listeners(struct sk · line 198

Quoted 10 lines

kernel/taskstats.c +7 -0

 
 
 
+	run_time = (u64)uptime.tv_sec*NSEC_PER_SEC + uptime.tv_nsec;
+	run_time -= (u64)current->group_leader->start_time.tv_sec * NSEC_PER_SEC
+			+ current->group_leader->start_time.tv_nsec;
+	do_div(run_time, NSEC_PER_USEC);	/* rebase run_time to usec */
+	stats->ac_etime = run_time;
+	do_div(run_time, USEC_PER_SEC);		/* rebase run_time to sec */
+	stats->ac_btime = xtime.tv_sec - run_time;

Above chunk can be written more succintly using the new timespec_sub function added.

Quoted 1 line

ts = timespec_sub(&current->group_leader->start_time, &uptime);

It should be
ts = timespec_sub(&uptime, &current->group_leader->start_time);

Quoted 1 line

stats->ac_etime = timespec_to_ns(&ts);

No, ac_etime is in microseconds, not nanoseconds.

Quoted 1 line

stats->ac_btime = xtime.tv_sec - ts.tv_sec;

This is correct.

Quoted 4 lines

Also, if there's any chance of run_time going negative due to overflow, that needs to be handled in both original and succinct code (in latter you can check for ts->tv_sec being negative).

The type of timespec->tv_sec is of type 'int'. It will takes 68 years for an int to wrap wround. Most kernel code are not ready to deal with it yet.

Quoted 37 lines

kernel/taskstats.c +28 -0

+	if (thread_group_leader(tsk)) {
+		stats->ac_exitcode = tsk->exit_code;
+		if (tsk->flags & PF_FORKNOEXEC)
+			stats->ac_flag |= AFORK;
+	}
+	if (tsk->flags & PF_SUPERPRIV)
+		stats->ac_flag |= ASU;
+	if (tsk->flags & PF_DUMPCORE)
+		stats->ac_flag |= ACORE;
+	if (tsk->flags & PF_SIGNALED)
+		stats->ac_flag |= AXSIG;
+	stats->ac_nice	= task_nice(tsk);
+	stats->ac_sched	= tsk->policy;
+	stats->ac_uid	= tsk->uid;
+	stats->ac_gid	= tsk->gid;
+	stats->ac_pid	= tsk->pid;
+	stats->ac_ppid	= (tsk->parent) ? tsk->parent->pid : 0;
+	stats->ac_utime	= tsk->utime * USEC_PER_TICK;
+	stats->ac_stime	= tsk->stime * USEC_PER_TICK;
+	/* Each process gets a minimum of a half tick cpu time */
+	if ((stats->ac_utime == 0) && (stats->ac_stime == 0)) {
+		stats->ac_stime = USEC_PER_TICK/2;
+	}
+
+	strncpy(stats->ac_comm, tsk->comm, sizeof(stats->ac_comm));
+}
+
+

static int fill_pid(pid_t pid, struct task_struct *pidtsk,
struct taskstats *stats)
{

kernel/taskstats.c +0 -0

@@ -200,6 +249,9 @@ static int fill_pid(pid_t pid, struct ta
 	delayacct_add_tsk(stats, tsk);
 	stats->version = TASKSTATS_VERSION;

Quoted 7 lines

kernel/taskstats.c static int fill_pid(pid_t pid, struct ta · line 254
Quoted 3 lines
kernel/taskstats.c +3 -0
+	/* fill in basic acct fields */
+	bacct_add_tsk(stats, tsk);
+
Should be added before stats->version is filled since that is a generic field, not delay accounting specific.

Will do. Thanks!

Regards,
- jay

Quoted 8 lines

Quoted 6 lines

kernel/taskstats.c +0 -0

 	/* Define err: label here if needed */
 	put_task_struct(tsk);
 	return rc;

Balbir Singh

2006-08-01 14:43 UTC #3 lore raw

In reply to Jay Lan

include/linux/taskstats.h line 34

Quoted 5 lines · Jay Lan

include/linux/taskstats.h +2 -1

-#define TASKSTATS_VERSION	1
+#define TASKSTATS_VERSION	2
+#define TASK_COMM_LEN		16

We should find a way to keep this in sync with with the definition in linux/sched.h (won't we a warning if both this header and linux/sched.h are included together?)

Quoted 25 lines

+ * fill in basic accounting fields
+ */

kernel/taskstats.c +23 -0

+static void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk)
+{
+	u64	run_time;
+	struct timespec uptime;
+
+	/* calculate run_time in nsec */
+	do_posix_clock_monotonic_gettime(&uptime);
+	run_time = (u64)uptime.tv_sec*NSEC_PER_SEC + uptime.tv_nsec;
+	run_time -= (u64)current->group_leader->start_time.tv_sec * NSEC_PER_SEC
+			+ current->group_leader->start_time.tv_nsec;
+	do_div(run_time, NSEC_PER_USEC);	/* rebase run_time to usec */
+	stats->ac_etime = run_time;
+	do_div(run_time, USEC_PER_SEC);		/* rebase run_time to sec */
+	stats->ac_btime = xtime.tv_sec - run_time;
+	if (thread_group_leader(tsk)) {
+		stats->ac_exitcode = tsk->exit_code;
+		if (tsk->flags & PF_FORKNOEXEC)
+			stats->ac_flag |= AFORK;
+	}
+	if (tsk->flags & PF_SUPERPRIV)
+		stats->ac_flag |= ASU;
+	if (tsk->flags & PF_DUMPCORE)
+		stats->ac_flag |= ACORE;

kernel/taskstats.c static void send_cpu_listeners(struct sk · line 214

Quoted 10 lines

kernel/taskstats.c +10 -0

+	if (tsk->flags & PF_SIGNALED)
+		stats->ac_flag |= AXSIG;
+	stats->ac_nice	= task_nice(tsk);
+	stats->ac_sched	= tsk->policy;
+	stats->ac_uid	= tsk->uid;
+	stats->ac_gid	= tsk->gid;
+	stats->ac_pid	= tsk->pid;
+	stats->ac_ppid	= (tsk->parent) ? tsk->parent->pid : 0;
+	stats->ac_utime	= tsk->utime * USEC_PER_TICK;
+	stats->ac_stime	= tsk->stime * USEC_PER_TICK;

I think you should use the portable cputime_xxxx() API since tsk->utime and tsk->stime are of type cputime_t

kernel/taskstats.c static void send_cpu_listeners(struct sk · line 219

Quoted 5 lines

kernel/taskstats.c +5 -0

+	/* Each process gets a minimum of a half tick cpu time */
+	if ((stats->ac_utime == 0) && (stats->ac_stime == 0)) {
+		stats->ac_stime = USEC_PER_TICK/2;
+	}
+

This is confusing. Half tick does not make any sense from the scheduler view point (or am I missing something?), so why return half a tick to the user.

Signature · 3 lines

	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs

Jay Lan

author

2006-08-01 21:51 UTC #4 lore raw

Quoted 12 lines · Balbir Singh

include/linux/taskstats.h line 34
Quoted 5 lines · Jay Lan
include/linux/taskstats.h +2 -1
-#define TASKSTATS_VERSION    1
+#define TASKSTATS_VERSION    2
+#define TASK_COMM_LEN        16
We should find a way to keep this in sync with with the definition in linux/sched.h (won't we a warning if both this header and linux/sched.h are included together?)

I do not know how to sync it up. This header linux/taskstats.h is meant to be included by userspace programs. If an application happens to include linux/sched.h, which includes linux/time.h, the application will very likely have compilation errors because the "struct timespec" declaration in <linux/time.h> and <time.h> are conflicting.

The <linux/acct.h> defines it to
#define ACCT_COMM 16

I can change our define to TS_COMM_LEN with remakes saying it should be in sync with the TAKS_COMM_LEN defined in linux/sched.h.

If there is a better way, i am eager to know it.

Quoted 36 lines

+ * fill in basic accounting fields
+ */
+static void bacct_add_tsk(struct taskstats *stats, struct task_struct *tsk)

kernel/taskstats.c +8 -0

+{
+    u64    run_time;
+    struct timespec uptime;
+
+    /* calculate run_time in nsec */
+    do_posix_clock_monotonic_gettime(&uptime);
+    run_time = (u64)uptime.tv_sec*NSEC_PER_SEC + uptime.tv_nsec;
+    run_time -= (u64)current->group_leader->start_time.tv_sec *

NSEC_PER_SEC

kernel/taskstats.c +20 -0

+            + current->group_leader->start_time.tv_nsec;
+    do_div(run_time, NSEC_PER_USEC);    /* rebase run_time to usec */
+    stats->ac_etime = run_time;
+    do_div(run_time, USEC_PER_SEC);        /* rebase run_time to sec */
+    stats->ac_btime = xtime.tv_sec - run_time;
+    if (thread_group_leader(tsk)) {
+        stats->ac_exitcode = tsk->exit_code;
+        if (tsk->flags & PF_FORKNOEXEC)
+            stats->ac_flag |= AFORK;
+    }
+    if (tsk->flags & PF_SUPERPRIV)
+        stats->ac_flag |= ASU;
+    if (tsk->flags & PF_DUMPCORE)
+        stats->ac_flag |= ACORE;
+    if (tsk->flags & PF_SIGNALED)
+        stats->ac_flag |= AXSIG;
+    stats->ac_nice    = task_nice(tsk);
+    stats->ac_sched    = tsk->policy;
+    stats->ac_uid    = tsk->uid;
+    stats->ac_gid    = tsk->gid;

Quoted 8 lines

kernel/taskstats.c static void send_cpu_listeners(struct sk · line 214
Quoted 4 lines
kernel/taskstats.c +4 -0
+    stats->ac_pid    = tsk->pid;
+    stats->ac_ppid    = (tsk->parent) ? tsk->parent->pid : 0;
+    stats->ac_utime    = tsk->utime * USEC_PER_TICK;
+    stats->ac_stime    = tsk->stime * USEC_PER_TICK;
I think you should use the portable cputime_xxxx() API since tsk->utime and tsk->stime are of type cputime_t

Will fix it.

Quoted 9 lines

kernel/taskstats.c static void send_cpu_listeners(struct sk · line 219
Quoted 7 lines
kernel/taskstats.c +5 -0
+    /* Each process gets a minimum of a half tick cpu time */
+    if ((stats->ac_utime == 0) && (stats->ac_stime == 0)) {
+        stats->ac_stime = USEC_PER_TICK/2;
+    }
+

Quoted 3 lines

This is confusing. Half tick does not make any sense from the scheduler view point (or am I missing something?), so why return half a tick to the user.

It must be inherited from old code dated back to Cray UNICOS. I do not know if bad thing can happen if both utime and stime are less than 1 usec... I guess not. But i agree that half a tick does not make sense. To play safe, we can change it to 1 usec if both utime and stime are sub microsecond. What do you think?

Thanks,
- jay

Quoted 2 lines

Shailabh Nagar

2006-08-02 15:32 UTC #5 lore raw

Quoted 24 lines · Jay Lan

Quoted 13 lines · Balbir Singh
include/linux/taskstats.h line 34
Quoted 5 lines · Jay Lan
include/linux/taskstats.h +2 -1
-#define TASKSTATS_VERSION    1
+#define TASKSTATS_VERSION    2
+#define TASK_COMM_LEN        16
We should find a way to keep this in sync with with the definition in linux/sched.h (won't we a warning if both this header and linux/sched.h are included together?)
I do not know how to sync it up. This header linux/taskstats.h is meant to be included by userspace programs. If an application happens to include linux/sched.h, which includes linux/time.h, the application will very likely have compilation errors because the "struct timespec" declaration in <linux/time.h> and <time.h> are conflicting.

Quoted 5 lines

The <linux/acct.h> defines it to
#define ACCT_COMM 16
I can change our define to TS_COMM_LEN with remakes saying it should be in sync with the TAKS_COMM_LEN defined in linux/sched.h.

This seems like a good enough way to do it. There's no real need for the taskstats comm length to remain exactly in sync with the task struct's comm length (by way of trying to include sched.h etc.) though avoiding the compile error by renaming is desirable as Balbir pointed out.

Moreover, TASK_COMM_LEN in linux/sched.h isn't likely to change much - if it increases and csa_acct users also really need the extra info provided, taskstats can always be changed and version bumped up. If the size decreases there's no harm done (strncpy should be sufficient protection).

--Shailabh

Balbir Singh

2006-08-02 17:17 UTC #6 lore raw

Quoted 36 lines · Shailabh Nagar

Quoted 24 lines · Jay Lan
Quoted 13 lines · Balbir Singh
include/linux/taskstats.h line 34
Quoted 5 lines · Jay Lan
include/linux/taskstats.h +2 -1
-#define TASKSTATS_VERSION    1
+#define TASKSTATS_VERSION    2
+#define TASK_COMM_LEN        16
We should find a way to keep this in sync with with the definition in linux/sched.h (won't we a warning if both this header and linux/sched.h are included together?)
I do not know how to sync it up. This header linux/taskstats.h is meant to be included by userspace programs. If an application happens to include linux/sched.h, which includes linux/time.h, the application will very likely have compilation errors because the "struct timespec" declaration in <linux/time.h> and <time.h> are conflicting.
Quoted 5 lines
The <linux/acct.h> defines it to
#define ACCT_COMM 16
I can change our define to TS_COMM_LEN with remakes saying it should be in sync with the TAKS_COMM_LEN defined in linux/sched.h.
This seems like a good enough way to do it. There's no real need for the taskstats comm length to remain exactly in sync with the task struct's comm length (by way of trying to include sched.h etc.) though avoiding the compile error by renaming is desirable as Balbir pointed out.

Quoted 7 lines

Moreover, TASK_COMM_LEN in linux/sched.h isn't likely to change much - if it increases and csa_acct users also really need the extra info provided, taskstats can always be changed and version bumped up. If the size decreases there's no harm done (strncpy should be sufficient protection).
--Shailabh

I am not sure if there is a version of BUG_ON() for compile time asserts. Basically, if we have an infrastructure of the form

/*
* From C/C++ users journal November 2004
*/

#define STATIC_BUG_ON(e) 	\
	switch (0) {		\
	case  0:		\
	case (e):		\
		;		\
	}

Then the STATIC_BUG_ON() can catch as shown below.

#define TASK_COMM_LEN 16
#define T_COMM_LEN 20

int
main(void)

{
	STATIC_BUG_ON(TASK_COMM_LEN == T_COMM_LEN);
}

STATIC_BUG_ON gives the following warning

bug_on_c.c: In function `main':
bug_on_c.c:19: duplicate case value
bug_on_c.c:19: previously used here

but with T_COMM_LEN set to 16

It compiles without any errors, the code generated also looks like it has no overhead

int
main(void)
{

 8048310:       55                      push   %ebp
 8048311:       89 e5                   mov    %esp,%ebp
 8048313:       83 ec 08                sub    $0x8,%esp
 8048316:       83 e4 f0                and    $0xfffffff0,%esp
        STATIC_BUG_ON(TASK_COMM_LEN == T_COMM_LEN);
}
 8048319:       c9                      leave
 804831a:       c3                      ret
 804831b:       90                      nop

Assuming such infrastructure is available, you could then do

#ifdef __KERNEL__
#include <linux/sched.h>
#define TS_COMM_LEN 16
STATIC_BUG_ON (TS_COMM_LEN == TASK_COMM_LEN);
#endif

Comments?

Signature · 3 lines

	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs

Shailabh Nagar

2006-08-02 17:38 UTC #7 lore raw

Quoted 53 lines · Balbir Singh

I am not sure if there is a version of BUG_ON() for compile time asserts. Basically, if we have an infrastructure of the form
/*
* From C/C++ users journal November 2004
*/
#define STATIC_BUG_ON(e)     \
    switch (0) {        \
    case  0:        \
    case (e):        \
        ;        \
    }
Then the STATIC_BUG_ON() can catch as shown below.
#define TASK_COMM_LEN 16
#define T_COMM_LEN 20
int
main(void)
{
    STATIC_BUG_ON(TASK_COMM_LEN == T_COMM_LEN);
}
STATIC_BUG_ON gives the following warning
bug_on_c.c: In function `main':
bug_on_c.c:19: duplicate case value
bug_on_c.c:19: previously used here
but with T_COMM_LEN set to 16
It compiles without any errors, the code generated also looks like it has no overhead
int
main(void)
{
 8048310:       55                      push   %ebp
 8048311:       89 e5                   mov    %esp,%ebp
 8048313:       83 ec 08                sub    $0x8,%esp
 8048316:       83 e4 f0                and    $0xfffffff0,%esp
        STATIC_BUG_ON(TASK_COMM_LEN == T_COMM_LEN);
}
 8048319:       c9                      leave
 804831a:       c3                      ret
 804831b:       90                      nop
Assuming such infrastructure is available, you could then do

Quoted 8 lines

#ifdef __KERNEL__
#include <linux/sched.h>
#define TS_COMM_LEN 16
STATIC_BUG_ON (TS_COMM_LEN == TASK_COMM_LEN);
#endif
Comments?

Neat trick !
Perhaps STATIC_WARNING is a more appropriate name but something like this for general use may be good.

--Shailabh

Andrew Morton

2006-08-02 21:09 UTC #8 lore raw

In reply to Balbir Singh

Quoted 2 lines · On Wed, 02 Aug 2006 22:47:07 +0530 Balbir Singh <[email protected]> wrote:

I am not sure if there is a version of BUG_ON() for compile time asserts

We have BUILD_BUG_ON()

Jay Lan

2006-08-02 23:47 UTC #9 lore raw

Quoted 10 lines · Andrew Morton

Quoted 3 lines · On Wed, 02 Aug 2006 22:47:07 +0530 Balbir Singh <[email protected]> wrote:
I am not sure if there is a version of BUG_ON() for compile time asserts
We have BUILD_BUG_ON()

BUILD_BUG_ON() is a statement. I will do that inside bacct_add_tsk().

Thanks,
- jay

Balbir Singh

2006-08-03 03:03 UTC #10 lore raw

In reply to Andrew Morton

Quoted 7 lines · Andrew Morton

Quoted 2 lines · On Wed, 02 Aug 2006 22:47:07 +0530 Balbir Singh <[email protected]> wrote:
I am not sure if there is a version of BUG_ON() for compile time asserts
We have BUILD_BUG_ON()

Thanks, I remember that we had something similar, but was lost looking for it in asm-*/bug.h.

Signature · 3 lines

	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs

Jay Lan

author

2006-08-07 21:23 UTC #11 lore raw

In reply to Jay Lan

Jay Lan wrote:

[snip]

Quoted 15 lines

Quoted 10 lines
kernel/taskstats.c static void send_cpu_listeners(struct sk · line 219
Quoted 7 lines
kernel/taskstats.c +5 -0
+    /* Each process gets a minimum of a half tick cpu time */
+    if ((stats->ac_utime == 0) && (stats->ac_stime == 0)) {
+        stats->ac_stime = USEC_PER_TICK/2;
+    }
+
Quoted 3 lines
This is confusing. Half tick does not make any sense from the scheduler view point (or am I missing something?), so why return half a tick to the user.

Quoted 6 lines

It must be inherited from old code dated back to Cray UNICOS. I do not know if bad thing can happen if both utime and stime are less than 1 usec... I guess not. But i agree that half a tick does not make sense. To play safe, we can change it to 1 usec if both utime and stime are sub microsecond. What do you think?

Hi Balbir,

I figured this out. The tsk->stime (and utime as well) are charged by 1 tick (or cputime) from the timer interrupt handler through update_process_times->account_{user,system}_time.

The clock resolution is a tick. Any short process less than 1 tick will the counter being 0. It can be from 0 to 0.99999... tick. A half tick is the average value.

I think it makes more sense to assign a half tick than assign 1 usec to the stime. What do you think? Certainly the code need better explanation.

Regards,
- jay

[snip]

Balbir Singh

2006-08-08 05:22 UTC #12 lore raw

Quoted 28 lines · Jay Lan

Jay Lan wrote:
[snip]
Quoted 15 lines
Quoted 10 lines
kernel/taskstats.c static void send_cpu_listeners(struct sk · line 219
Quoted 7 lines
kernel/taskstats.c +5 -0
+    /* Each process gets a minimum of a half tick cpu time */
+    if ((stats->ac_utime == 0) && (stats->ac_stime == 0)) {
+        stats->ac_stime = USEC_PER_TICK/2;
+    }
+
Quoted 3 lines
This is confusing. Half tick does not make any sense from the scheduler view point (or am I missing something?), so why return half a tick to the user.
Quoted 6 lines
It must be inherited from old code dated back to Cray UNICOS. I do not know if bad thing can happen if both utime and stime are less than 1 usec... I guess not. But i agree that half a tick does not make sense. To play safe, we can change it to 1 usec if both utime and stime are sub microsecond. What do you think?
Hi Balbir,

Quoted 8 lines

I figured this out. The tsk->stime (and utime as well) are charged by 1 tick (or cputime) from the timer interrupt handler through update_process_times->account_{user,system}_time.
The clock resolution is a tick. Any short process less than 1 tick will the counter being 0. It can be from 0 to 0.99999... tick. A half tick is the average value.

But the scheduling happens in the granularity of a tick, so the minimum each task gets is a tick.

Quoted 4 lines

I think it makes more sense to assign a half tick than assign 1 usec to the stime. What do you think? Certainly the code need better explanation.

Can't we leave these values as zero in case both stime and utime are zero.

Quoted 5 lines

Regards,
- jay
[snip]

Signature · 4 lines

	Warm Regards,
	Balbir Singh,
	Linux Technology Center,
	IBM Software Labs

Jay Lan

2006-08-08 16:40 UTC #13 lore raw

Quoted 41 lines · Balbir Singh

Quoted 28 lines · Jay Lan
Jay Lan wrote:
[snip]
Quoted 15 lines
Quoted 10 lines
kernel/taskstats.c static void send_cpu_listeners(struct sk · line 219
Quoted 7 lines
kernel/taskstats.c +5 -0
+    /* Each process gets a minimum of a half tick cpu time */
+    if ((stats->ac_utime == 0) && (stats->ac_stime == 0)) {
+        stats->ac_stime = USEC_PER_TICK/2;
+    }
+
Quoted 3 lines
This is confusing. Half tick does not make any sense from the scheduler view point (or am I missing something?), so why return half a tick to the user.
Quoted 6 lines
It must be inherited from old code dated back to Cray UNICOS. I do not know if bad thing can happen if both utime and stime are less than 1 usec... I guess not. But i agree that half a tick does not make sense. To play safe, we can change it to 1 usec if both utime and stime are sub microsecond. What do you think?
Hi Balbir,
Quoted 8 lines
I figured this out. The tsk->stime (and utime as well) are charged by 1 tick (or cputime) from the timer interrupt handler through update_process_times->account_{user,system}_time.
The clock resolution is a tick. Any short process less than 1 tick will the counter being 0. It can be from 0 to 0.99999... tick. A half tick is the average value.
But the scheduling happens in the granularity of a tick, so the minimum each task gets is a tick.

Quoted 7 lines

Quoted 4 lines
I think it makes more sense to assign a half tick than assign 1 usec to the stime. What do you think? Certainly the code need better explanation.
Can't we leave these values as zero in case both stime and utime are zero.

Yes, i will leave them as zero in this case.

Regards,
- jay

Quoted 9 lines

Quoted 7 lines
Regards,
- jay
[snip]