RFR: 8256155: 2M large pages for code when LargePageSizeInBytes is set to 1G for heap

[Thomas Stuefe]

On Wed, 11 Nov 2020 01:48:46 GMT, Marcus G K Williams <github.com+168222+mgkwill at openjdk.org> wrote:

> Use 2m pages for executable large
> pages and in large page requests less
> than 1g on linux.
> 
> - Add os::exec_large_page_size() that
> returns 2m as size
> - Add os::select_large_page_size() to return
> correct large page size for size_t bytes
> - Add 2m size to _page_sizes array
> - Update reserve_memory_special methods
> to set/use large_page_size based on exec
> size
> - Update large page not reserved warnings
> to include large_page_size attempted
> - Update TestLargePageUseForAuxMemory.java
> to expect 2m large pages in some instances
> 
> Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>

Hi,

this seems like a improvement for a very specific scenario (2M instead of 1G pages on x86(?) Linux(?)). At the moment this feels more like an early prototype. The lack of comments/documentation is not helping.

Both JBS and PR are a bit taciturn. It would help if you could elaborate a bit. E.g. is this just for Linux? for x86 only? since the ticket talks about 4K pages, which are not universal across all architectures.

I can glean some of what you want to do from the patch itself, but the spec is vague so there is no way to verify if the patch matches the spec.

What does page size have to do with exec permission? This should not be tied to exec. The whole patch should not contain the word "exec" :)

Why is this proposal hard coded to 2M pages?

What memory regions are supposed to be affected by this? JBS ticket talks about "code, card table and other". 

One problem I see is that the notion of "we have a small standard page and a single large pinned page size" is - I believe - baked in into a few places. Are there any places where an implicit assumption of the page size or their "pinned-ness" could break things now (see also below remark about UseSHM)? For instance, are these pages pinned on all our platforms, and if no, could code be affected which commits/uncommits and assumes a certain page size?

What tests have you run? On what platforms? Also platforms with different page sizes? How well did you test UseSHM?

The latter is interesting because arguably there is the bigger behavioral change. TLBFS path was using a mixture of large and small pages anyway, so adding another page size into the mix is not a big stretch. But for SHM, things would change: where before reserve_memory_special would return NULL and we'd invoke fallback reservation, now we return a region consisting of 2M pinned pages.

For SHM, I think you need to make sure that alignment matches SHMLBA?

It was not clear from the patch or the JBS item whether you propose to change the semantics of LargePageSizeInBytes. E.g. what happens if the value specified explicitely is smaller than your exec_page size? Your patch seems to give preference to exec_page_size. But this would be a behavioral change, and may need a CSR.

Finally, comments would be nice. Clear API specs. Extending regression tests for reserve_memory_special would be good too, to test the new behavior (for gtests examples, see test/hotspot/gtest/runtime in the source folder).

The linux-2m-page-specific code in the platform-generic G1 test seems wrong.

Cheers, Thomas

src/hotspot/os/linux/os_linux.cpp line 3970:

> 3968:                                             char* req_addr, bool exec) {
> 3969:   size_t large_page_size;
> 3970:   large_page_size = os::select_large_page_size(bytes, exec);

The "os" is shared and platform generic. Please don't add anything there unless you write (and test as much as possible) the different platforms. I do not see why this API should even be exported from this unit.

src/hotspot/os/linux/os_linux.cpp line 4002:

> 4000:     char msg[128];
> 4001:     jio_snprintf(msg, sizeof(msg), "Failed to reserve shared memory with large_page_size: " SIZE_FORMAT ".", large_page_size);
> 4002:     shm_warning_format_with_errno("%s", msg);

Why the double printf here? But you can just use Univeral Logging ` log_info(os)("..") `. See e.g. thread creation in this file for examples.

test/hotspot/jtreg/gc/g1/TestLargePageUseForAuxMemory.java line 80:

> 78:     }
> 79: 
> 80:     static void testVM(String what, long heapsize, boolean cardsShouldUseLargePages, boolean bitmapShouldUseLargePages, boolean largePages2m) throws Exception {

Having this linux-specific stuff in a generic G1 test :(

test/hotspot/jtreg/gc/g1/TestLargePageUseForAuxMemory.java line 150:

> 148:         if (Platform.isLinux() && largePageSize != largePageExecSize) {
> 149:             try {
> 150:                 Scanner scan_hugepages = new Scanner(new File("/sys/kernel/mm/hugepages/hugepages-2048kB/nr_hugepages"));

2M hard coded.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1153