Re: locked reads for atomics

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