Faulty pg_get_shmem_pagesize() on Windows
Hi all,
While working on resizable shared memory, I noticed an inconsistency
in pg_get_shmem_pagesize(). Attached are two patches fixing that
inconsistency.
0001: Fix pg_get_shmem_pagesize() on Windows
When huge pages are in use, pg_get_shmem_pagesize() calls
GetHugePageSize(). On Windows, however, that function is a
compatibility stub returning zero. The patch uses
GetLargePageMinimum() instead when large pages are actually in use.
The only caller of this function today rejects unsupported platforms
before reaching it. I don't think there is a current user-visible
failure on Windows. But we are not restricting its usage on Windows
and thus a future caller may face problems. I am proposing this for
the master rather than requesting a backpatch since it's purely
pro-active fix.
I looked through the relevant earlier discussions (CC'ed relevant people).
Commit 43c1c4f65eab77bcfc4f535a7e9ac0421e0cf2a5 introduced
shared_memory_size_in_huge_pages. Its thread [1]/messages/by-id/F2772387-CE0F-46BF-B5F1-CC55516EB885@amazon.com explicitly discussed
Windows reporting. An early patch used GetLargePageMinimum(), but the
participants agreed that the required page count was not particularly
useful on Windows and left the GUC at -1 [2]/messages/by-id/7ED221D0-0D75-46F8-8D6C-8AF70D8EABE7@amazon.com. A later exchange
clarified that Windows has no equivalent of Linux's vm.nr_hugepages
setting [3]/messages/by-id/YmXoIEaR2CWRV7KP@paquier.xyz. Considering those discussions, the attached patch leaves
the stub and that GUC behavior unchanged.
Commit 3887d0cfeb3315bc4c6219bd2eb90e89cf487fa8 renamed
pg_numa_get_pagesize() to pg_get_shmem_pagesize() and moved it into
shmem.c, retaining the existing lookup logic. In that thread [4]/messages/by-id/CALdSSPi5fj0a7UG7Fmw2cUD1uWuckU_e8dJ+6x-bJEokcSXzqA@mail.gmail.com,
Windows portability and the requirement to call the function after
shared-memory initialization were discussed [5]/messages/by-id/tvdbhi7rkk7rvpcm22bj6jnri4ucdb4dnzefer6q7hqflpkxm7@eq37undomnxw. I did not find an
explicit discussion of the zero returned by the Windows huge-page
stub.
Commit 65c298f61fc70f2f960437c05649f71b862e2c48 originally introduced
pg_numa_get_pagesize(), including its call to GetHugePageSize(). In
the original NUMA observability thread [6]/messages/by-id/CAKZiRmxh6KWo0aqRqvmcoaX2jUxZYb4kGp3N=q1w+DiH-696Xw@mail.gmail.com, Windows portability was
considered: one exchange addressed the absence of sysconf() on Windows
[7]: /messages/by-id/CAKZiRmz_ERUH0udcZgpAinBSqpWoOYeao_rNNEe9f8Z9asTihw@mail.gmail.com
and requiring shared-memory initialization before calling the function
[5]: /messages/by-id/tvdbhi7rkk7rvpcm22bj6jnri4ucdb4dnzefer6q7hqflpkxm7@eq37undomnxw
GetHugePageSize() stub returning zero.
It looks like the faulty use of GetHugePageSize() on Windows was never
noticed or considered.
0002: Record the main shared memory segment's page size
Rather than determining the page size again in
pg_get_shmem_pagesize(), this patch records it in PGShmemHeader during
allocation and makes pg_get_shmem_pagesize() return the recorded
value. That way we eliminate any future possibility of this function
returning a different size than actually used - hoping that every
method of allocating shared memory will continue to record page size.
This associates the value with the successful allocation, including
ordinary-page fallback with huge_pages=try. It remains constant for
the mapping's lifetime and is initialized afresh if shared memory is
recreated, for example after a crash.
It also avoids repeated page-size discovery. In particular, on Linux,
the current huge-page path opens and scans /proc/meminfo on every
call. I have not measured a significant performance benefit for the
existing NUMA reporting caller; the additional motivation is that
resizable shared-memory support will need the page size during
resizing and protection changes.
I could not find a proposal to retain the page size in the
shared-memory header, or a rejection of that approach, in either of
the above threads.
The second patch supersedes the accessor implementation changed by the
first, but I have kept the Windows correction separate so it can be
considered independently.
[1]: /messages/by-id/F2772387-CE0F-46BF-B5F1-CC55516EB885@amazon.com
[2]: /messages/by-id/7ED221D0-0D75-46F8-8D6C-8AF70D8EABE7@amazon.com
[3]: /messages/by-id/YmXoIEaR2CWRV7KP@paquier.xyz
[4]: /messages/by-id/CALdSSPi5fj0a7UG7Fmw2cUD1uWuckU_e8dJ+6x-bJEokcSXzqA@mail.gmail.com
[5]: /messages/by-id/tvdbhi7rkk7rvpcm22bj6jnri4ucdb4dnzefer6q7hqflpkxm7@eq37undomnxw
[6]: /messages/by-id/CAKZiRmxh6KWo0aqRqvmcoaX2jUxZYb4kGp3N=q1w+DiH-696Xw@mail.gmail.com
[7]: /messages/by-id/CAKZiRmz_ERUH0udcZgpAinBSqpWoOYeao_rNNEe9f8Z9asTihw@mail.gmail.com
--
Best Wishes,
Ashutosh Bapat