On Tue, Feb 13, 2024 at 10:01 AM Dave Cramer <davecramer@postgres.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
0001-Optionally-do-port-atomics.h-with-stdatomic.h.patch
Description: Binary data
