Date: Wed, 22 Aug 2007 08:35:52 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: John Baldwin <jhb@freebsd.org> Cc: Alfred Perlstein <alfred@freebsd.org>, freebsd-arch@freebsd.org Subject: Re: Lockless uidinfo. Message-ID: <20070822063552.GC4187@garage.freebsd.pl> In-Reply-To: <200708211753.34697.jhb@freebsd.org> References: <20070818120056.GA6498@garage.freebsd.pl> <200708211403.29293.jhb@freebsd.org> <20070821191902.GA4187@garage.freebsd.pl> <200708211753.34697.jhb@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20070822063552.GC4187>