Date: Fri, 10 Nov 2023 20:38:13 -0600
From: Nathan Bossart <nathandbossart@gmail.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org
Subject: Re: locked reads for atomics
Message-ID: <20231111023813.GB1408159@nathanxps13>
References: <20231110205128.GB1315705@nathanxps13>
 <20231110231150.fjm77gup2i7xu6hc@alap3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231110231150.fjm77gup2i7xu6hc@alap3.anarazel.de>
Archived-At: <https://www.postgresql.org/message-id/20231111023813.GB1408159%40nathanxps13>
Precedence: bulk

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