Date: Mon, 27 Oct 2014 11:27:45 -0400 From: John Baldwin <jhb@freebsd.org> To: freebsd-arch@freebsd.org Cc: John-Mark Gurney <jmg@funkthat.com>, Mateusz Guzik <mjguzik@gmail.com> Subject: Re: refcount_release_take_##lock Message-ID: <2629048.tOq3sNXcCP@ralph.baldwin.cx> In-Reply-To: <20141025190407.GU82214@funkthat.com> References: <20141025184448.GA19066@dft-labs.eu> <20141025190407.GU82214@funkthat.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Saturday, October 25, 2014 12:04:07 PM 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...
No, this has a race as others have noted. Please go fix the IPsec code. :)
> My code gets rid of a branch, and is better in that it uses refcount
> API properly, instead of using atomic_cmpset_int...
He is extending the refcount() API (which uses atomic_* internally).
The API implementation _should_ use atomic_* directly.
Mateusz,
Please keep the refcount_*() prefix so it matches the rest of the API. I
would just declare the functions directly in refcount.h rather than requiring
a macro to be invoked in each C file. We can also just implement the needed
lock types for now instead of all of them.
You could maybe replace 'take' with 'lock', but either name is fine.
--
John Baldwin
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?2629048.tOq3sNXcCP>