Re: LogwrtResult contended spinlock - Mailing list pgsql-hackers
From | Alvaro Herrera |
---|---|
Subject | Re: LogwrtResult contended spinlock |
Date | |
Msg-id | 202407021455.jilgqot55gdg@alvherre.pgsql Whole thread Raw |
In response to | Re: LogwrtResult contended spinlock (Andres Freund <andres@anarazel.de>) |
Responses |
Re: LogwrtResult contended spinlock
|
List | pgsql-hackers |
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)
Attachment
pgsql-hackers by date: