Correct fences for sys/refcount.h

[kib]

Summary

The acq barrier in refcount_acquire() has no use, constructor must ensure that the changes are visible before publication by other means.

Last release must sync/with the constructor and all updaters.

This is quite subtle and based on the refcount/shared_ptr analysis I heard at the Hans Boehm/ Herb Sutter talks about C++ atomics.

[jhb]

I agree with this.

[alc]

sys/sys/refcount.h

69 ↗

(On Diff #29809)

"destructing code" -> "destructor"

72 ↗

(On Diff #29809)

It's unclear what "there" refers to.

[kib]

sys/sys/refcount.h

72 ↗

(On Diff #29809)

The claim is that the code for refcount_release() should look like this, using C11 operations:

int
refcount_release(volatile atomic_uint *count)
{
  int res;

   res = atomic_fetch_sub_explicit(count, 1, memory_order_acq_rel);
   return (res == 1);
}

The release operation in non-final refcount_release() calls which return false, sync/w the acquire operation on the final refcount_release() returning true. In other words, 'there' means the atomic_fetchadd_int op.

[kib]

In fact, we can add atomic_fence_rel() before the atomic_fetchadd_int() line for the same effect.

[kib]

Provide the release fence in refcount_release(), instead of relying on user handling it correctly.

[markj]

Are the talk(s) you referenced the ones from cppcon?

edit: looks like this is covered in the last ~15 minutes of https://www.youtube.com/watch?v=M15UKpNlpeM

[kib]

In D11270#233386, @markj wrote:

Are the talk(s) you referenced the ones from cppcon?

edit: looks like this is covered in the last ~15 minutes of https://www.youtube.com/watch?v=M15UKpNlpeM

Yes, and Herb's talk from 2015.

[kib]

Alan, are you fine with the latest variant of the patch ?

[alc]

In D11270#235791, @kib wrote:

Alan, are you fine with the latest variant of the patch ?

I haven't had a chance to think about it in any depth. Give me until Friday.

[alc]

sys/sys/refcount.h

68 ↗

(On Diff #29817)

"destructor" is misspelled.