From owner-freebsd-arch@FreeBSD.ORG Sat Oct 25 19:42:20 2014
Return-Path: <owner-freebsd-arch@FreeBSD.ORG>
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 1E2564CB
for <freebsd-arch@freebsd.org>; Sat, 25 Oct 2014 19:42:20 +0000 (UTC)
Received: from mho-02-ewr.mailhop.org (mho-02-ewr.mailhop.org [204.13.248.72])
(using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits))
(Client did not present a certificate)
by mx1.freebsd.org (Postfix) with ESMTPS id D4291FBC
for <freebsd-arch@freebsd.org>; Sat, 25 Oct 2014 19:42:19 +0000 (UTC)
Received: from [73.34.117.227] (helo=ilsoft.org)
by mho-02-ewr.mailhop.org with esmtpsa (TLSv1:AES256-SHA:256)
(Exim 4.72) (envelope-from <ian@FreeBSD.org>)
id 1Xi6wP-000JMj-1Z; Sat, 25 Oct 2014 19:23:57 +0000
Received: from [172.22.42.240] (revolution.hippie.lan [172.22.42.240])
by ilsoft.org (8.14.9/8.14.9) with ESMTP id s9PJNtVS072720;
Sat, 25 Oct 2014 13:23:55 -0600 (MDT) (envelope-from ian@FreeBSD.org)
X-Mail-Handler: Dyn Standard SMTP by Dyn
X-Originating-IP: 73.34.117.227
X-Report-Abuse-To: abuse@dyndns.com (see
http://www.dyndns.com/services/sendlabs/outbound_abuse.html for abuse
reporting information)
X-MHO-User: U2FsdGVkX19cxQyYTC9Uz84e/ruQTnuK
X-Authentication-Warning: paranoia.hippie.lan: Host revolution.hippie.lan
[172.22.42.240] claimed to be [172.22.42.240]
Subject: Re: refcount_release_take_##lock
From: Ian Lepore <ian@FreeBSD.org>
To: John-Mark Gurney <jmg@funkthat.com>
In-Reply-To: <20141025190407.GU82214@funkthat.com>
References: <20141025184448.GA19066@dft-labs.eu>
<20141025190407.GU82214@funkthat.com>
Content-Type: text/plain; charset="us-ascii"
Date: Sat, 25 Oct 2014 13:23:55 -0600
Message-ID: <1414265035.12052.646.camel@revolution.hippie.lan>
Mime-Version: 1.0
X-Mailer: Evolution 2.32.1 FreeBSD GNOME Team Port
Content-Transfer-Encoding: 7bit
Cc: Mateusz Guzik <mjguzik@gmail.com>, 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 <freebsd-arch.freebsd.org>
List-Unsubscribe: <http://lists.freebsd.org/mailman/options/freebsd-arch>,
<mailto:freebsd-arch-request@freebsd.org?subject=unsubscribe>
List-Archive: <http://lists.freebsd.org/pipermail/freebsd-arch/>
List-Post: <mailto:freebsd-arch@freebsd.org>
List-Help: <mailto:freebsd-arch-request@freebsd.org?subject=help>
List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/freebsd-arch>,
<mailto:freebsd-arch-request@freebsd.org?subject=subscribe>
X-List-Received-Date: Sat, 25 Oct 2014 19:42:20 -0000
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?
(Also, I think that should be != 0, above?)
-- Ian
> 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?
>