Make memory barrier definitions consistent across kernels

We inherited membar_consumer() and membar_producer() from OpenSolaris, but we had replaced membar_consumer() with Linux's smp_rmb() in zfs_ioctl.c. The FreeBSD SPL consequently implemented a shim for the Linux-only smp_rmb(). Additionally, FreeBSD did a clever optimization by implementing membar_producer() as a compiler memory barrier on x86/amd64 to take advantage of the total store ordering (TSO) on those architectures, but did not implement that in its smp_rmb() shim. We reinstate membar_consumer() in platform independent code and adopt the FreeBSD practice of relying the TSO on amd64/x86 on Linux. Signed-off-by: Richard Yao <[email protected]>
openzfs · Sep 7, 2022 · b0f36cf · b0f36cf
1 parent 5976747
commit b0f36cf
Show file tree

Hide file tree

Showing 4 changed files with 22 additions and 3 deletions.
diff --git a/include/os/freebsd/linux/compiler.h b/include/os/freebsd/linux/compiler.h
@@ -83,7 +83,6 @@
 #define	__printf(a, b)			__printflike(a, b)
 
 #define	barrier()			__asm__ __volatile__("": : :"memory")
-#define	smp_rmb()		rmb()
 #define	___PASTE(a, b) a##b
 #define	__PASTE(a, b) ___PASTE(a, b)
 

diff --git a/include/os/freebsd/spl/sys/atomic.h b/include/os/freebsd/spl/sys/atomic.h
@@ -57,7 +57,8 @@ extern uint64_t atomic_cas_64(volatile uint64_t *target, uint64_t cmp,
     uint64_t newval);
 #endif
 
-#define	membar_producer	atomic_thread_fence_rel
+#define	membar_consumer()		atomic_thread_fence_acq()
+#define	membar_producer()		atomic_thread_fence_rel()
 
 static __inline uint32_t
 atomic_add_32_nv(volatile uint32_t *target, int32_t delta)

diff --git a/include/os/linux/spl/sys/vmsystm.h b/include/os/linux/spl/sys/vmsystm.h
@@ -44,7 +44,26 @@
 #define	zfs_totalhigh_pages	totalhigh_pages
 #endif
 
+/* Compiler memory barrier on GCC and Clang */
+#define	barrier()			__asm__ __volatile__("": : :"memory")
+
+/*
+ * As per the discussion in openzfs/zfs#13843, x86/amd64's TSO will
+ * implicitly ensure ordering, so we do not need explicit fences.
+ */
+
+#if defined(__x86_64) || defined(__i386)
+
+#define	membar_consumer()		barrier()
+#define	membar_producer()		barrier()
+
+#else
+
+#define	membar_consumer()		smp_rmb()
 #define	membar_producer()		smp_wmb()
+
+#endif /* defined(__x86_64) || defined(__i386) */
+
 #define	physmem				zfs_totalram_pages
 
 #define	xcopyin(from, to, size)		copy_from_user(to, from, size)

diff --git a/module/zfs/zfs_ioctl.c b/module/zfs/zfs_ioctl.c
@@ -7482,7 +7482,7 @@ zfsdev_get_state(minor_t minor, enum zfsdev_state_type which)
 
 	for (zs = zfsdev_state_list; zs != NULL; zs = zs->zs_next) {
 		if (zs->zs_minor == minor) {
-			smp_rmb();
+			membar_consumer();
 			switch (which) {
 			case ZST_ONEXIT:
 				return (zs->zs_onexit);