RFR: JDK-8310233: Linux: THP initialization is incorrect

[Thomas Stuefe]

On Mon, 3 Jul 2023 09:55:09 GMT, Johan Sjölen <jsjolen 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. THPs are enabled depending on the mode in `/sys/kernel/mm/transparent_hugepage/enabled` (tri-state). And the pagesize used by khugepaged is the one set in `/sys/kernel/mm/transparent_hugepage/hpage_pmd_size`. The latter can differ from the default large page size on the system (e.g. static hugepage default size could be 1g, whereas THP hugepage size is 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 hugepage support: 2M, 1G (default)
>> [0.001s][info][pagesize]   default pagesize: 1G
>> [0.001s][info][pagesize] Transparent hugepage (THP) suppor...
>
> src/hotspot/os/linux/hugepages.cpp line 68:
> 
>> 66: 
>> 67:   FILE *fp = os::fopen("/proc/meminfo", "r");
>> 68:   if (fp) {
> 
> Nit: you could always switch to `if (fp == nullptr) { return 0; }`, reducing indentation for the body that will do actual work.

Absolutely agree, for a future cleanup, see above.

> src/hotspot/os/linux/hugepages.cpp line 93:
> 
>> 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) {
>> 93:   (*out) = 0;
> 
> Unnecessary parens

I'd like to leave it.

While I agree with you, Hotspot style guide says something like "write parentheses if order of ops is unclear". Most of the code in hotspot uses parentheses in assignments like this. I'd like to avoid mixing styles.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14739#discussion_r1250886392
PR Review Comment: https://git.openjdk.org/jdk/pull/14739#discussion_r1250885629