Date: Tue, 8 Oct 2019 19:43:27 +0300 From: Konstantin Belousov <kostikbel@gmail.com> To: Andriy Gapon <avg@FreeBSD.org> Cc: freebsd-arch@FreeBSD.ORG Subject: Re: illumos membar_producer emulation (zfs, dtrace) Message-ID: <20191008164327.GH44691@kib.kiev.ua> In-Reply-To: <28c5d9c1-740f-7832-873f-3cf03c6d94a4@FreeBSD.org> References: <28c5d9c1-740f-7832-873f-3cf03c6d94a4@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Oct 08, 2019 at 06:22:07PM +0300, Andriy Gapon wrote: > > I have got a passing interest in cleaning up of the atomic and related illumos > compatibility (and contrib) code. > > At the moment I am looking at membar_producer. > >From Oracle Solaris documentation it follows that membar_producer is a store to > store barrier. > https://docs.oracle.com/cd/E88353_01/html/E37855/membar-producer-9f.html > > I do not think that we have an MI abstraction for that, so it seems that > atomic_thread_fence_rel could be the closest thing that we have. It also > affects loads which is redundant but not a big deal (IMO). > > As of now, we have membar_producer implemented in assembly for several platforms > (aarch64, amd64, i386, powerpc64, sparc64) and it's a no-op for the rest. > I think that the latter is a problem as it can affect correctness of the code. > > Also, the assembly implementations seem to be closer to wmb() although not > always the same. E.g., on amd64 both are sfence, while on sparc64 > membar_producer is "membar #StoreStore" while wmb is "membar #MemIssue". > I think that those implementations, maybe except for sparc64, are a bit of an > overkill as the code that uses membar_producer only deals with "normal" memory. > Maybe illumos uses membar_producer for access ordering of both normal memory and > memory (or instructions) with special caching properties. > > The relevant code is in: > - sys/cddl/compat/opensolaris/sys/atomic.h > - sys/cddl/compat/opensolaris/kern/opensolaris_atomic.c > - sys/cddl/contrib/opensolaris/common/atomic/*/opensolaris_atomic.S > > So, what do you think, would it be safe to emulate membar_producer with > atomic_thread_fence_rel? > Or is it better to leave the current code (copied from illumos for amd64, i386, > sparc64 and home-grown for aarch64, powerpc64) alone? > In that case, we still need to add something for the rest of the platforms > (arm-s, mips-es, riscv). > > P.S. > A bit offtopic. > > I noticed a couple of uses of wmb() with stores to regular memory. > I think that they are either not needed or better be "release barriers". > > vm_set_rendezvous_func() in sys/amd64/vmm/vmm.c > rendezvous_func is checked and modified under rendezvous_mtx anyway. > > cpu_reset() in sys/x86/x86/cpu_machdep.c > I do not see a need for a store-store barrier here. > > pmc_initialize() in sys/dev/hwpmc/hwpmc_mod.c > I think that a store_rel or fence_rel would be more appropriate. > > Note sure about all uses of wmb in xen code, seems like at least some of them > should be release operations or fences as well. SFENCE on x86 WB memory is NOP for all practical reasons. The only use of SFENCE known to me is to brace instructions like CLFLUSH(OPT) or CLZERO which list SFENCE as the requirements. So use of atomic_thread_fence_rel() might be more optimal if the semantics fit.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20191008164327.GH44691>