Re: pgsql: Introduce pg_shmem_allocations_numa view

On 6/27/25 19:33, Bertrand Drouvot wrote:
> 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?
> 

Of course, I merged it into the wrong patch. Here's a v2 that fixes
this, and also reworded some of the comments and commit messages a
little bit.

In particular it now uses "chunking" instead of "batching". I believe
bathing is "combining multiple requests into a single one", but we're
doing exactly the opposite - splitting a large request into smaller
ones. Which is what "chunking" does.

> === 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).
> 

Agreed, I think the CFI is a good thing to have.

>> 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.
> 

Thanks!

I plan to push this tomorrow morning.

-- 
Tomas Vondra