Horton

Thread

Commits

  1. Convert archiver's force_dir_scan variable to an atomic variable.

  2. Introduce atomic read/write functions with full barrier semantics.

  1. locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-10T20:51:28Z

    Moving this to a new thread and adding it to the January commitfest.

On Thu, Nov 09, 2023 at 03:27:33PM -0600, Nathan Bossart wrote:
> On Tue, Nov 07, 2023 at 04:58:16PM -0800, Andres Freund wrote:
>> However, even if there's likely some other implied memory barrier that we
>> could piggyback on, the patch much simpler to understand if it doesn't change
>> coherency rules. There's no way the overhead could matter.
> 
> I wonder if it's worth providing a set of "locked read" functions.  Those
> could just do a compare/exchange with 0 in the generic implementation.  For
> patches like this one where the overhead really shouldn't matter, I'd
> encourage folks to use those to make it easy to reason about correctness.

Concretely, like this.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  2. Re: locked reads for atomics

    Andres Freund <andres@anarazel.de> — 2023-11-10T23:11:50Z

    Hi,

On 2023-11-10 14:51:28 -0600, Nathan Bossart wrote:
> Moving this to a new thread and adding it to the January commitfest.
> 
> On Thu, Nov 09, 2023 at 03:27:33PM -0600, Nathan Bossart wrote:
> > On Tue, Nov 07, 2023 at 04:58:16PM -0800, Andres Freund wrote:
> >> However, even if there's likely some other implied memory barrier that we
> >> could piggyback on, the patch much simpler to understand if it doesn't change
> >> coherency rules. There's no way the overhead could matter.
> > 
> > I wonder if it's worth providing a set of "locked read" functions.  Those
> > could just do a compare/exchange with 0 in the generic implementation.  For
> > patches like this one where the overhead really shouldn't matter, I'd
> > encourage folks to use those to make it easy to reason about correctness.
> 
> Concretely, like this.

I don't think "locked" is a good name - there's no locking. I think that'll
deter their use, because it will make it sound more expensive than they are.

pg_atomic_read_membarrier_u32()?



> @@ -228,7 +228,8 @@ pg_atomic_init_u32(volatile pg_atomic_uint32 *ptr, uint32 val)
>   * The read is guaranteed to return a value as it has been written by this or
>   * another process at some point in the past. There's however no cache
>   * coherency interaction guaranteeing the value hasn't since been written to
> - * again.
> + * again.  Consider using pg_atomic_locked_read_u32() unless you have a strong
> + * reason (e.g., performance) to use unlocked reads.

I think that's too strong an endorsement. Often there will be no difference in
difficulty analysing correctness, because the barrier pairing / the
interaction with the surrounding code needs to be analyzed just as much.


>   * No barrier semantics.
>   */
> @@ -239,6 +240,24 @@ pg_atomic_read_u32(volatile pg_atomic_uint32 *ptr)
>  	return pg_atomic_read_u32_impl(ptr);
>  }
>  
> +/*
> + * pg_atomic_read_u32 - locked read from atomic variable.

Un-updated name...


> + * 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).


> +#ifndef PG_HAVE_ATOMIC_LOCKED_READ_U32
> +#define PG_HAVE_ATOMIC_LOCKED_READ_U32
> +static inline uint32
> +pg_atomic_locked_read_u32_impl(volatile pg_atomic_uint32 *ptr)
> +{
> +	uint32 old = 0;
> +
> +	/*
> +	 * In the generic implementation, locked reads are implemented as a
> +	 * compare/exchange with 0.  That'll fail or succeed, but always return the
> +	 * most up-to-date value.  It might also store a 0, but only if the
> +	 * previous value was also a zero, i.e., harmless.
> +	 */
> +	pg_atomic_compare_exchange_u32_impl(ptr, &old, 0);
> +
> +	return old;
> +}
> +#endif

I suspect implementing it with an atomic fetch_add of 0 would be faster...

Greetings,

Andres Freund

  3. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-11T02:38:13Z

    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

  4. Re: locked reads for atomics

    Andres Freund <andres@anarazel.de> — 2023-11-11T02:48:39Z

    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

  5. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-11T02:55:29Z

    On Fri, Nov 10, 2023 at 06:48:39PM -0800, Andres Freund wrote:
> 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.

I might give this a try, if for no other reason than it'd force me to
improve my mental model of this stuff.  :)

>> 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.

Thanks for clarifying.  I thought it might've been hinting at something
beyond the compare/exchange implications.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  6. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2023-11-27T21:00:30Z

    Here's a v2 of the patch set in which I've attempted to address all
feedback.  I've also added a pg_write_membarrier_u* pair of functions that
provide an easy way to write to an atomic variable with full barrier
semantics.  In the generic implementation, these are just aliases for an
atomic exchange.

