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

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

    >
    > 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@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    [W:0.024 / U:30.160 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site