Date: Sat, 25 Oct 2014 21:26:32 +0200 From: Mateusz Guzik <mjguzik@gmail.com> To: freebsd-arch@freebsd.org Subject: Re: refcount_release_take_##lock Message-ID: <20141025192632.GB19066@dft-labs.eu> 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 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 <mjguzik gmail.com>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20141025192632.GB19066>