Date: Sat, 25 Oct 2014 12:04:07 -0700 From: John-Mark Gurney <jmg@funkthat.com> To: Mateusz Guzik <mjguzik@gmail.com> Cc: freebsd-arch@freebsd.org Subject: Re: refcount_release_take_##lock Message-ID: <20141025190407.GU82214@funkthat.com> In-Reply-To: <20141025184448.GA19066@dft-labs.eu> References: <20141025184448.GA19066@dft-labs.eu>
next in thread | previous in thread | raw e-mail | index | archive | help
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...
> 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."
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141025190407.GU82214>