Re: [Lse-tech] [PATCH 2.6.9 1/2] enhanced accounting data collection

[Andrew M. <ak...@os...>]

Jay Lan <jl...@en...> 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 ;)