From: Alexander Bluhm <bluhm@openbsd.org>
Subject: smr memory barrier
To: tech@openbsd.org
Cc: Visa Hankala <visa@openbsd.org>
Date: Mon, 10 Feb 2025 21:57:29 +0100

Hi,

Usually membar producer and consumer should be symmetric.

If the CPU that runs SMR_PTR_SET_LOCKED() wants to make sure that
other CPUs see the changes of the object before they see the new
pointer, it needs a membar_producer().

But then the other CPUs must call membar_consumer() to read the
data, that had been modified before the new pointer was published.

If this is the intended semantics of SMR, we must add a membar_consumer()
to SMR_PTR_GET().  Otherwise, if the macros don't care and expect
the caller to insert barriers, the membar_producer() should be
removed from SMR_PTR_SET_LOCKED().  I think it is less error prone
if the SMR_PTR_GET() and SMR_PTR_SET_LOCKED() handle both.

ok?

bluhm

Index: sys/smr.h
===================================================================
RCS file: /data/mirror/openbsd/cvs/src/sys/sys/smr.h,v
diff -u -p -U7 -r1.9 smr.h
--- sys/smr.h	25 Jul 2022 08:06:44 -0000	1.9
+++ sys/smr.h	10 Feb 2025 20:47:30 -0000
@@ -65,15 +65,19 @@ smr_init(struct smr_entry *smr)
 #else
 #define SMR_ASSERT_CRITICAL()		do {} while (0)
 #define SMR_ASSERT_NONCRITICAL()	do {} while (0)
 #endif
 
 #endif /* _KERNEL */
 
-#define SMR_PTR_GET(pptr)		READ_ONCE(*pptr)
+#define SMR_PTR_GET(pptr) ({						\
+	typeof(*pptr) __ptr = *(volatile typeof(*pptr) *)(pptr);	\
+	membar_consumer();						\
+	__ptr;								\
+})
 
 #define SMR_PTR_GET_LOCKED(pptr)	(*(pptr))
 
 #define SMR_PTR_SET_LOCKED(pptr, val) do {				\
 	membar_producer();						\
 	WRITE_ONCE(*pptr, val);						\
 } while (0)