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

[Andrew Morton]

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

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

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

> +/* 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.

> +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'?

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