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)
>From f9df825c8c61f3e4c8597af3cfbccc5d69967872 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@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 <exclus...@gmail.com> Discussion: https://postgr.es/m/36796438-a718-cf9b-2071-b2c1b947c...@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.h index 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.h index 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.h index 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.h index 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.h index 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
