From owner-freebsd-hackers@FreeBSD.ORG Tue May 4 21:25:03 2010 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 732FD106564A; Tue, 4 May 2010 21:25:03 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from mail.zoral.com.ua (mx0.zoral.com.ua [91.193.166.200]) by mx1.freebsd.org (Postfix) with ESMTP id 0C0588FC08; Tue, 4 May 2010 21:25:01 +0000 (UTC) Received: from deviant.kiev.zoral.com.ua (root@deviant.kiev.zoral.com.ua [10.1.1.148]) by mail.zoral.com.ua (8.14.2/8.14.2) with ESMTP id o44LOmgY063660 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Wed, 5 May 2010 00:24:48 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: from deviant.kiev.zoral.com.ua (kostik@localhost [127.0.0.1]) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4) with ESMTP id o44LOm1b051577; Wed, 5 May 2010 00:24:48 +0300 (EEST) (envelope-from kostikbel@gmail.com) Received: (from kostik@localhost) by deviant.kiev.zoral.com.ua (8.14.4/8.14.4/Submit) id o44LOmgJ051576; Wed, 5 May 2010 00:24:48 +0300 (EEST) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: deviant.kiev.zoral.com.ua: kostik set sender to kostikbel@gmail.com using -f Date: Wed, 5 May 2010 00:24:48 +0300 From: Kostik Belousov To: Alexander Krizhanovsky Message-ID: <20100504212448.GS23646@deviant.kiev.zoral.com.ua> 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> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="p2u4WfPhYOuYlOsk" Content-Disposition: inline In-Reply-To: <4BE0C075.2040106@natsys-lab.com> User-Agent: Mutt/1.4.2.3i X-Virus-Scanned: clamav-milter 0.95.2 at skuns.kiev.zoral.com.ua X-Virus-Status: Clean X-Spam-Status: No, score=-2.4 required=5.0 tests=ALL_TRUSTED,AWL,BAYES_50, DNS_FROM_OPENWHOIS autolearn=no version=3.2.5 X-Spam-Checker-Version: SpamAssassin 3.2.5 (2008-06-10) on skuns.kiev.zoral.com.ua Cc: freebsd-hackers@freebsd.org, ur4ina@gmail.com, bde@freebsd.org Subject: Re: [PATCH] RUSAGE_THREAD X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 04 May 2010 21:25:03 -0000 --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--