Re: [PATCH] Add native windows on arm64 support

From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
To: Dave Cramer <davecramer(at)postgres(dot)rocks>
Cc: Andres Freund <andres(at)anarazel(dot)de>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Michael Paquier <michael(at)paquier(dot)xyz>, Anthony Roberts <anthony(dot)roberts(at)linaro(dot)org>, Daniel Gustafsson <daniel(at)yesql(dot)se>, Lina Iyer <lina(dot)iyer(at)linaro(dot)org>, Mike Holmes <mike(dot)holmes(at)linaro(dot)org>, Peter Eisentraut <peter(at)eisentraut(dot)org>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: Re: [PATCH] Add native windows on arm64 support
Date: 2024-09-29 00:02:21
Message-ID: CA+hUKG+2E-yQ-zg5pvUAh8QPfOF6R=yf_PiKXJ0o+j7yV_oSuQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Tue, Feb 13, 2024 at 10:01 AM Dave Cramer <davecramer(at)postgres(dot)rocks> wrote:
> > postgres.exe!dsa_free(dsa_area * area, unsigned __int64 dp) Line 869 C
> postgres.exe!resize(dshash_table * hash_table, unsigned __int64 new_size_log2) Line 879 C
> postgres.exe!dshash_find_or_insert(dshash_table * hash_table, const void * key, bool * found) Line 480 C
> postgres.exe!pgstat_get_entry_ref(PgStat_Kind kind, unsigned int dboid, unsigned int objoid, bool create, bool * created_entry) Line 455 C

Hmm, we can see that the spinlock stuff is too weak (assumes TSO), but
there aren't any spinlocks near that code so I think something else is
wrong. What does pg_read_barrier() compile to? I mention that
because it's used near there in a somewhat complicated way. But I'm a
bit confused because by reading through the code it looks like it
should be too strong, not too weak. I think it should fall back to
pg_memory_barrier_impl() for lack of pg_read_barrier_impl(), which
should produce MemoryBarrier() and I guess DMB SY (ARM instruction for
full system data memory barrier). So maybe some other pg_atomic_XXX
operations are falling back to code that doesn't work.

Just for experimentation, you could see what happens if you redirect
all that stuff to C11's <stdatomic.h>. Here's a quick-and-dirty patch
for that, which works on my ARM Mac, but I have no idea if it'll work
on MSVC (our CI uses MSVC 2019; they only added <stdatomic.h> in MSVC
2022, but that's the same edition that added ARM so you must have it).
It's not a very finely tuned patch (for example I think several calls
should use the _explicit() variants and be weaker, but they err on the
side of being too strong, so the code they generate is probably not as
fast as it could be; I lost interested when the general idea was
rejected for now, but it might help learn something about the MSVC+ARM
situation).

Combined with the patch that redirects spinlocks to atomics[1], there
would be no 'hand rolled' code relating to memory order or atomics,
just standard modern C. There are obviously other architecture tests
here and there, and some of them will be wrong (some of them are wrong
already for MSVC on x86), and that might be fixed by project
standardisation[2].

[1] https://www.postgresql.org/message-id/CA%2BhUKGJ%2BoA%2B62iUZ-EQb5R2cAOW3Y942ZoOtzOD%3D1sQ05iNg6Q%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CA%2BhUKG%2BOu%2BC%2BpXioo_W1gKcwRCEN1WNrLBBPSMJyxjgc%2B1JEJA%40mail.gmail.com

Attachment Content-Type Size
0001-Optionally-do-port-atomics.h-with-stdatomic.h.patch application/octet-stream 8.1 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2024-09-29 01:19:11 Re: First draft of PG 17 release notes
Previous Message Tom Lane 2024-09-28 22:59:02 Re: pg_verifybackup: TAR format backup verification