Date: Sat, 25 Oct 2014 15:47:02 -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: <20141025224702.GY82214@funkthat.com> In-Reply-To: <20141025201240.GC19066@dft-labs.eu> References: <20141025184448.GA19066@dft-labs.eu> <20141025190407.GU82214@funkthat.com> <20141025192632.GB19066@dft-labs.eu> <20141025195334.GW82214@funkthat.com> <20141025201240.GC19066@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 22:12 +0200:
> On Sat, Oct 25, 2014 at 12:53:34PM -0700, John-Mark Gurney wrote:
> > 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...
>
> Well, this is for stuff which cleans up after itself.
I hope that everything cleans up after itself.. :) otherwise we'd
have memory leaks everywhere...
> Example usage is with per-uid stats for resource limits. These
> automatically free themselves with the last cred with given uid.
This example doesn't give me enough information to decide what you
mean.. Is there a hash table that we look up these cred structures?
or are they referenced from an already existing reference?
> This has its own problems (like constant creation and destruction of
> stuff for the same cred), but seems ok enough for some cases.
>
> Otherwise we would have to actively free these structs somehow.
I'm still not sure how your example addresses this..
I believe you wrote the data structure case, but it wasn't clear that
is what you were doing, and as I said, I think it's a bug to have an
implicit ref in such data structures w/o properly documenting them...
Part of the reason why we need documentation to make sure people don't
make mistakes like these...
> > > 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?20141025224702.GY82214>