RFR: JDK-8310233: Fix THP detection on Linux [v3]

[David Holmes]

On Tue, 4 Jul 2023 13:02:22 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Today, if we use UseTransparentHugePages, we assume that the *static* hugepage detection we do is valid for THPs:
>> - that THPs use the page size (in hotspot used as "default large page size") found in `/proc/memlimit` "Hugepagesize")
>> - that THPs are enabled if that page size is >0.
>> 
>> Both assumptions are incorrect:
>> 
>> - whether THPs are enabled should be checked at `/sys/kernel/mm/transparent_hugepage/enabled`, which is a tri-state value ("always", "madvise", "never"). THPs are available for the first two states.
>> - The page size employed by `khugepaged` is set in `/sys/kernel/mm/transparent_hugepage/hpage_pmd_size`. It can differ from the default page size used for static hugepages. For example, we could configure a system such that it uses 1G static hugepages, but the THP page size would still be 2M.
>> 
>> ------
>> 
>> About the patch:
>> 
>> This is a limited, minimally invasive patch to fix THP detection. The patch aims to be easy to downport. There is more work to do, which I will do in subsequent RFEs.
>> 
>> Functionally, for *static* (non-THP) pages nothing changes. THP-mode now correctly detects THP support in the OS, and uses the correct page size (see examples below).
>> 
>> -------------
>> 
>> Example 1: System has THPs disabled, but static hugepages (1g, 2m) configured:
>> 
>> 
>> thomas at starfish $ cat /sys/kernel/mm/transparent_hugepage/enabled
>> always madvise [never]
>> thomas at starfish $ cat /proc/meminfo | grep Hugepage
>> Hugepagesize:    1048576 kB
>> 
>> 
>> Without patch, we incorrectly assume THPs are enabled, and that THP page size is 1G (!), which we then proceed and use as heap page size, causing the heap size to be rounded up from 512m -> 1G. But - even though it is printed as "1G page backed" in log output - in reality it will still 4K-page-backed: the `madvise(2)` we use to set the THP page size will be ignored by the system, since THPs are disabled.
>> 
>> 
>> thomas at starfish $ ./images/jdk/bin/java -Xmx512m -XX:+UseLargePages -XX:+UseTransparentHugePages -Xlog:pagesize -version
>> [0.001s][info][pagesize] Using the default large page size: 1G
>> [0.001s][info][pagesize] Usable page sizes: 4k, 2M, 1G
>> ...
>> [0.016s][info][pagesize] Heap:  min=1G max=1G base=0x00000000c0000000 size=1G page_size=1G
>> 
>> 
>> With patch, we correctly refuse to use large pages (and we log more info):
>> 
>> 
>> thomas at starfish $ ./images/jdk/bin/java -Xmx512m -XX:+UseLargePages -XX:+UseTransparentHugePages -Xlog:pagesize -version
>> [0.001s][info][pagesize] Static...
>
> Thomas Stuefe has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Feedback johan

Not an area I am familiar with but scanning through the code this seems reasonable. A few minor comments below.

Thanks.

src/hotspot/os/linux/hugepages.cpp line 92:

> 90: 
> 91: // Given a file that contains a single (integral) number, return that number, 0 and false if failed.
> 92: static bool read_number_file(const char* file, size_t* out) {

Why `bool` return when you don't check it? Also `*out == 0` also indicates failure. Though why use an out parameter instead of just returning the value?

src/hotspot/share/utilities/globalDefinitions.hpp line 413:

> 411: 
> 412: #define EXACTFMT            SIZE_FORMAT "%s"
> 413: #define EXACTFMTARGS(s)     byte_size_in_exact_unit(s), exact_unit_for_byte_size(s)

Not sure this is worthy of inclusion in globalDefinitions. And I struggle to understand the use of "exact" in this context (pre-existing).

test/hotspot/jtreg/runtime/os/HugePageConfiguration.java line 65:

> 63:     }
> 64: 
> 65:     @java.lang.Override

The `java.lang` is not necessary and typically not used.

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

PR Review: https://git.openjdk.org/jdk/pull/14739#pullrequestreview-1529621792
PR Review Comment: https://git.openjdk.org/jdk/pull/14739#discussion_r1263311652
PR Review Comment: https://git.openjdk.org/jdk/pull/14739#discussion_r1263308053
PR Review Comment: https://git.openjdk.org/jdk/pull/14739#discussion_r1263305494