From owner-freebsd-arch@FreeBSD.ORG Tue Aug 21 20:22:46 2007 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id B64DD16A418; Tue, 21 Aug 2007 20:22:46 +0000 (UTC) (envelope-from pjd@garage.freebsd.pl) Received: from mail.garage.freebsd.pl (arm132.internetdsl.tpnet.pl [83.17.198.132]) by mx1.freebsd.org (Postfix) with ESMTP id D1E3613C4B6; Tue, 21 Aug 2007 20:22:45 +0000 (UTC) (envelope-from pjd@garage.freebsd.pl) Received: by mail.garage.freebsd.pl (Postfix, from userid 65534) id 90AFB48804; Tue, 21 Aug 2007 22:22:44 +0200 (CEST) Received: from localhost (154.81.datacomsa.pl [195.34.81.154]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.garage.freebsd.pl (Postfix) with ESMTP id 1CF9645685; Tue, 21 Aug 2007 22:22:36 +0200 (CEST) Date: Tue, 21 Aug 2007 22:21:36 +0200 From: Pawel Jakub Dawidek To: John Baldwin Message-ID: <20070821202136.GB4187@garage.freebsd.pl> References: <20070818120056.GA6498@garage.freebsd.pl> <20070818155041.GY90381@elvis.mu.org> <20070818161449.GE6498@garage.freebsd.pl> <200708211403.29293.jhb@freebsd.org> <20070821191902.GA4187@garage.freebsd.pl> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="3uo+9/B/ebqu+fSQ" Content-Disposition: inline In-Reply-To: <20070821191902.GA4187@garage.freebsd.pl> User-Agent: Mutt/1.4.2.3i X-PGP-Key-URL: http://people.freebsd.org/~pjd/pjd.asc X-OS: FreeBSD 7.0-CURRENT i386 X-Spam-Checker-Version: SpamAssassin 3.0.4 (2005-06-05) on mail.garage.freebsd.pl X-Spam-Level: X-Spam-Status: No, score=-2.6 required=3.0 tests=BAYES_00 autolearn=ham version=3.0.4 Cc: Alfred Perlstein , freebsd-arch@freebsd.org Subject: Re: Lockless uidinfo. X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 21 Aug 2007 20:22:46 -0000 --3uo+9/B/ebqu+fSQ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 21, 2007 at 09:19:02PM +0200, Pawel Jakub Dawidek wrote: > > Memory barriers on another CPU don't mean anything about the CPU thread= 2 is=20 > > on. Memory barriers do not flush caches on other CPUs, etc. Normally = when=20 > > objects are refcounted in a table, the table holds a reference on the o= bject,=20 > > but that doesn't seem to be the case here. [...] >=20 > But the memory barrier from 'mtx_lock(&uihashtbl_mtx)' above > 'if (uip->ui_ref > 0)' would do the trick and I can safely avoid using > atomic read in this if statement, right? >=20 > > [...] Have you tried doing something=20 > > very simple in uifree(): > >=20 > > { > > mtx_lock(&uihashtbl_mtx); > > if (refcount_release(...)) { > > LIST_REMOVE(); > > mtx_unlock(&uihashtbl_mtx); > > ... > > free(); > > } else > > mtx_unlock(&uihashtbl_mtx); > > } > >=20 > > I wouldn't use a more complex algo in uifree() unless the simple one is= shown=20 > > to perform badly. Needless complexity is a hindrance to future mainten= ance. >=20 > Of coure we could do that, but I was trying really hard to remove > contention in the common case. Before we used UIDINFO_LOCK() in the > common case, now you suggesting using global lock here, and I'd really, > really prefer using one atomic only. >=20 > > Also, even if you do go with the more complex route, I'd rather you red= uce=20 > > diffs with the current code by keeping the test as 'uip->ui_ref =3D=3D = 0' and=20 > > keeping the removal code in the if-block. >=20 > Will do. >=20 > > In chgproccnt() you should use atomic_fetchadd_long() to avoid a race w= hen=20 > > reading ui_proccnt. > >=20 > >=20 > > old =3D atomic_fetchadd_long(&uip->ui_proccnt, diff); > > if (old + diff < 0) > > printf("...."); >=20 > I'm aware of this race, but I don't find closing it that much important. > We won't generate false positive here. My vote is to leave it as it is, > because atomic_fetchadd_long() is slower on some archs than > atomic_add_long(), ie. it is implemented using atomic_cmpset_long() > loop, and as I checked by running 8 processes on 8way machine with > older code that used atomic_cmpset_long() loop in 'diff > 0' case, > there is almost one extra loop on every call, which makes it about 6% > slower. >=20 > > OTOH, atomic_fetchadd_long() doesn't yet exist, so you will need to fix= that,=20 > > or just always use an atomic_cmpset() loop. >=20 > I already implemented those. New patch is here: http://people.freebsd.org/~pjd/patches/uidinfo_waitfree.patch --=20 Pawel Jakub Dawidek http://www.wheel.pl pjd@FreeBSD.org http://www.FreeBSD.org FreeBSD committer Am I Evil? Yes, I Am! --3uo+9/B/ebqu+fSQ Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4 (FreeBSD) iD8DBQFGy0lQForvXbEpPzQRAgIlAKDIP5P5TWOBUAHsgLX7lIe4tYsgxwCgvmpD d/K97EVO24dghgaNS0woxDA= =rVvA -----END PGP SIGNATURE----- --3uo+9/B/ebqu+fSQ--