RFR: 8256155: Allow multiple large page sizes to be used on Linux [v32]

[Stefan Johansson]

On Fri, 7 May 2021 16:16:22 GMT, Marcus G K Williams <mgkwill at openjdk.org> wrote:

>> Change the meaning of LargePageSizeInBytes to be the maximum large page size the JVM may use (not the only one). A default value of zero will mean to allow the JVM use large page sizes up to the system's default large page size.
>
> Marcus G K Williams has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 53 commits:
> 
>  - Remove reserve_memory_special_huge_tlbfs mods except assert
>    
>    Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>
>  - Merge branch 'master' into update_hlp
>  - Merge branch 'master' into update_hlp
>  - Remove extranous vm_page_size check
>    
>    Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>
>  - kstefanj review
>    
>    Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>
>  - Set LargePageSizeInBytes to largepage upper limit
>    
>    Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>
>  - Merge branch 'master' into update_hlp
>  - Fix merge
>    
>    Signed-off-by: Marcus G K Williams <marcus.williams at intel.com>
>  - Merge branch 'master' into update_hlp
>  - Merge branch 'master' into update_hlp
>  - ... and 43 more: https://git.openjdk.java.net/jdk/compare/0790e601...7368e7d4

Some additional comments, mostly small nits. But this is really starting to look ready.

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

> 3605:         if (x && fgets(buf, sizeof(buf), fp) && strcmp(buf, " kB\n") == 0) {
> 3606:           default_large_page_size = x * K;
> 3607:           return default_large_page_size;

If we return here the we will not close the file, consider going back to using `break` or adding a call to `fclose(fp)`

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

> 3625:   // to discover the available page sizes
> 3626:   const char* sys_hugepages = "/sys/kernel/mm/hugepages";
> 3627:   static os::PageSizes page_sizes;

Any reason for this being `static`.

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

> 3750:   } else {
> 3751:     _large_page_size = default_large_page_size;
> 3752:   }

I would prefer turning this around a bit. Currently this will trigger an assert in all_large_pages.contains() if you set `LargePageSizeInBytes=0`. What do you think of instead first checking for the "default" case. Something like:
Suggestion:

  if (FLAG_IS_DEFAULT(LargePageSizeInBytes) ||
      LargePageSizeInBytes == 0 ||
      LargePageSizeInBytes == default_large_page_size) {
    _large_page_size = default_large_page_size;
    log_info(pagesize)("Using the default large page size: " SIZE_FORMAT "%s",
                       byte_size_in_exact_unit(_large_page_size),
                       exact_unit_for_byte_size(_large_page_size));
  } else {
    if (all_large_pages.contains(LargePageSizeInBytes)) {
      _large_page_size = LargePageSizeInBytes;
      log_info(pagesize)("Overriding default large page size (" SIZE_FORMAT "%s) "
                         "using LargePageSizeInBytes: " SIZE_FORMAT "%s",
                         byte_size_in_exact_unit(default_large_page_size),
                         exact_unit_for_byte_size(default_large_page_size),
                         byte_size_in_exact_unit(_large_page_size),
                         exact_unit_for_byte_size(_large_page_size));
    } else {
      _large_page_size = default_large_page_size;
      log_info(pagesize)("LargePageSizeInBytes is not a valid large page size (" SIZE_FORMAT "%s) "
                         "using the default large page size: " SIZE_FORMAT "%s",
                         byte_size_in_exact_unit(LargePageSizeInBytes),
                         exact_unit_for_byte_size(LargePageSizeInBytes),
                         byte_size_in_exact_unit(_large_page_size),
                         exact_unit_for_byte_size(_large_page_size));
    }
  }


I also added some additional logging for the three different cases.

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

> 3977:                                                         SIZE_FORMAT,
> 3978:                                                         page_size,
> 3979:                                                         (size_t)os::vm_page_size());

I would prefer keeping the assert message short as in the other cases.
Suggestion:

  assert(page_size > (size_t)os::vm_page_size(), "Must be a large page size");

Please also move it down to below the other assert checking `_page_sizes.contains(...)`

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

Changes requested by sjohanss (Reviewer).

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