From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Andres Freund <andres@anarazel.de>
Cc: Alexander Lakhin <exclusion@gmail.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jeff Davis <pgsql@j-davis.com>
Cc: pgsql-hackers@lists.postgresql.org, Jaime Casanova <jcasanov@systemguards.com.ec>
Subject: Re: LogwrtResult contended spinlock
Date: Tue, 2 Jul 2024 16:55:01 +0200
Message-ID: <202407021455.jilgqot55gdg@alvherre.pgsql> (raw)
In-Reply-To: <20240702121759.jmig2lz6eytpm3lb@alap3.anarazel.de>
On 2024-Jul-02, Andres Freund wrote:
> On 2024-07-01 21:12:25 +0200, Alvaro Herrera wrote:
> > On 2024-Jul-01, Andres Freund wrote:
> > I'm pretty sure the Microsoft docs I linked to are saying it must be
> > aligned.
>
> I don't think so:
> https://learn.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-interlockedcompareexchange64
>
> LONG64 InterlockedCompareExchange64(
> [in, out] LONG64 volatile *Destination,
> [in] LONG64 ExChange,
> [in] LONG64 Comperand
> );
>
> Note that Destination is the only argument passed by reference (and thus the
> caller controls alignment of the in-memory value). ExChange is passed by
> value, so we don't control alignment in any way.
Hmm, you're right, assuming LONG64 is passed by value. Here
https://learn.microsoft.com/en-us/windows/win32/winprog/windows-data-types
it says that the type is declared as
typedef __int64 LONG64;
and
https://learn.microsoft.com/en-us/cpp/cpp/int8-int16-int32-int64?view=msvc-170
says that __int64 is a normal integer type. So yes, 'ExChange' is
passed by value and therefore testing it for alignment is useless on
this platform.
> > There are in some of them, but not in pg_atomic_compare_exchange_u64_impl.
>
> But there's one in pg_atomic_read_u64_impl().
Sure, but pg_atomic_read_u64 is given 'ptr', not 'currval'.
> But I actually think it's wrong for pg_atomic_monotonic_advance_u64()
> to use _impl(), that's just for the wrapper functions around the
> implementation. Wheras pg_atomic_monotonic_advance_u64() should just
> use the generic interface.
True. Well, I can remove the assertion from
pg_atomic_monotonic_advance_u64 and use pg_atomic_compare_exchange_u64
instead. But that one does this:
static inline bool
pg_atomic_compare_exchange_u64(volatile pg_atomic_uint64 *ptr,
uint64 *expected, uint64 newval)
{
#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
AssertPointerAlignment(ptr, 8);
AssertPointerAlignment(expected, 8);
#endif
return pg_atomic_compare_exchange_u64_impl(ptr, expected, newval);
}
AFAICS this is still going to fail, because uint64 *expected comes from
our &currval, which was not aligned before so it'll still be unaligned
now. The only difference is that the assertion failure will be in
pg_atomic_compare_exchange_u64 instead of in
pg_atomic_monotonic_advance_u64.
Other platforms do have the 'expected' argument as a pointer, so the
assertion there is not completely stupid. I think we could move the
alignment assertions to appear inside the platform-specific _impl
routines that need it, and refrain from adding it to the MSVC one.
> > > > - 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_?
> >
> > When two processes are hitting the cmpxchg concurrently, we need to
> > return the highest value that was written, even if it was the other
> > process that did it.
>
> Sure. That explains needing to use currval. But not really needing to use
> Max(). If cmpxchg succeeds, we need to return target_, if the loop terminates
> otherwise we need to return currval. No?
Oh, you're suggesting to change the break statement with a return.
Seems reasonable.
> > > And why does target_ end in an underscore?
> >
> > Heh, you tell me -- I just copied the style of the functions just above.
>
> IIRC using plain "and" "or" "add" caused conflicts with some headers or such.
Ah, that makes sense. It should be no problem to remove the underscore.
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La primera ley de las demostraciones en vivo es: no trate de usar el sistema.
Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen)
Attachments:
[text/x-diff] v5-0001-Remove-bogus-assertion-in-pg_atomic_monotonic_adv.patch (4.9K, ../202407021455.jilgqot55gdg@alvherre.pgsql/2-v5-0001-Remove-bogus-assertion-in-pg_atomic_monotonic_adv.patch)
download | inline diff:
From f9df825c8c61f3e4c8597af3cfbccc5d69967872 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
Date: Tue, 2 Jul 2024 13:42:46 +0200
Subject: [PATCH v5] Remove bogus assertion in pg_atomic_monotonic_advance_u64
This code wanted to ensure that the 'exchange' variable passed to
pg_atomic_compare_exchange_u64 has correct alignment, but apparently
platforms don't actually require anything that doesn't come naturally.
While messing with pg_atomic_monotonic_advance_u64: instead of using
Max() to determine the value to return, just use
pg_atomic_compare_exchange_u64()'s return value to decide; also, use
pg_atomic_compare_exchange_u64 instead of the _impl version; also remove
the unnecessary underscore at the end of variable name "target".
Backpatch to 17.
Reported-by: Alexander Lakhin <exclusion@gmail.com>
Discussion: https://postgr.es/m/36796438-a718-cf9b-2071-b2c1b947c1b5@gmail.com
---
src/include/port/atomics.h | 17 ++++++-----------
src/include/port/atomics/arch-ppc.h | 2 ++
src/include/port/atomics/arch-x86.h | 2 ++
src/include/port/atomics/generic-gcc.h | 3 +++
src/include/port/atomics/generic-sunpro.h | 1 +
5 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.hindex c911c6b956..f6fa432d2d 100644--- a/src/include/port/atomics.h+++ b/src/include/port/atomics.h@@ -507,7 +507,6 @@ pg_atomic_compare_exchange_u64(volatile pg_atomic_uint64 *ptr,
{
#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
AssertPointerAlignment(ptr, 8);
- AssertPointerAlignment(expected, 8);
#endif
return pg_atomic_compare_exchange_u64_impl(ptr, expected, newval);
}
@@ -576,7 +575,7 @@ pg_atomic_sub_fetch_u64(volatile pg_atomic_uint64 *ptr, int64 sub_)
* Full barrier semantics (even when value is unchanged).
*/
static inline uint64
-pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)+pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target)
{
uint64 currval;
@@ -585,23 +584,19 @@ pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
#endif
currval = pg_atomic_read_u64_impl(ptr);
- if (currval >= target_)+ if (currval >= target)
{
pg_memory_barrier();
return currval;
}
-#ifndef PG_HAVE_ATOMIC_U64_SIMULATION- AssertPointerAlignment(&currval, 8);-#endif-- while (currval < target_)+ while (currval < target)
{
- if (pg_atomic_compare_exchange_u64_impl(ptr, &currval, target_))- break;+ if (pg_atomic_compare_exchange_u64(ptr, &currval, target))+ return target;
}
- return Max(target_, currval);+ return currval;
}
#undef INSIDE_ATOMICS_H
diff --git a/src/include/port/atomics/arch-ppc.h b/src/include/port/atomics/arch-ppc.hindex 94ba2597fb..edaab7c895 100644--- a/src/include/port/atomics/arch-ppc.h+++ b/src/include/port/atomics/arch-ppc.h@@ -173,6 +173,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
uint32 condition_register;
bool ret;
+ AssertPointerAlignment(expected, 8);+
/* Like u32, but s/lwarx/ldarx/; s/stwcx/stdcx/; s/cmpw/cmpd/ */
#ifdef HAVE_I_CONSTRAINT__BUILTIN_CONSTANT_P
if (__builtin_constant_p(*expected) &&
diff --git a/src/include/port/atomics/arch-x86.h b/src/include/port/atomics/arch-x86.hindex 3efa79dc3d..2a8eca30fc 100644--- a/src/include/port/atomics/arch-x86.h+++ b/src/include/port/atomics/arch-x86.h@@ -207,6 +207,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
{
char ret;
+ AssertPointerAlignment(expected, 8);+
/*
* Perform cmpxchg and use the zero flag which it implicitly sets when
* equal to measure the success.
diff --git a/src/include/port/atomics/generic-gcc.h b/src/include/port/atomics/generic-gcc.hindex 9d91370fa8..872d2f02af 100644--- a/src/include/port/atomics/generic-gcc.h+++ b/src/include/port/atomics/generic-gcc.h@@ -240,6 +240,7 @@ static inline bool
pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
uint64 *expected, uint64 newval)
{
+ AssertPointerAlignment(expected, 8);
return __atomic_compare_exchange_n(&ptr->value, expected, newval, false,
__ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST);
}
@@ -253,6 +254,8 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
{
bool ret;
uint64 current;
++ AssertPointerAlignment(expected, 8);
current = __sync_val_compare_and_swap(&ptr->value, *expected, newval);
ret = current == *expected;
*expected = current;
diff --git a/src/include/port/atomics/generic-sunpro.h b/src/include/port/atomics/generic-sunpro.hindex e060c0868a..840a45e778 100644--- a/src/include/port/atomics/generic-sunpro.h+++ b/src/include/port/atomics/generic-sunpro.h@@ -102,6 +102,7 @@ pg_atomic_compare_exchange_u64_impl(volatile pg_atomic_uint64 *ptr,
bool ret;
uint64 current;
+ AssertPointerAlignment(expected, 8);
current = atomic_cas_64(&ptr->value, *expected, newval);
ret = current == *expected;
*expected = current;
--
2.39.2
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: pgsql-hackers@postgresql.org
Cc: alvherre@alvh.no-ip.org, andres@anarazel.de, exclusion@gmail.com, bharath.rupireddyforpostgres@gmail.com, tgl@sss.pgh.pa.us, pgsql@j-davis.com, jcasanov@systemguards.com.ec
Subject: Re: LogwrtResult contended spinlock
In-Reply-To: <202407021455.jilgqot55gdg@alvherre.pgsql>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox