From owner-freebsd-arch@FreeBSD.ORG Wed Aug 22 15:30:27 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 C524716A420; Wed, 22 Aug 2007 15:30:27 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from speedfactory.net (mail6.speedfactory.net [66.23.216.219]) by mx1.freebsd.org (Postfix) with ESMTP id 5AA5313C4CE; Wed, 22 Aug 2007 15:30:26 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from server.baldwin.cx (unverified [66.23.211.162]) by speedfactory.net (SurgeMail 3.8k) with ESMTP id 204651572-1834499 for multiple; Wed, 22 Aug 2007 11:30:18 -0400 Received: from localhost.corp.yahoo.com (john@localhost [127.0.0.1]) (authenticated bits=0) by server.baldwin.cx (8.13.8/8.13.8) with ESMTP id l7MFUCFU009663; Wed, 22 Aug 2007 11:30:13 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: Pawel Jakub Dawidek Date: Wed, 22 Aug 2007 10:16:33 -0400 User-Agent: KMail/1.9.6 References: <20070818120056.GA6498@garage.freebsd.pl> <200708211753.34697.jhb@freebsd.org> <20070822063552.GC4187@garage.freebsd.pl> In-Reply-To: <20070822063552.GC4187@garage.freebsd.pl> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200708221016.34107.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH authentication, not delayed by milter-greylist-2.0.2 (server.baldwin.cx [127.0.0.1]); Wed, 22 Aug 2007 11:30:13 -0400 (EDT) X-Virus-Scanned: ClamAV 0.88.3/4029/Wed Aug 22 09:37:05 2007 on server.baldwin.cx X-Virus-Status: Clean X-Spam-Status: No, score=-4.4 required=4.2 tests=ALL_TRUSTED,AWL,BAYES_00 autolearn=ham version=3.1.3 X-Spam-Checker-Version: SpamAssassin 3.1.3 (2006-06-01) on server.baldwin.cx 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 15:30:28 -0000 On Wednesday 22 August 2007 02:35:52 am Pawel Jakub Dawidek wrote: > 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 thread 2 > > is > > > > on. Memory barriers do not flush caches on other CPUs, etc. Normally > > when > > > > objects are refcounted in a table, the table holds a reference on the > > object, > > > > but that doesn't seem to be the case here. [...] > > > > > > 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? > > > > Yes, though that may be rather non-obvious to other people, or to yourself 6 > > 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 > > shown > > > > to perform badly. Needless complexity is a hindrance to future > > maintenance. > > > > > > 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. > > > > *sigh* You ignored my last sentence. You need to think about other people > > who come and read this later or yourself 12 months from now when you've paged > > out all the uidinfo stuff from your head. > > > > Hmm, what happens if you get this: > > > > thread A > > -------- > > > > refcount_release() - drops to 0 > > > > preempted > > > > thread B > > -------- > > uihold() - refcount goes to 1 > > ... > > uifree() - refcount goes to 0 > > removes object from table and frees it > > > > resumes > > > > mtx_lock(&uihashtbl); > > > > sees refcount of 0 so tries to destroy object again > > > > *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. > > > > Probably you need to not drop the last reference unless you hold the > > uihashtbl_mtx, which means not using refcount_release() and manually use an > > atomic_cmpset_int() if the refcount is > 1 to drop a ref to optimize the > > common case: > > > > old = uip->ui_refs; > > if (old > 1) { > > if (atomic_cmpset_int(&uip->ui_refs, old, old - 1)) > > return; > > } > > > > 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 = uip->ui_uid; > if (!refcount_release(&uip->ui_ref)) > return; > mtx_lock(&uihashtbl_mtx); > uip = uilookup(uid); > if (uip != NULL && uip->ui_ref == 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 That actually adds more overhead than what I suggested above to the case where you are going to free it. Also, I'm leery of having an object hang around with a zero ref count while it is in the table with the lock not held. -- John Baldwin