From owner-freebsd-arch@FreeBSD.ORG Sat Oct 25 19:47:00 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 8FABB659; Sat, 25 Oct 2014 19:47:00 +0000 (UTC) Received: from h2.funkthat.com (gate2.funkthat.com [208.87.223.18]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client CN "funkthat.com", Issuer "funkthat.com" (not verified)) by mx1.freebsd.org (Postfix) with ESMTPS id 654DDFEA; Sat, 25 Oct 2014 19:47:00 +0000 (UTC) Received: from h2.funkthat.com (localhost [127.0.0.1]) by h2.funkthat.com (8.14.3/8.14.3) with ESMTP id s9PJkw8Q031397 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sat, 25 Oct 2014 12:46:59 -0700 (PDT) (envelope-from jmg@h2.funkthat.com) Received: (from jmg@localhost) by h2.funkthat.com (8.14.3/8.14.3/Submit) id s9PJkwJD031396; Sat, 25 Oct 2014 12:46:58 -0700 (PDT) (envelope-from jmg) Date: Sat, 25 Oct 2014 12:46:58 -0700 From: John-Mark Gurney To: Ian Lepore Subject: Re: refcount_release_take_##lock Message-ID: <20141025194658.GV82214@funkthat.com> Mail-Followup-To: Ian Lepore , Mateusz Guzik , freebsd-arch@FreeBSD.org References: <20141025184448.GA19066@dft-labs.eu> <20141025190407.GU82214@funkthat.com> <1414265035.12052.646.camel@revolution.hippie.lan> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1414265035.12052.646.camel@revolution.hippie.lan> User-Agent: Mutt/1.4.2.3i X-Operating-System: FreeBSD 7.2-RELEASE i386 X-PGP-Fingerprint: 54BA 873B 6515 3F10 9E88 9322 9CB1 8F74 6D3F A396 X-Files: The truth is out there X-URL: http://resnet.uoregon.edu/~gurney_j/ X-Resume: http://resnet.uoregon.edu/~gurney_j/resume.html X-TipJar: bitcoin:13Qmb6AeTgQecazTWph4XasEsP7nGRbAPE X-to-the-FBI-CIA-and-NSA: HI! HOW YA DOIN? can i haz chizburger? X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.2.2 (h2.funkthat.com [127.0.0.1]); Sat, 25 Oct 2014 12:46:59 -0700 (PDT) Cc: Mateusz Guzik , freebsd-arch@FreeBSD.org 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:47:00 -0000 Ian Lepore wrote this message on Sat, Oct 25, 2014 at 13:23 -0600: > On Sat, 2014-10-25 at 12:04 -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)) { > > Could you not get preempted at this point, whereupon another thread > acquires then releases obj, deletes it because it keeps running through > this point, then eventually your original thread wakes up, gets the > lock, and dereferences the now-defunct obj pointer? Depends upon how you handle reference counts... If you allow someone to create a reference when they don't have one (by definition since the object has 0 references), then yes, that would be a problem... But by definition, if the current thread transitions an object from 1 to 0 count, you are the only one w/ a reference, and are safe from another thread getting a reference and doing what you said... Now if you're talking about a data structure that keeps a reference so that others can create references to the object, then shouldn't there be one more reference for the data structure? And that case is different, and you are correct, if the above code is used, a race will be introduced... > (Also, I think that should be != 0, above?) Yes, or just drop the comparision... I didn't read the refcount_release man page to check for return value... > > 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... > > > > > 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_ > > > > > Comments? > > > > Will you update the refcount(9) man page w/ documentation before > > committing? -- John-Mark Gurney Voice: +1 415 225 5579 "All that I will do, has been done, All that I have, has not."