0002 demonstrates how these functions might be used to eliminate the
arch_lck spinlock, which is only ever used for one boolean variable.  My
hope is that the membarrier functions make eliminating spinlocks for
non-performance-sensitive code easy to reason about.

(We might be able to use a pg_atomic_flag instead for 0002, but that code
seems intended for a slightly different use-case and has more complicated
barrier semantics.)

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  7. Re: locked reads for atomics

    Li, Yong <yoli@ebay.com> — 2024-01-17T03:48:43Z

    
> On Nov 28, 2023, at 05:00, Nathan Bossart <nathandbossart@gmail.com> wrote:
> 
> External Email
> 
> Here's a v2 of the patch set in which I've attempted to address all
> feedback.  I've also added a pg_write_membarrier_u* pair of functions that
> provide an easy way to write to an atomic variable with full barrier
> semantics.  In the generic implementation, these are just aliases for an
> atomic exchange.
> 
> 0002 demonstrates how these functions might be used to eliminate the
> arch_lck spinlock, which is only ever used for one boolean variable.  My
> hope is that the membarrier functions make eliminating spinlocks for
> non-performance-sensitive code easy to reason about.
> 
> (We might be able to use a pg_atomic_flag instead for 0002, but that code
> seems intended for a slightly different use-case and has more complicated
> barrier semantics.)
> 
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com

Hi Nathan,

The patch looks good to me.

The patch adds two pairs of atomic functions that provide full-barrier semantics to atomic read/write operations. The patch also includes an example of how this new functions can be used to replace spin locks.

The patch applies cleanly to HEAD. “make check-world” also runs cleanly with no error.  I am moving it to Ready for Committers.

Regards,
Yong

  8. Re: locked reads for atomics

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2024-02-22T07:28:37Z

    On Tue, Nov 28, 2023 at 2:30 AM Nathan Bossart <nathandbossart@gmail.com> wrote:
>
> Here's a v2 of the patch set in which I've attempted to address all
> feedback.  I've also added a pg_write_membarrier_u* pair of functions that

There's some immediate use for reads/writes with barrier semantics -
https://www.postgresql.org/message-id/CALj2ACXrePj4E6ocKr-%2Bb%3DrjT-8yeMmHnEeWQP1bc-WXETfTVw%40mail.gmail.com.
Any plan for taking this forward?

-- 
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

  9. Re: locked reads for atomics

    Jeff Davis <pgsql@j-davis.com> — 2024-02-22T19:53:50Z

    On Thu, 2024-02-22 at 12:58 +0530, Bharath Rupireddy wrote:
> There's some immediate use for reads/writes with barrier semantics -

Is this mainly a convenience for safety/readability? Or is it faster in
some cases than doing an atomic access with separate memory barriers?

Regards,
	Jeff Davis

  10. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-23T16:17:58Z

    On Thu, Feb 22, 2024 at 11:53:50AM -0800, Jeff Davis wrote:
> On Thu, 2024-02-22 at 12:58 +0530, Bharath Rupireddy wrote:
>> There's some immediate use for reads/writes with barrier semantics -
> 
> Is this mainly a convenience for safety/readability? Or is it faster in
> some cases than doing an atomic access with separate memory barriers?

The former.  Besides the 0002 patch tracked here, there's at least one
other patch [0] that could probably use these new functions.  The idea is
to provide an easy way to remove spinlocks, etc. and use atomics for less
performance-sensitive stuff.  The implementations are intended to be
relatively inexpensive and might continue to improve in the future, but the
functions are primarily meant to help reason about correctness.

I don't mind prioritizing these patches, especially since there now seems
to be multiple patches waiting on it.  IIRC I was worried about not having
enough support for this change, but I might now have it.

[0] https://commitfest.postgresql.org/47/4330/

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  11. Re: locked reads for atomics

    Jeff Davis <pgsql@j-davis.com> — 2024-02-23T18:25:00Z

    On Fri, 2024-02-23 at 10:17 -0600, Nathan Bossart wrote:
> The idea is
> to provide an easy way to remove spinlocks, etc. and use atomics for
> less
> performance-sensitive stuff.  The implementations are intended to be
> relatively inexpensive and might continue to improve in the future,
> but the
> functions are primarily meant to help reason about correctness.

To be clear:

  x = pg_atomic_[read|write]_membarrier_u64(&v);

is semantically equivalent to:

  pg_memory_barrier();
  x = pg_atomic_[read|write]_u64(&v);
  pg_memory_barrier();

?

If so, that does seem more convenient.

Regards,
	Jeff Davis

  12. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-23T19:32:47Z

    On Fri, Feb 23, 2024 at 10:25:00AM -0800, Jeff Davis wrote:
> To be clear:
> 
>   x = pg_atomic_[read|write]_membarrier_u64(&v);
> 
> is semantically equivalent to:
> 
>   pg_memory_barrier();
>   x = pg_atomic_[read|write]_u64(&v);
>   pg_memory_barrier();
> 
> ?
> 
> If so, that does seem more convenient.

I think that's about right.  The upthread feedback from Andres [0] provides
some additional context.

[0] https://postgr.es/m/20231110231150.fjm77gup2i7xu6hc%40alap3.anarazel.de

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  13. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-23T20:58:12Z

    Here is a v3 of the patch set with the first draft of the commit messages.
There are no code differences between v2 and v3.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  14. Re: locked reads for atomics

    Andres Freund <andres@anarazel.de> — 2024-02-24T01:30:26Z

    Hi,

On 2024-02-23 10:25:00 -0800, Jeff Davis wrote:
> On Fri, 2024-02-23 at 10:17 -0600, Nathan Bossart wrote:
> > The idea is
> > to provide an easy way to remove spinlocks, etc. and use atomics for
> > less
> > performance-sensitive stuff.  The implementations are intended to be
> > relatively inexpensive and might continue to improve in the future,
> > but the
> > functions are primarily meant to help reason about correctness.
> 
> To be clear:
> 
>   x = pg_atomic_[read|write]_membarrier_u64(&v);
> 
> is semantically equivalent to:
> 
>   pg_memory_barrier();
>   x = pg_atomic_[read|write]_u64(&v);
>   pg_memory_barrier();
> ?
> 
> If so, that does seem more convenient.

Kinda. Semantically I think that's correct, however it doesn't commonly make
sense to have both those memory barriers, so you wouldn't really write code
like that and thus comparing on the basis of convenience doesn't quite seem
right.

Rather than convenience, I think performance and simplicity are better
arguments. If you're going to execute a read and then a memory barrier, it's
going to be faster to just do a single atomic operation. And it's a bit
simpler to analyze on which "side" of the read/write the barrier is needed.

Greetings,

Andres Freund

  15. Re: locked reads for atomics

    Andres Freund <andres@anarazel.de> — 2024-02-24T01:34:49Z

    Hi,

On 2024-02-23 14:58:12 -0600, Nathan Bossart wrote:
> +/*
> + * pg_atomic_write_membarrier_u32 - write with barrier semantics.
> + *
> + * The write is guaranteed to succeed as a whole, i.e., it's not possible to
> + * observe a partial write for any reader.  Note that this correctly interacts
> + * with both pg_atomic_compare_exchange_u32() and
> + * pg_atomic_read_membarrier_u32().  While this may be less performant than
> + * pg_atomic_write_u32() and pg_atomic_unlocked_write_u32(), it may be easier
> + * to reason about correctness with this function in less performance-sensitive
> + * code.
> + *
> + * Full barrier semantics.
> + */

The callout to pg_atomic_unlocked_write_u32() is wrong. The reason to use
pg_atomic_unlocked_write_u32() is for variables where we do not ever want to
fall back to spinlocks/semaphores, because the underlying variable isn't
actually shared. In those cases using the other variants is a bug. The only
use of pg_atomic_unlocked_write_u32() is temp-table buffers which share the
data structure with the shared buffers case.

Greetings,

Andres Freund

  16. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-24T15:27:34Z

    On Fri, Feb 23, 2024 at 05:34:49PM -0800, Andres Freund wrote:
> On 2024-02-23 14:58:12 -0600, Nathan Bossart wrote:
>> +/*
>> + * pg_atomic_write_membarrier_u32 - write with barrier semantics.
>> + *
>> + * The write is guaranteed to succeed as a whole, i.e., it's not possible to
>> + * observe a partial write for any reader.  Note that this correctly interacts
>> + * with both pg_atomic_compare_exchange_u32() and
>> + * pg_atomic_read_membarrier_u32().  While this may be less performant than
>> + * pg_atomic_write_u32() and pg_atomic_unlocked_write_u32(), it may be easier
>> + * to reason about correctness with this function in less performance-sensitive
>> + * code.
>> + *
>> + * Full barrier semantics.
>> + */
> 
> The callout to pg_atomic_unlocked_write_u32() is wrong. The reason to use
> pg_atomic_unlocked_write_u32() is for variables where we do not ever want to
> fall back to spinlocks/semaphores, because the underlying variable isn't
> actually shared. In those cases using the other variants is a bug. The only
> use of pg_atomic_unlocked_write_u32() is temp-table buffers which share the
> data structure with the shared buffers case.

I removed the reference to pg_atomic_unlocked_write_u32().

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

  17. Re: locked reads for atomics

    Nathan Bossart <nathandbossart@gmail.com> — 2024-02-29T16:24:57Z

    Committed.  Thank you for reviewing!

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com