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

[Jay Lan]

Andrew Morton wrote:
> Please don't send multiple patches under the same Subject:.  It confuses me
> and breaks my patch processing tools (I strip out the "1/2" numbering
> because it becomes irrelevant).
>
> Please choose a meaningful and distinct title for each patch.  See
> http://www.zip.com.au/~akpm/linux/patches/stuff/tpp.txt

Will do better next time :)

> Jay Lan <jlan@xxxxxxxxxxxx> wrote:
>
> > 2/2: acct_mm
> >
> > Enhanced MM accounting data collection.
>
>
> > Index: linux/include/linux/sched.h
> > ===================================================================
> > --- linux.orig/include/linux/sched.h	2004-10-01 17:16:35.105905373 -0700
> > +++ linux/include/linux/sched.h	2004-10-14 12:15:33.450280955 -0700
> > @@ -249,6 +249,8 @@
> > 	struct kioctx		*ioctx_list;
> >
> > 	struct kioctx		default_kioctx;
> > +
> > +	unsigned long hiwater_rss, hiwater_vm;
> > };
>
>
> 	unsigned long hiwater_rss;	/* comment goes here */
> 	unsigned long hiwater_vm;	/* and here */

Will do.

> > extern int mmlist_nr;
> > @@ -593,6 +595,10 @@
> >
> > /* i/o counters(bytes read/written, #syscalls */
> > 	unsigned long rchar, wchar, syscr, syscw;
> > +#if defined(CONFIG_BSD_PROCESS_ACCT)
> > +	u64 acct_rss_mem1, acct_vm_mem1;
> > +	clock_t acct_stimexpd;
> > +#endif
>
>
> Please place the above three fields on separate lines and document them.
>
> It's not clear to me what, semantically, these fields represent.  That's
> something which is appropriate for the supporting changelog entry.

Certainly!

> > +/* Update highwater values */
> > +static inline void update_mem_hiwater(void)
> > +{
> > +	if (current->mm) {
> > +		if (current->mm->hiwater_rss < current->mm->rss) {
> > +			current->mm->hiwater_rss = current->mm->rss;
> > +		}
> > +		if (current->mm->hiwater_vm < current->mm->total_vm) {
> > +			current->mm->hiwater_vm = current->mm->total_vm;
> > +		}
> > +	}
> > +}
>
>
> If this has more than one callsite then it it too big to inline.
>
> If it has a single callsite then it's OK to inline it, but it can and
> should be moved into the .c file.
>
>
> > +
> > +static inline void acct_update_integrals(void)
> > +{
> > +	long delta;
> > +
> > +	if (current->mm) {
> > +		delta = current->stime - current->acct_stimexpd;
> > +		current->acct_stimexpd = current->stime;
> > +		current->acct_rss_mem1 += delta * current->mm->rss;
> > +		current->acct_vm_mem1 += delta * current->mm->total_vm;
> > +	}
> > +}
>
>
> Consider caching `current' in a local variable - sometimes gcc likes to
> reevaluate it each time and it takes 14 bytes of code per pop.
>
> This function is too big to inline.

If not inline, where these functions do you suggest to place? How
about kernel/acct.c?

> > +static inline void acct_clear_integrals(struct task_struct *tsk)
> > +{
> > +	if (tsk) {
> > +		tsk->acct_stimexpd = 0;
> > +		tsk->acct_rss_mem1 = 0;
> > +		tsk->acct_vm_mem1 = 0;
> > +	}
> > +}
>
>
> Do any of the callers pass in a null `tsk'?

No. Just to be safe, i guess :(

***

The revised version of acct_io and acct_mm should be posted soon.

Thank you very much for reviewing and comments.

- 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/