On Mon, Feb 10, 2025 at 10:35:58PM +0100, Mark Kettenis wrote:
> > 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@

Just to be clear for SMR this is not strictly needed.
The membar_producer() is there to ensure that the object is written and
stable before setting its SMR pointer.

In the SMR_PTR_GET() case there is no real need for the consumer membar
since the data we care about must be accessed through the returned
pointer. So the CPU can not reorder loads in a way that makes this membar
necessary.  If you have code that requires this membar then you are not
doing clean SMR and with that you have to run your own membars.
 
> > 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)
> > 
> > 
> 

-- 
:wq Claudio