Re: LogwrtResult contended spinlock

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)