refcount_release_take_##lock

[John-Mark Gurney]

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."