From owner-freebsd-arch@FreeBSD.ORG Tue Aug 21 21:53:45 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 97F2816A418; Tue, 21 Aug 2007 21:53:45 +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 3925813C480; Tue, 21 Aug 2007 21:53:45 +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 204537137-1834499 for multiple; Tue, 21 Aug 2007 17:53:44 -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 l7LLrexf001627; Tue, 21 Aug 2007 17:53:42 -0400 (EDT) (envelope-from jhb@freebsd.org) From: John Baldwin To: Pawel Jakub Dawidek Date: Tue, 21 Aug 2007 17:53:34 -0400 User-Agent: KMail/1.9.6 References: <20070818120056.GA6498@garage.freebsd.pl> <200708211403.29293.jhb@freebsd.org> <20070821191902.GA4187@garage.freebsd.pl> In-Reply-To: <20070821191902.GA4187@garage.freebsd.pl> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-15" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200708211753.34697.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]); Tue, 21 Aug 2007 17:53:42 -0400 (EDT) X-Virus-Scanned: ClamAV 0.88.3/4021/Tue Aug 21 11:42:43 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: Tue, 21 Aug 2007 21:53:45 -0000 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. > > [...] Have you tried doing something > > very simple in uifree(): > > > > { > > mtx_lock(&uihashtbl_mtx); > > if (refcount_release(...)) { > > LIST_REMOVE(); > > mtx_unlock(&uihashtbl_mtx); > > ... > > free(); > > } else > > mtx_unlock(&uihashtbl_mtx); > > } > > > > 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.) 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); -- John Baldwin