Re: LogwrtResult contended spinlock

Hi,

On 2024-07-01 11:10:24 +0200, Alvaro Herrera wrote:
> In the meantime I noticed that pg_attribute_aligned() is not supported
> in every platform/compiler, so for safety sake I think it's better to go
> with what we do for PGAlignedBlock: use a union with a double member.
> That should be 8-byte aligned on x86 as well, unless I misunderstand.

If a platform wants to support 8 byte atomics, it better provides a way to
make variables 8 bytes aligned. We already rely on that, actually. See use of
pg_attribute_aligned in e.g. src/include/port/atomics/generic-msvc.h



> From 9d240e90f87bf8b53bd5d92b623e33419db64717 Mon Sep 17 00:00:00 2001
> From: Alvaro Herrera <alvherre@alvh.no-ip.org>
> Date: Mon, 1 Jul 2024 10:41:06 +0200
> Subject: [PATCH v2] Fix alignment of variable in
>  pg_atomic_monotonic_advance_u64
> 
> Reported-by: Alexander Lakhin <exclusion@gmail.com>
> Discussion: https://postgr.es/m/36796438-a718-cf9b-2071-b2c1b947c1b5@gmail.com
> ---
>  src/include/port/atomics.h | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
> index 78987f3154..964732e660 100644
> --- a/src/include/port/atomics.h
> +++ b/src/include/port/atomics.h
> @@ -580,30 +580,38 @@ pg_atomic_sub_fetch_u64(volatile pg_atomic_uint64 *ptr, int64 sub_)
>  static inline uint64
>  pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
>  {
> -    uint64        currval;
> +    /*
> +     * On 32-bit machines, declaring a bare uint64 variable doesn't promise
> +     * the alignment we need, so coerce the compiler this way.
> +     */
> +    union
> +    {
> +        uint64        u64;
> +        double        force_align_d;
> +    }            currval;

I wonder if we should just relax the alignment requirement for currval. It's
crucial that the pointer is atomically aligned (atomic ops across pages are
either forbidden or extremely slow), but it's far from obvious that it's
crucial for comparator value to be aligned.


>  #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
>      AssertPointerAlignment(ptr, 8);
>  #endif

What's the point of this assert, btw? This stuff is already asserted in lower
level routines, so it just seems redundant to have it here?


> -    currval = pg_atomic_read_u64_impl(ptr);
> -    if (currval >= target_)
> +    currval.u64 = pg_atomic_read_u64_impl(ptr);
> +    if (currval.u64 >= target_)
>      {
>          pg_memory_barrier();
> -        return currval;
> +        return currval.u64;
>      }
>  
>  #ifndef PG_HAVE_ATOMIC_U64_SIMULATION
> -    AssertPointerAlignment(&currval, 8);
> +    AssertPointerAlignment(&currval.u64, 8);
>  #endif
>  
> -    while (currval < target_)
> +    while (currval.u64 < target_)
>      {
> -        if (pg_atomic_compare_exchange_u64_impl(ptr, &currval, target_))
> +        if (pg_atomic_compare_exchange_u64_impl(ptr, &currval.u64, target_))
>              break;
>      }
>  
> -    return Max(target_, currval);
> +    return Max(target_, currval.u64);

What does the Max() actually achieve here? Shouldn't it be impossible to reach
this with  currval < target_?

And why does target_ end in an underscore?

Greetings,

Andres Freund