Message-ID: <44CE7B0E.30805@sgi.com>
Date:	Mon, 31 Jul 2006 14:50:06 -0700
From:	Jay Lan <jlan@....com>
To:	Shailabh Nagar <nagar@...son.ibm.com>
Cc:	Andrew Morton <akpm@...l.org>, lkml <linux-kernel@...r.kernel.org>,
	Balbir Singh <balbir@...ibm.com>, Jes Sorensen <jes@....com>,
	Chris Sturtivant <csturtiv@....com>, Tony Ernst <tee@....com>
Subject: 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 <jlan@....com>
>>
>>
>>------------------------------------------------------------------------
>>
>>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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists