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.
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?"
[0] https://postgr.es/m/14947.1475690465%40sss.pgh.pa.us
--
Nathan Bossart
Amazon Web Services: https://aws.amazon.com