From owner-freebsd-arch@FreeBSD.ORG Sat Oct 25 20:12:47 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 0F282BF4 for ; Sat, 25 Oct 2014 20:12:47 +0000 (UTC) Received: from mail-wi0-x232.google.com (mail-wi0-x232.google.com [IPv6:2a00:1450:400c:c05::232]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 90D82335 for ; Sat, 25 Oct 2014 20:12:46 +0000 (UTC) Received: by mail-wi0-f178.google.com with SMTP id q5so3608716wiv.17 for ; Sat, 25 Oct 2014 13:12:44 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:subject:message-id:references:mime-version :content-type:content-disposition:in-reply-to:user-agent; bh=f/j9gSaBbZGgnuOjuxdiHxX4aS7Sbr2qIWBFsJkI65w=; b=RstkupRo505BGqBzI6woiM+ZSGLmyHx0LQ2BCEXdlg4jcCqnSO/UtinM5i1eb4h5HG tkdxg2PZFgNmZl3FDdcSLOXLDf+3RyA2RMHnrTizGHV6SJvn9f2N2Undf5T0NROwXgTh wbGKFF5Q9NoaPmSuG14T9woAQqDam1AveTYyZ0ASqsB1jERURC8muDtyYjx8ywP0cTWD Ze9ZBbn5h/UefcEiHi9H2G+7X9kcmq1tfs6Bwkx2AbRXWnquYTgYe7R4pTgrI5QBGFY1 NXkyetSmTohppaRRimQSNizNf9NhOXv5oTyBrv66gsClDeoqYuLsP7o6sx64O7wqSPPl +qKQ== X-Received: by 10.194.58.8 with SMTP id m8mr13260118wjq.43.1414267964341; Sat, 25 Oct 2014 13:12:44 -0700 (PDT) Received: from dft-labs.eu (n1x0n-1-pt.tunnel.tserv5.lon1.ipv6.he.net. [2001:470:1f08:1f7::2]) by mx.google.com with ESMTPSA id j8sm6071270wib.10.2014.10.25.13.12.43 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 25 Oct 2014 13:12:43 -0700 (PDT) Date: Sat, 25 Oct 2014 22:12:40 +0200 From: Mateusz Guzik To: freebsd-arch@freebsd.org Subject: Re: refcount_release_take_##lock Message-ID: <20141025201240.GC19066@dft-labs.eu> References: <20141025184448.GA19066@dft-labs.eu> <20141025190407.GU82214@funkthat.com> <20141025192632.GB19066@dft-labs.eu> <20141025195334.GW82214@funkthat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20141025195334.GW82214@funkthat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: freebsd-arch@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: Discussion related to FreeBSD architecture List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 25 Oct 2014 20:12:47 -0000 On Sat, Oct 25, 2014 at 12:53:34PM -0700, John-Mark Gurney wrote: > Mateusz Guzik wrote this message on Sat, Oct 25, 2014 at 21:26 +0200: > > On Sat, Oct 25, 2014 at 12:04:07PM -0700, John-Mark Gurney wrote: > > > Mateusz Guzik wrote this message on Sat, Oct 25, 2014 at 20:44 +0200: > > > > The following idiom is used here and there: > > > > > > > > int old; > > > > old = obj->ref; > > > > if (old > 1 && atomic_cmpset_int(&obj->ref, old, old -1)) > > > > return; > > > > lock(&something); > > > > if (refcount_release(&obj->ref) == 0) { > > > > unlock(&something); > > > > return; > > > > } > > > > free up > > > > unlock(&something); > > > > > > > > ========== > > > > > > Couldn't this be better written as: > > > if (__predict_false(refcount_release(&obj->ref) == 0)) { > > > lock(&something); > > > if (__predict_true(!obj->ref)) { > > > free up > > > } > > > unlock(&something); > > > } > > > > > > The reason I'm asking is that I changed how IPsec SA ref counting was > > > handled, and used something similar... > > > > > > My code gets rid of a branch, and is better in that it uses refcount > > > API properly, instead of using atomic_cmpset_int... > > > > This is used when given obj is kept on a list and code which traverses > > it (locked) expects found objects to be valid to ref. > > > > If we were to reach count of 0 and then lock, it would be possible that > > other thread refed + unrefed the object and is now trying to lock as > > well. > > Per the email I wrote to Ian, this "assumption" needs to be well > documented that though the "list" has a reference, and that this > reference is not accounted for in the ref count... > > And I personally think that it's a bug for the list to not hold it's > own reference... Yes, then you need to compare for when the ref count > hits one, and do the lock/dec/free/unlock, but that keeps the refcount > sane... > Well, this is for stuff which cleans up after itself. Example usage is with per-uid stats for resource limits. These automatically free themselves with the last cred with given uid. This has its own problems (like constant creation and destruction of stuff for the same cred), but seems ok enough for some cases. Otherwise we would have to actively free these structs somehow. > > That could be remedied for type stable object by having a generation > > counter, but I doubt it's worth it. Not to mention objects we lock here > > are freeable :) > > That's too heavy weight... > > > > > I decided to implement it as a common function. > > > > > > > > We have only refcount.h and I didn't want to bloat all including code > > > > with additional definitions and as such I came up with a macro that has > > > > to be used in .c file and that will define appropriate inline func. > > > > > > > > I'm definitely looking for better names for REFCOUNT_RELEASE_TAKE_USE_ > > > > macro, assuming it has to stay. > > > > > > You could shorten it to REFCNT_REL_TAKE_ > > > > > > > All function use full 'refcount_release' and the like, so that would be > > inconsistent. > > > > Losing 'take' may be an option, I don't know. > > Yeh, the only advantage is that it only appears once per file used, > so it's not THAT long... > > > > > Comments? > > > > > > > > Will you update the refcount(9) man page w/ documentation before > > > committing? > > > > Sure. > > Thanks. > > -- > John-Mark Gurney Voice: +1 415 225 5579 > > "All that I will do, has been done, All that I have, has not." -- Mateusz Guzik