From: | Bertrand Drouvot <bertranddrouvot(dot)pg(at)gmail(dot)com> |
---|---|
To: | Tomas Vondra <tomas(at)vondra(dot)me> |
Cc: | Christoph Berg <myon(at)debian(dot)org>, Andres Freund <andres(at)anarazel(dot)de>, Tomas Vondra <tomas(dot)vondra(at)postgresql(dot)org>, pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: pgsql: Introduce pg_shmem_allocations_numa view |
Date: | 2025-06-27 17:33:26 |
Message-ID: | aF7V5hLDOc54QKfP@ip-10-97-1-34.eu-west-3.compute.internal |
Views: | Whole Thread | Raw Message | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-committers pgsql-hackers |
Hi,
On Fri, Jun 27, 2025 at 04:52:08PM +0200, Tomas Vondra wrote:
> Here's three small patches, that should handle the issue
Thanks for the patches!
> 0001 - Adds the batching into pg_numa_query_pages, so that the callers
> don't need to do anything.
>
> The batching doesn't seem to cause any performance regression. 32-bit
> systems can't use that much memory anyway, and on 64-bit systems the
> batch is sufficiently large (1024).
=== 1
-#define pg_numa_touch_mem_if_required(ro_volatile_var, ptr) \
+#define pg_numa_touch_mem_if_required(ptr) \
Looks unrelated, should be in 0002?
=== 2
I thought that it would be better to provide a batch size only in the 32-bit
case (see [1]), but I now think it makes sense to also provide (a larger) one
for non 32-bit (as you did) due to the CFI added in 0003 (as it's also good to
have it for non 32-bit).
> 0002 - Silences the valgrind about the memory touching. It replaces the
> macro with a static inline function, and adds suppressions for both
> 32-bit and 64-bits. The 32-bit may be a bit pointless, because on my
> rpi5 valgrind produces about a bunch of other stuff anyway. But doesn't
> hurt.
>
> The function now looks like this:
>
> static inline void
> pg_numa_touch_mem_if_required(void *ptr)
> {
> volatile uint64 touch pg_attribute_unused();
> touch = *(volatile uint64 *) ptr;
> }
>
> I did a lot of testing on multiple systems to check replacing the macro
> with a static inline function still works - and it seems it does. But if
> someone thinks the function won't work, I'd like to know.
LGTM.
> 0003 - While working on these patches, it occurred to me we could/should
> add CHECK_FOR_INTERRUPTS() into the batch loop. This querying can take
> quite a bit of time, so letting people to interrupt it seems reasonable.
> It wasn't possible with just one call into the kernel, but with the
> batching we can add a CFI.
Yeah, LGTM.
[1]: https://www.postgresql.org/message-id/aFuRoUieUVh%2BpMfZ%40ip-10-97-1-34.eu-west-3.compute.internal
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
From | Date | Subject | |
---|---|---|---|
Next Message | Nathan Bossart | 2025-06-27 18:38:51 | pgsql: Use correct DatumGet*() function in test_shm_mq_main(). |
Previous Message | Tomas Vondra | 2025-06-27 14:52:08 | Re: pgsql: Introduce pg_shmem_allocations_numa view |
From | Date | Subject | |
---|---|---|---|
Next Message | Nathan Bossart | 2025-06-27 18:44:48 | Re: [PATCH] Use DatumGetUInt32() for dsm_attach() in test_shm_mq_main() |
Previous Message | Daniel Gustafsson | 2025-06-27 17:24:28 | libpq OpenSSL and multithreading |