From owner-freebsd-arch@FreeBSD.ORG Sat Oct 25 19:26:37 2014 Return-Path: Delivered-To: freebsd-arch@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id C52B6EC3 for ; Sat, 25 Oct 2014 19:26:37 +0000 (UTC) Received: from mail-wg0-x22b.google.com (mail-wg0-x22b.google.com [IPv6:2a00:1450:400c:c00::22b]) (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 5DFE6E2A for ; Sat, 25 Oct 2014 19:26:37 +0000 (UTC) Received: by mail-wg0-f43.google.com with SMTP id n12so3072886wgh.26 for ; Sat, 25 Oct 2014 12:26:35 -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=RdolTdLegQqgq2jDw7DW0z/AQpTGlRin2yNLiXwg4lU=; b=UM05/IO/AeRROvlNuaqGxoKUwTC4BucOvRymOBiVMyIh7fybREcPu2fEvNyhx9aMeE spS5m3flVC6HNEbzssESSJbU7AN6xdZYxK0TNII4rPmPwDVG31cSDo0eYn8y4uXRPpe4 WXiWeVqm89jHq0ETWYCrkV/BADEoa5ivdpJmEZ7tLfmjD+FcWQ2hHYwF+tY1phPAeGw9 sk98RZ3cZHcOPu7oyiP3DULzu+6cgkUcWoqeLKYv6gFRlCREjEk3pMGnFoGfFGAHbEU1 7Vfjnpe0c9uVPpBTu879qXpCUjvurFXK9b1GzNtbGWniVwLdlY4bv+o5ucKNWNoTHUUj IkUA== X-Received: by 10.180.211.70 with SMTP id na6mr11619901wic.3.1414265195651; Sat, 25 Oct 2014 12:26:35 -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 bl9sm5951081wib.24.2014.10.25.12.26.34 for (version=TLSv1.2 cipher=RC4-SHA bits=128/128); Sat, 25 Oct 2014 12:26:34 -0700 (PDT) Date: Sat, 25 Oct 2014 21:26:32 +0200 From: Mateusz Guzik To: freebsd-arch@freebsd.org Subject: Re: refcount_release_take_##lock Message-ID: <20141025192632.GB19066@dft-labs.eu> References: <20141025184448.GA19066@dft-labs.eu> <20141025190407.GU82214@funkthat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20141025190407.GU82214@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 19:26:38 -0000 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. 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 :) > > 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. > > Comments? > > Will you update the refcount(9) man page w/ documentation before > committing? > Sure. -- Mateusz Guzik