Date: Thu, 21 Oct 2004 19:16:08 -0700
From: Andrew Morton <akpm@osdl.org>
To: Jay Lan <jlan@engr.sgi.com>
Cc: lse-tech@lists.sourceforge.net, linux-kernel@vger.kernel.org,
       guillaume.thouvenin@bull.net
Subject: Re: [Lse-tech] [PATCH 2.6.9 1/2] enhanced accounting data
 collection
Message-Id: <20041021191608.06b74417.akpm@osdl.org>
In-Reply-To: <41786344.9070504@engr.sgi.com>
References: <41785FE3.806@engr.sgi.com>
	<41786344.9070504@engr.sgi.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1603
Lines: 58

Jay Lan <jlan@engr.sgi.com> wrote:
>
> 1/2: acct_io
> 
> Enahanced I/O accounting data collection.
> 

It's nice to use `diff -p' so people can see what functions you're hitting.

> +			current->syscr++;

Should these metrics be per-thread or per-heavyweight process?

> +	if (ret > 0) {
> +		current->rchar += ret;
> +	}

It's conventional to omit the braces if there is only one statement in the
block.

> ===================================================================
> --- linux.orig/include/linux/sched.h	2004-10-01 17:01:21.412848229 -0700
> +++ linux/include/linux/sched.h	2004-10-01 17:09:42.723482260 -0700
> @@ -591,6 +591,9 @@
>  	struct rw_semaphore pagg_sem;

There is no `pagg_sem' in the kernel, so this will spit a reject.

>  #endif
>  
> +/* i/o counters(bytes read/written, #syscalls */
> +	unsigned long rchar, wchar, syscr, syscw;
> +

These will overflow super-quick.  Shouldn't they be 64-bit?

> --- linux.orig/kernel/fork.c	2004-10-01 17:01:21.432379595 -0700
> +++ linux/kernel/fork.c	2004-10-01 17:09:42.732271376 -0700
> @@ -995,6 +995,7 @@
>  	p->real_timer.data = (unsigned long) p;
>  
>  	p->utime = p->stime = 0;
> +	p->rchar = p->wchar = p->syscr = p->syscw = 0;

We generally prefer

	p->rchar = 0;
	p->wchar = 0;
	etc.

yes, the code which is there has already sinned - feel free to clean it up
while you're there ;)

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