Date: Sat, 25 Oct 2014 12:53:34 -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: <20141025195334.GW82214@funkthat.com> In-Reply-To: <20141025192632.GB19066@dft-labs.eu> References: <20141025184448.GA19066@dft-labs.eu> <20141025190407.GU82214@funkthat.com> <20141025192632.GB19066@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 21:26 +0200:
> 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.
Per the email I wrote to Ian, this "assumption" needs to be well
documented that though the "list" has a reference, and that this
reference is not accounted for in the ref count...
And I personally think that it's a bug for the list to not hold it's
own reference... Yes, then you need to compare for when the ref count
hits one, and do the lock/dec/free/unlock, but that keeps the refcount
sane...
> 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 :)
That's too heavy weight...
> > > 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.
Yeh, the only advantage is that it only appears once per file used,
so it's not THAT long...
> > > Comments?
>
> >
> > Will you update the refcount(9) man page w/ documentation before
> > committing?
>
> Sure.
Thanks.
--
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?20141025195334.GW82214>