Re: [patch 1/3] add basic accounting fields to taskstats

Shailabh Nagar wrote:

Jay Lan wrote:

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
===================================================================
--- linux.orig/include/linux/taskstats.h	2006-07-31 11:38:54.132326042 -0700
+++ linux/include/linux/taskstats.h	2006-07-31 11:42:10.634609610 -0700
@@ -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;



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



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.

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



Combine the padding's ?

				 * 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] */



s/sinec/since


Good eyes! :)

+	__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
===================================================================
--- linux.orig/kernel/taskstats.c	2006-07-31 11:38:54.160326367 -0700
+++ linux/kernel/taskstats.c	2006-07-31 11:44:54.952523699 -0700
@@ -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);

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


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

stats->ac_etime = timespec_to_ns(&ts);


No, ac_etime is in microseconds, not nanoseconds.

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


This is correct.


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.

+	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)
{
@@ -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.


Will do. Thanks!


Regards,
 - jay

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


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

References:
- [patch 1/3] add basic accounting fields to taskstats
  - From: Jay Lan <[email protected]>
- Re: [patch 1/3] add basic accounting fields to taskstats
  - From: Shailabh Nagar <[email protected]>

Prev by Date: Re: let md auto-detect 128+ raid members, fix potential race condition
Next by Date: Re: Solaris ZFS on Linux [Was: Re: the " 'official' point of view" expressed by kernelnewbies.org regarding reiser4 inclusion]
Previous by thread: Re: [patch 1/3] add basic accounting fields to taskstats
Next by thread: Re: [patch 1/3] add basic accounting fields to taskstats
Index(es):
- Date
- Thread

[Index of Archives] [Kernel Newbies] [Netfilter] [Bugtraq] [Photo] [Stuff] [Gimp] [Yosemite News] [MIPS Linux] [ARM Linux] [Linux Security] [Linux RAID] [Video 4 Linux] [Linux for the blind] [Linux Resources]