public inbox for [email protected]
help / color / mirror / Atom feedFrom: Andres Freund <[email protected]>
To: Alvaro Herrera <[email protected]>
Cc: Alexander Lakhin <[email protected]>
Cc: Bharath Rupireddy <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Jeff Davis <[email protected]>
Cc: [email protected]
Cc: Jaime Casanova <[email protected]>
Subject: Re: LogwrtResult contended spinlock
Date: Mon, 1 Jul 2024 10:17:03 -0700
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<[email protected]>
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 <[email protected]>
> 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 <[email protected]>
> Discussion: https://postgr.es/m/[email protected]
> ---
> 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
view thread (5+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
Subject: Re: LogwrtResult contended spinlock
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox