Date: Tue, 24 Jun 2025 13:25:07 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Tomas Vondra <tomas@vondra.me>
Cc: Christoph Berg <myon@debian.org>, Andres Freund <andres@anarazel.de>,
	Tomas Vondra <tomas.vondra@postgresql.org>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: pgsql: Introduce pg_shmem_allocations_numa view
Message-ID: <aFqnM/iiL+MB62dG@ip-10-97-1-34.eu-west-3.compute.internal>
References: <ce305c79-0a68-46ce-b563-ab9f87bb5f20@vondra.me>
 <aFm-ZfL-vz9I2Zmc@msg.df7cb.de>
 <0643ae61-cf9d-482c-9b2c-fb861b24fd22@vondra.me>
 <aFnGQ2lfb8cfukim@msg.df7cb.de>
 <cd16c4a3-2860-46de-a74e-5532d1b2ee39@vondra.me>
 <6342f601-77de-4ee0-8c2a-3deb50ceac5b@vondra.me>
 <aFpg1de9ZfS1QgUt@ip-10-97-1-34.eu-west-3.compute.internal>
 <a3a4fe3d-1a80-4e03-aa8e-150ee15f6c35@vondra.me>
 <aFqHoNXQ/uWKXJ4U@ip-10-97-1-34.eu-west-3.compute.internal>
 <8649a4e3-c60d-4f37-aa6f-e7e7c14c581e@vondra.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <8649a4e3-c60d-4f37-aa6f-e7e7c14c581e@vondra.me>
Archived-At: 
 <https://www.postgresql.org/message-id/aFqnM/iiL%2BMB62dG%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Tue, Jun 24, 2025 at 02:33:59PM +0200, Tomas Vondra wrote:
> 
> 
> On 6/24/25 13:10, Bertrand Drouvot wrote:
> > So, if we look at do_pages_stat() ([1]), we can see that it uses an hardcoded
> > "#define DO_PAGES_STAT_CHUNK_NR 16UL" and that this pointers arithmetic:
> > 
> > "
> >         pages += chunk_nr;
> >         status += chunk_nr;
> > "
> > 
> > is done but has no effect since nr_pages will exit the loop if we use a batch
> > size <= 16.
> > 
> > So if this pointer arithmetic is not correct, (it seems that it should advance
> > by 16 * sizeof(compat_uptr_t) instead) then it has no effect as long as the batch
> > size is <= 16.
> > 
> > Does test_chunk_size also fails at 17 for you?
> 
> Yes, it fails for me at 17 too. So you're saying the access within each
> chunk of 16 elements is OK, but that maybe advancing to the next chunk
> is not quite right?

Yes, I think compat_uptr_t usage is missing in do_pages_stat() (while it's used
in do_pages_move()).

Having a chunk size <= DO_PAGES_STAT_CHUNK_NR ensures we are not affected
by the wrong pointer arithmetic.

> In which case limiting the access to 16 entries
> might be a workaround.

Yes, something like:

diff --git a/src/backend/storage/ipc/shmem.c b/src/backend/storage/ipc/shmem.c
index c9ae3b45b76..070ad2f13e7 100644
--- a/src/backend/storage/ipc/shmem.c
+++ b/src/backend/storage/ipc/shmem.c
@@ -689,8 +689,17 @@ pg_get_shmem_allocations_numa(PG_FUNCTION_ARGS)
                        CHECK_FOR_INTERRUPTS();
                }

-               if (pg_numa_query_pages(0, shm_ent_page_count, page_ptrs, pages_status) == -1)
-                       elog(ERROR, "failed NUMA pages inquiry status: %m");
+               #define NUMA_QUERY_CHUNK_SIZE 16  /* has to be <= DO_PAGES_STAT_CHUNK_NR (do_pages_stat())*/
+
+               for (uint64 chunk_start = 0; chunk_start < shm_ent_page_count; chunk_start += NUMA_QUERY_CHUNK_SIZE) {
+                        uint64 chunk_size = Min(NUMA_QUERY_CHUNK_SIZE, shm_ent_page_count - chunk_start);
+
+                       if (pg_numa_query_pages(0, chunk_size, &page_ptrs[chunk_start],
+                                                                       &pages_status[chunk_start]) == -1)
+                               elog(ERROR, "failed NUMA pages inquiry status: %m");
+               }
+
+               #undef NUMA_QUERY_CHUNK_SIZE

> In any case, this sounds like a kernel bug, right?

yes it sounds like a kernel bug.

> I don't have much
> experience with the kernel code, so don't want to rely too much on my
> interpretation of it.

I don't have that much experience too but I think the issue is in do_pages_stat()
and that "pages += chunk_nr" should be advanced by sizeof(compat_uptr_t) instead.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com