membar_enter is currently documented to be a store-before-load/store barrier: https://man.netbsd.org/NetBSD-9.2/membar_ops.3
membar_enter() Any store preceding membar_enter() will happen before all memory operations following it. Prompted by the use of membar_enter in sys_select.c that appeared to be intended as a load-before-load/store barrier, a.k.a. acquire barrier (like atomic_load_acquire -- often much cheaper than, and more useful than, a store-before-load/store barrier), I started reviewing the various definitions and uses of membar_enter. It looks like: - Every definition of membar_enter implies load-before-load/store semantics -- and one of the definitions, powerpc's, _does not_ imply store-before-load/store semantics. Exception: riscv membar_enter is fence w,rw ...but I'm not sure the code we have in riscv matters at the moment -- as far as I know a working userland hasn't gone out in any release yet. - Every use of membar_enter appears to assume load-before-load/store semantics, or occurs immediately after an atomic r/m/w operation, where the difference between the two barrier types is immaterial because it's an atomic load-and-store. Exception: There's _one_ use of membar_enter as store-before- load/store...except it's not necessary and can be safely deleted anyway. The names membar_enter/exit came from Solaris. Solaris's man page (https://docs.oracle.com/cd/E86824_01/html/E54779/membar-enter-9f.html) says: The membar_enter() function is a generic memory barrier used during lock entry. It is placed after the memory operation that acquires the lock to guarantee that the lock protects its data. No stores from after the memory barrier will reach visibility and no loads from after the barrier will be resolved before the lock acquisition reaches global visibility. I can't find anything about store-before-load/store in this wording -- I suspect our man page's wording (which was `Any store preceding membar_enter() will reach global visibility before all loads and stores following it' before I changed it to use `happens before' in netbsd-9, <https://man.netbsd.org/NetBSD-8.2/membar_ops.3>) arose from a misunderstanding in parroting Solaris. So I propose to change the membar_enter documentation to match the definitions and usage (and change the riscv definition), making it instead: membar_enter() Any load preceding membar_enter() will happen before all memory operations following it. This will also let us delete the obnoxious text I added to atomic_loadstore(9) warning about membar_enter semantics. Thoughts? * Definitions The following definitions provide load-before-load/store: aarch64: alias for membar_sync alpha: alias for membar_sync arm: alias for membar_sync hppa: same instructions as membar_sync i386: lfence or LOCK ADDL -- load-before-load/store or full barrier ia64: same instructions as membar_sync mips: alias for membar_sync or1k: alias for membar_sync powerpc: isync -- load-before-load/store, BUT NOT store-before-load/store (!) sparc: alias for membar_sync sparc64: alias for membar_sync (not sure membar #LoadLoad is right, though!) x86_64: lfence or LOCK ADDQ -- load-before-load/store or full barrier The following definitions provide store-before-load/store but not load-before-load/store: riscv: fence w,rw But I'm not sure our riscv code works at all, and it has certainly never gone out in a release, so I'm not sure this one matters. * Uses These uses appear to be clearly intended as load-before-load/store: https://nxr.netbsd.org/xref/src/sys/kern/sys_select.c?r=1.57#656 https://nxr.netbsd.org/xref/src/lib/libpthread/pthread_rwlock.c?r=1.42#232 [1] https://nxr.netbsd.org/xref/src/lib/libpthread/pthread_rwlock.c?r=1.42#356 [1] https://nxr.netbsd.org/xref/src/sys/external/bsd/ena-com/ena_com.c?r=1.1#508 [2] https://nxr.netbsd.org/xref/src/sys/kern/sys_select.c?r=1.57#656 https://nxr.netbsd.org/xref/src/sys/kern/vfs_vnode.c?r=1.128#274 These uses appear to be intended as store-before-load/store: https://nxr.netbsd.org/xref/src/sys/kern/subr_copy.c?r=1.14#447 => However, this is not necessary because ipi_trigger_broacdast already serves as a release operation. So we can just delete this membar_enter. These uses I just can't figure out and I suspect they're wrong (should probably transpose the order with the lwproc_curlwpop calls, at which point most likely load-before-load/store is appropriate -- if any barriers are needed at all here, which remains unclear): https://nxr.netbsd.org/xref/src/sys/rump/librump/rumpkern/lwproc.c?r=1.51#382 [1] These also looks like they're misusing PTHREAD__ATOMIC_IS_MEMBAR, because there's no atomic r/m/w between the the pthread__park and the conditional membar_enter. Side note: looks like pthread_rwlock_unlock needs a membar_exit or atomic_store_release for thread->pt_rwlocked. [2] In ena_com.c, rmb is defined as an alias for membar_enter at <https://nxr.netbsd.org/xref/src/sys/external/bsd/ena-com/ena_plat.h?r=1.7#381>.