On Wed, May 05, 2010 at 12:48:53AM +0000, Alexander Krizhanovsky wrote:
> Konstantin,
>
> Concerning i/o counters we collect them in rucollect() in for loop and
> update in various places, for example in vfs_bio.c. Rusage of an exiting
> threads is handled in exit1() -> ruadd().
>
> Your version of the patch certainly is more robust, however I'm still
> concerning about following things, which could be done better:
>
> 1) Zeroing of thread runtime statistic looks fine if we use it only for
> whole process statistic calculating, but now (when it's also used as is
> for the thread statistic) it should be handled independently, i.e.
> RUSAGE_{SELF,CHILDREN} calls should not affect the thread structures
> somehow. So I suppose we need to introduce some new counters to proc
> structure and counters update code (it was stopped me to go on this way).
What do you mean by zeroing of thread runtime statistic ? Can you, please,
point me to exact location in code ? I did not found such code when
I did initial review of your patch.I did testing by repeatedly calling getrusage with alternated RUSAGE_SELF/RUSAGE_THREAD commands, and got sane increasing snapshots of statistic. > > 2) With first in mind, getruasge(RUSAGE_THREAD) is called in current > thread and doesn't affect or use information from other threads, so it > definitely should use less number of locks, may be with atomic > operations for read-update-write operations. In fact the same getting > per-thread statistic in Linux is done _without_ locks at all (however > Linux uses different process/thread model). thread_lock() is spinlock, and it disables preemption. calcru1() is very sensible to have ticks counters to be in consistent state. You can look at kern_time.c implementation of CLOCK_THREAD_CPUTIME_ID, where indeed only preepmtion is disabled by use of critical section. On the other hand, td_rux is accessed by other threads, and caclru1() updates should be properly syncronized. Since thread_lock would be needed for this, and it would give slightly more consistent results for the copy of td_ru, I used it. I do not think that thread_lock for running thread is a bottleneck, and getrusage definitely should be not a contention point for properly written application. > > If we're in time and it really looks like a problem (is getrusage() ever > a hotspot?) I can try to prepare the patch with less locking on this > weekend. > > Also I still don't understand the sanity check in calccru1() for > negativeness of tu ( (int64_t)tu < 0 ) - is it reachable? It seems that > it is possible only after about 300 thousand years of uptime... Could > you please explain this? I never saw this message, may be change it to assertion, as proposed in phk comment, is reasonable. I do not have an opinion. > > Should I write further about the patch to svn-src-all@ (sorry, but I'm > new in FreeBSD mailing) ? I dropped svn-src@, lets discuss it there.
pgpRoEZeiOx7K.pgp
Description: PGP signature
