From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: smr memory barrier
To: Alexander Bluhm <bluhm@openbsd.org>
Cc: tech@openbsd.org, visa@openbsd.org
Date: Mon, 10 Feb 2025 22:35:58 +0100

> Date: Mon, 10 Feb 2025 21:57:29 +0100
> From: Alexander Bluhm <bluhm@openbsd.org>
> 
> 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?

Makes sense to me; ok kettenis@

> 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)
> 
>