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

[Jay Lan <jlan@engr.sgi.com>]

Andrew Morton wrote:
> 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.

Will be done that way next time.

> 
> 
>>+			current->syscr++;
> 
> 
> Should these metrics be per-thread or per-heavyweight process?

Our customers found it useful per process.

> 
> 
>>+	if (ret > 0) {
>>+		current->rchar += ret;
>>+	}
> 
> 
> It's conventional to omit the braces if there is only one statement in the
> block.

Fixed. Also a few other places in the same siutation.

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

Fixed. That was from pagg, but i should not assume that.

> 
> 
>> #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?

You are correct. Thanks!

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

Will be corrected.

Thanks,
  - jay

> 
> 
> 
> -------------------------------------------------------
> This SF.net email is sponsored by: IT Product Guide on ITManagersJournal
> Use IT products in your business? Tell us what you think of them. Give us
> Your Opinions, Get Free ThinkGeek Gift Certificates! Click to find out more
> http://productguide.itmanagersjournal.com/guidepromo.tmpl
> _______________________________________________
> Lse-tech mailing list
> Lse-tech@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/lse-tech