Hi Alexandre,
> #define SLOCK_DEFINE_SCALAR_ACCESSORS(typename) \
> static inline typename \
> slock_read_barrier_##typename(volatile slock_t *lock, volatile typename *p) \
> { \
> typename val; \
> \
> (void) lock; \
> AssertPointerAlignment(p, alignof(typename)); \
> val = *p; \
> pg_read_barrier(); \
> return val; \
> } \
> static inline void \
> slock_write_barrier_##typename(volatile slock_t *lock, volatile typename *p, typename v) \
> { \
> (void) lock; \
> AssertPointerAlignment(p, alignof(typename)); \
> *p = v; \
> pg_write_barrier(); \
>
> }
On x86 pg_read_barrier() and pg_write_barrier() are only compiler
barriers, so v1 ends up as a plain load and a plain store there. That
hides problems. ARM, RISC-V and POWER are weakly ordered, and on those
a write barrier placed after the store does not order it against the
stores before it. The spinlock version does not have that problem.
pg_write_barrier(), pg_read_barrier(), are compiler barriers, and they
are the same as pg_compiler_barrier()?
Those barriers have no cost at run time right? So it wouldn't hurt
having two.
I think the safer route is what recent commits like df3978c2340 did,
convert the field to pg_atomic and use pg_atomic_read_membarrier_u32()
and pg_atomic_write_membarrier_u32().
I see that pg_atomic_write_membarrier_u32 uses an exchange,
In the patch above I used __atomic_store (that AFAIK will not be reordered)
and might block other processes trying to write on the same
cache line.
Trying to make it portable, to handle at least bool, uint32, uint64, Pointer
in a regular pattern.
Full barrier
pg_memory_barrier();
*out = *in; /* where only one side is shared */
pg_memory_barrier();
Release barrier
pg_compiler_barrier();
*p_shared = v_local;
pg_memory_barrier();
Acquire barrier
pg_memory_barrier();
v_local = *p_shared;
pg_compiler_barrier();
Does it make sense?
Can release/acquire semantics be used or we MUST have a full barrier?
Regards,
Alexandre
