Open riastradh opened 1 year ago
@riastradh: Thanks. However, I think __HAVE_ATOMIC_AS_MEMBAR
is really ugly, error-prone and should better be removed.
Can we just use C11 here? I would even use a plain atomic_fetch_add() / atomic_fetch_sub() as performance is really not a concern in this code path (i.e. just keep it straightorward to read!). If needed, we can define NetBSD-specific wrappers in the npf_os.c (under the #ifdef __NetBSD__
fragment).
@riastradh: Thanks. However, I think
__HAVE_ATOMIC_AS_MEMBAR
is really ugly, error-prone and should better be removed.Can we just use C11 here? I would even use a plain atomic_fetch_add() / atomic_fetch_sub() as performance is really not a concern in this code path (i.e. just keep it straightorward to read!). If needed, we can define NetBSD-specific wrappers in the npf_os.c (under the
#ifdef __NetBSD__
fragment).
No real disagreement here. This was part of a much larger change I made in NetBSD to audit reference counts so I could fix bugs promptly -- without taking extra effort to tidy everything up or rework the abstractions, and without incurring a performance penalty on machines that don't need it. I'm just posting all of my commits to npf in NetBSD for posterity so they don't languish as local changes.
@riastradh: I would encourage you to amend atomic_{inc,dec}_uint{_nv,}
in NetBSD to include the memory barriers. Otherwise, such semantics makes code more error-prone and more verbose. It is not worth, especially when acquire/release are free on x86/amd64.
In order to ensure that that all users of an object have finished all access to it before the last one actually frees it, we need the decrement to have release/acquire semantics to establish a happens-before relation.
To guarantee that A in one thread happens-before B in another thread, we need the reference count decrement (and branch) to have release/acquire semantics, so put membar_release before and membar_acquire after. (We could use memory_order_acq_rel with atomic_fetch_add_explicit in C11.)
Note: membar_producer and membar_consumer are not enough, because they only order stores on one side and loads on the other, whereas it is necessary to order loads and stores on both sides.