refcount_release_take_##lock

[John Baldwin]

On Saturday, October 25, 2014 12:04:07 PM 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...

No, this has a race as others have noted.  Please go fix the IPsec code. :)
 
> My code gets rid of a branch, and is better in that it uses refcount
> API properly, instead of using atomic_cmpset_int...

He is extending the refcount() API (which uses atomic_* internally).
The API implementation _should_ use atomic_* directly.

Mateusz,

Please keep the refcount_*() prefix so it matches the rest of the API.  I 
would just declare the functions directly in refcount.h rather than requiring 
a macro to be invoked in each C file.  We can also just implement the needed 
lock types for now instead of all of them.

You could maybe replace 'take' with 'lock', but either name is fine.

-- 
John Baldwin