re: kern/45539: add support for getrusage(2) memory size statistics

[matthew green <mrg%eterna.com.au@localhost>]

The following reply was made to PR kern/45539; it has been noted by GNATS.

From: matthew green <mrg%eterna.com.au@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: kern-bug-people%netbsd.org@localhost, gnats-admin%netbsd.org@localhost,
    netbsd-bugs%netbsd.org@localhost
Subject: re: kern/45539: add support for getrusage(2) memory size statistics
Date: Sun, 30 Oct 2011 13:11:55 +1100

 > >Synopsis:       add support for getrusage(2) memory size statistics
 
 thanks for looking at this.  i've wished it worked for ages.
 
 this is from statclock():
 
 > +    /*
 > +     * If the CPU is currently scheduled to a non-idle process, then charge
 > +     * that process with the appropriate VM resource utilization for a tick.
 > +     *
 > +     * Assume that the current process has been running the entire last
 > +     * tick, and account for VM use regardless of whether in user mode or
 > +     * system mode (XXX or interrupt mode?).
 > +     *
 > +     * rusage VM stats are expressed in kilobytes * ticks-of-execution.
 > +     */
 > +    /* based on code from 4.3BSD kern_clock.c and from FreeBSD */
 > +
 > +# define pg2kb(n)   (((n) * PAGE_SIZE) / 1024)
 > +
 > +    if (p != NULL) {
 > +            struct vmspace *vm = p->p_vmspace;
 > +            long rss;
 > +
 > +            /*
 > +             * XXX is p_stmutex currently enough to protect updates to
 > +             * p_stats too?
 > +             */
 > +            p->p_stats->p_ru.ru_idrss += pg2kb(vm->vm_dsize); /* unshared 
 > data */
 > +            p->p_stats->p_ru.ru_isrss += pg2kb(vm->vm_ssize); /* unshared 
 > stack */
 > +            p->p_stats->p_ru.ru_ixrss += pg2kb(vm->vm_tsize); /* "shared" 
 > text? */
 
 this is the wrong way to get these values.  the += will
 accumulate over time, 'hz' times a second, depend on the target.
 this is probably why you have wrong values.
 
 i don't think statclock() is the right place to do it.  i would
 simply grab them when filling in the rusage as normal.  (but this
 makes me question what these values actually mean.  the member
 names are "ru_i?rss".  is the "rss" supposed to imply "resident
 set size" here, to?  if so, i don't know that we have any way of
 getting that info.)
 
 > +            rss = pg2kb(vm_resident_count(vm));
 > +            if (rss > p->p_stats->p_ru.ru_maxrss)
 > +                    p->p_stats->p_ru.ru_maxrss = rss;
 > +    }
 
 initially, i didn't like the look of this one either.
 vm_resident_count() can be expensive (but i don't think it is
 on any current netbsd ports) but i'm not sure that pushing the
 cost of it anywhere else would help.  eg, pushing it into lwp
 switch would end up being more expensive to maintain on a system
 with very active but switching lwp's.
 
 
 .mrg.