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

[Jay Lan]

Andrew Morton wrote:
> Jay Lan <jlan@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.sourceforge.net/lists/listinfo/lse-tech

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