lkml.org 
[lkml]   [2004]   [Nov]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [Lse-tech] [PATCH 2.6.9 2/2] enhanced accounting data collection

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@engr.sgi.com> 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@lists.sourceforge.net
> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-03-22 14:07    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site