From owner-freebsd-arch@FreeBSD.ORG Wed Aug 22 06:37:00 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 6FF7116A41A; Wed, 22 Aug 2007 06:37:00 +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 8D42813C45D; Wed, 22 Aug 2007 06:36:59 +0000 (UTC) (envelope-from pjd@garage.freebsd.pl) Received: by mail.garage.freebsd.pl (Postfix, from userid 65534) id 0878B48800; Wed, 22 Aug 2007 08:36:58 +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 6D5CB45683; Wed, 22 Aug 2007 08:36:53 +0200 (CEST) Date: Wed, 22 Aug 2007 08:35:52 +0200 From: Pawel Jakub Dawidek To: John Baldwin Message-ID: <20070822063552.GC4187@garage.freebsd.pl> References: <20070818120056.GA6498@garage.freebsd.pl> <200708211403.29293.jhb@freebsd.org> <20070821191902.GA4187@garage.freebsd.pl> <200708211753.34697.jhb@freebsd.org> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="PuGuTyElPB9bOcsM" Content-Disposition: inline In-Reply-To: <200708211753.34697.jhb@freebsd.org> 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: Wed, 22 Aug 2007 06:37:00 -0000 --PuGuTyElPB9bOcsM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 21, 2007 at 05:53:34PM -0400, John Baldwin wrote: > On Tuesday 21 August 2007 03:19:02 pm Pawel Jakub Dawidek wrote: > > > Memory barriers on another CPU don't mean anything about the CPU thre= ad 2=20 > is=20 > > > on. Memory barriers do not flush caches on other CPUs, etc. Normall= y=20 > when=20 > > > objects are refcounted in a table, the table holds a reference on the= =20 > object,=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 > Yes, though that may be rather non-obvious to other people, or to yoursel= f 6=20 > months down the road. You should probably document it. Agreed, I added a comment that should explain it. > > > I wouldn't use a more complex algo in uifree() unless the simple one = is=20 > shown=20 > > > to perform badly. Needless complexity is a hindrance to future=20 > maintenance. > >=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 > *sigh* You ignored my last sentence. You need to think about other peop= le=20 > who come and read this later or yourself 12 months from now when you've p= aged=20 > out all the uidinfo stuff from your head. >=20 > Hmm, what happens if you get this: >=20 > thread A > -------- >=20 > refcount_release() - drops to 0 >=20 > preempted >=20 > thread B > -------- > uihold() - refcount goes to 1 > ... > uifree() - refcount goes to 0 > removes object from table and frees it >=20 > resumes >=20 > mtx_lock(&uihashtbl); >=20 > sees refcount of 0 so tries to destroy object again >=20 > *BOOM* (corruption, panic, etc.) Grr, good catch. > This is the race that the current code handles by taking a reference > on the uid while it drops the uidinfo lock to acquire the table lock. >=20 > Probably you need to not drop the last reference unless you hold the=20 > uihashtbl_mtx, which means not using refcount_release() and manually use = an=20 > atomic_cmpset_int() if the refcount is > 1 to drop a ref to optimize the= =20 > common case: >=20 > old =3D uip->ui_refs; > if (old > 1) { > if (atomic_cmpset_int(&uip->ui_refs, old, old - 1)) > return; > } >=20 > mtx_lock(&uihashtbl_mtx); > if (refcount_release(&uip->ui_refs)) { > /* remove from table and free */ > } > mtx_unlock(&uihashtbl_mtx); How about we relookup it after acquiring the uihashtbl_mtx lock? Something like this (comments removed): uid_t uid; uid =3D uip->ui_uid; if (!refcount_release(&uip->ui_ref)) return; mtx_lock(&uihashtbl_mtx); uip =3D uilookup(uid); if (uip !=3D NULL && uip->ui_ref =3D=3D 0) { LIST_REMOVE(uip, ui_hash); mtx_unlock(&uihashtbl_mtx); FREE(uip, M_UIDINFO); return; } mtx_unlock(&uihashtbl_mtx); I updated the patch at: 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! --PuGuTyElPB9bOcsM Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4 (FreeBSD) iD8DBQFGy9lIForvXbEpPzQRAkyOAJoCwVBu1Fnr1tcYreempKrSrzJg+ACg2Vw9 T95TsOerhH6jnArM9tD3pno= =kw9m -----END PGP SIGNATURE----- --PuGuTyElPB9bOcsM--