Date: Wed, 5 May 2010 00:24:48 +0300 From: Kostik Belousov <kostikbel@gmail.com> To: Alexander Krizhanovsky <ak@natsys-lab.com> Cc: freebsd-hackers@freebsd.org, ur4ina@gmail.com, bde@freebsd.org Subject: Re: [PATCH] RUSAGE_THREAD Message-ID: <20100504212448.GS23646@deviant.kiev.zoral.com.ua> In-Reply-To: <4BE0C075.2040106@natsys-lab.com> References: <4BCBA7F8.3060109@natsys-lab.com> <20100425145929.GJ2422@deviant.kiev.zoral.com.ua> <4BDF2E4C.4090009@natsys-lab.com> <20100503163524.GE23646@deviant.kiev.zoral.com.ua> <4BE0C075.2040106@natsys-lab.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
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.
[-- Attachment #2 --]
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (FreeBSD)
iEYEARECAAYFAkvgkJ8ACgkQC3+MBN1Mb4jvxACeJc0aX1u7F5sVKJztUDLJN0oR
T5AAoM+ltDqR0I032lNdrTNY91rcrOPz
=YJdg
-----END PGP SIGNATURE-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100504212448.GS23646>