Re: locked reads for atomics

Hi,

On 2023-11-10 20:38:13 -0600, Nathan Bossart wrote:
> On Fri, Nov 10, 2023 at 03:11:50PM -0800, Andres Freund wrote:
> > On 2023-11-10 14:51:28 -0600, Nathan Bossart wrote:
> >> + * This read is guaranteed to read the current value,
> > 
> > It doesn't guarantee that *at all*. What it guarantees is solely that the
> > current CPU won't be doing something that could lead to reading an outdated
> > value. To actually ensure the value is up2date, the modifying side also needs
> > to have used a form of barrier (in the form of fetch_add, compare_exchange,
> > etc or an explicit barrier).
> 
> Okay, I think I was missing that this doesn't work along with
> pg_atomic_write_u32() because that doesn't have any barrier semantics
> (probably because the spinlock version does).  IIUC you'd want to use
> pg_atomic_exchange_u32() to write the value instead, which seems to really
> just be another compare/exchange under the hood.

Yes. We should optimize pg_atomic_exchange_u32() one of these days - it can be
done *far* faster than a cmpxchg. When I was adding the atomic abstraction
there was concern with utilizing too many different atomic instructions. I
didn't really agree back then, but these days I really don't see a reason to
not use a few more intrinsics.


> Speaking of the spinlock implementation of pg_atomic_write_u32(), I've been
> staring at this comment for a while and can't make sense of it:
> 
>     void
>     pg_atomic_write_u32_impl(volatile pg_atomic_uint32 *ptr, uint32 val)
>     {
>         /*
>          * One might think that an unlocked write doesn't need to acquire the
>          * spinlock, but one would be wrong. Even an unlocked write has to cause a
>          * concurrent pg_atomic_compare_exchange_u32() (et al) to fail.
>          */
>         SpinLockAcquire((slock_t *) &ptr->sema);
>         ptr->value = val;
>         SpinLockRelease((slock_t *) &ptr->sema);
>     }
> 
> It refers to "unlocked writes," but this isn't
> pg_atomic_unlocked_write_u32_impl().  The original thread for this comment
> [0] doesn't offer any hints, either.  Does "unlocked" mean something
> different here, such as "write without any barrier semantics?"

It's just about not using the spinlock. If we were to *not* use a spinlock
here, we'd break pg_atomic_compare_exchange_u32(), because the
spinlock-implementation of pg_atomic_compare_exchange_u32() needs to actually
be able to rely on no concurrent changes to the value to happen.

Greetings,

Andres Freund