public inbox for [email protected]  

help / color / mirror / Atom feed

From: Bertrand Drouvot <[email protected]>
To: Tomas Vondra <[email protected]>
Cc: Christoph Berg <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Tomas Vondra <[email protected]>
Cc: [email protected]
Subject: Re: pgsql: Introduce pg_shmem_allocations_numa view
Date: Fri, 27 Jun 2025 17:33:26 +0000
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>

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

view thread (83+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: pgsql: Introduce pg_shmem_allocations_numa view
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox