RFR: JDK-8256155: os::Linux Populate all large_page_sizes, select smallest page size in reserve_memory_special_huge_tlbfs* [v9]

[Thomas Stuefe]

On Tue, 8 Dec 2020 23:25:46 GMT, Marcus G K Williams <github.com+168222+mgkwill at openjdk.org> wrote:

>> Updated with a merge for changes from master. 
>> 
>> It appears that some failures were caused by previous merge. See https://bugs.openjdk.java.net/browse/JDK-8257855
>
> There also appears to be an issue on TestSegments.java. See https://github.com/openjdk/jdk/pull/1688 
> 
> https://bugs.openjdk.java.net/browse/JDK-8257887
> 
> https://github.com/mgkwill/jdk/runs/1520624742#step:13:15

Hi Marcus,

Sorry, I changed my opinion about JDK16.

According to Ivan, one of your last commits broke os::large_page_size(). That is fine, things happen. But what makes me nervous is that it took a reviewer to find this, this should have popped up in tests right away (eg runtime/test_os.cpp, "os_pagesizes" test). So we have holes in test coverage or in the way the tests are executed.

The GH actions are of course not enough, since they do not run with large pages to my knowledge. What would be needed, in my opinion:
- one jtreg test to test that the VM comes up with `-XX:+UseLargePages -XX:LargePageSizeInBytes=1G` and allocates small-large-pages as expected. This is not only needed as a function proof but to prevent regressions when we reform the code (which will happen)
- We should have a gtest run with large pages. I opened https://bugs.openjdk.java.net/browse/JDK-8257959 to track that.
- This patch changes behavior insofar as that now we return memory to the caller with a page size which he may not expect. We _think_ this is fine, since committing/uncommitting this memory is disabled. But since this is used by GC and Compiler and potentially other consumers as well, this should be thoroughly tested. I think tier1...3 at least, plus gc stress tests? probably with LargePageSizeInBytes=1G specified for all those tests.

Side note: gtests are not bound to the build. You can run them manually by launching the gtestlauncher:
`./hotspot/variant-xxx/libjvm/gtest/gtestLauncher -jdk:./images/jdk`

You can add VM options to it, e.g.:
`./hotspot/variant-xxx/libjvm/gtest/gtestLauncher -jdk:./images/jdk -Xmx128m -XX:+UseLargePages -XX:LargePageSizeInBytes=1G`

and you probably should run this at least manually for your patch. Note caveat: death tests will fail with LP, see JDK-8257229.

Figuring all this out would be something we would assist you with. But you try to push this into JDK16, whose deadline is tomorrow. That puts us into an awkward position. The way it is now, we only could integrate this without having run many tests, without regression testing and without testing on non-Intel platforms. I do not think this is the right way.

Note that if you think JDK16 is important you always can backport changes to older releases once the patch is in JDK17 and has been tested there.

Cheers, Thomas

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

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