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
--p2u4WfPhYOuYlOsk
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
On Wed, May 05, 2010 at 12:48:53AM +0000, Alexander Krizhanovsky wrote:
> Konstantin,
>=20
> Concerning i/o counters we collect them in rucollect() in for loop and=20
> update in various places, for example in vfs_bio.c. Rusage of an exiting=
=20
> threads is handled in exit1() -> ruadd().
>=20
> Your version of the patch certainly is more robust, however I'm still=20
> concerning about following things, which could be done better:
>=20
> 1) Zeroing of thread runtime statistic looks fine if we use it only for=
=20
> whole process statistic calculating, but now (when it's also used as is=
=20
> for the thread statistic) it should be handled independently, i.e.=20
> RUSAGE_{SELF,CHILDREN} calls should not affect the thread structures=20
> somehow. So I suppose we need to introduce some new counters to proc=20
> 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.
>=20
> 2) With first in mind, getruasge(RUSAGE_THREAD) is called in current=20
> thread and doesn't affect or use information from other threads, so it=20
> definitely should use less number of locks, may be with atomic=20
> operations for read-update-write operations. In fact the same getting=20
> per-thread statistic in Linux is done _without_ locks at all (however=20
> 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.
>=20
> If we're in time and it really looks like a problem (is getrusage() ever=
=20
> a hotspot?) I can try to prepare the patch with less locking on this=20
> weekend.
>=20
> Also I still don't understand the sanity check in calccru1() for=20
> negativeness of tu ( (int64_t)tu < 0 ) - is it reachable? It seems that=
=20
> it is possible only after about 300 thousand years of uptime... Could=20
> 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.
>=20
> Should I write further about the patch to svn-src-all@ (sorry, but I'm=20
> new in FreeBSD mailing) ?
I dropped svn-src@, lets discuss it there.
--p2u4WfPhYOuYlOsk
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.10 (FreeBSD)
iEYEARECAAYFAkvgkJ8ACgkQC3+MBN1Mb4jvxACeJc0aX1u7F5sVKJztUDLJN0oR
T5AAoM+ltDqR0I032lNdrTNY91rcrOPz
=YJdg
-----END PGP SIGNATURE-----
--p2u4WfPhYOuYlOsk--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20100504212448.GS23646>