RFR: 8244752: Enable Linux support for multiple huge page sizes -XX:LargePageSizeInBytes

[Kim Barrett]

> On May 11, 2020, at 12:46 PM, Ivan Walulya <ivan.walulya at oracle.com> wrote:
> 
> Hi all,
> 
> Please review this enhancement to enable selecting a desired huge page size on Linux systems that support multiple huge page sizes. 
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8244752
> Webrev: http://cr.openjdk.java.net/~iwalulya/8244752/00/
> Testing: Tier 1 - 3
> 
> 
> //Ivan

------------------------------------------------------------------------------
src/hotspot/os/linux/os_linux.cpp  
3819         sscanf(entry->d_name, "hugepages-%zukB", &page_size) ) {

Implicit boolean.

------------------------------------------------------------------------------
src/hotspot/os/linux/os_linux.cpp 
3845   if (!FLAG_IS_DEFAULT(LargePageSizeInBytes) && LargePageSizeInBytes != _large_page_size ) {
3846      if (is_valid_large_page_size(LargePageSizeInBytes)) {

Inner if appears to be indented 3 spaces rather than the usual 2.

------------------------------------------------------------------------------
src/hotspot/os/linux/os_linux.cpp 
3799 void os::Linux::find_large_page_sizes() {

I think that's not a great name for what this function is doing, which
is collecting large page sizes in _page_sizes[].  However, I think
there are other problems here.

The collection of sizes into _page_sizes[] doesn't conform to the
description of _page_sizes[], which says the array is sorted in
decreasing order.  Here it's whatever order the directory iteration
happens upon them.

I think that the pre-existing code at the end of setup_large_page_size
will fix it up on any reasonable configuration, but I'm not certain of
that and I'm not certain all systems are configured reasonably.

find_large_page_sizes is only called from is_valid_large_page_size,
which is only called by setup_large_page_size.  It seems like it would
be more straightforward to hoist the iteration over /sys/kernel/mm/hugepages
into is_valid_large_page_size and not touch _page_sizes at all there.

------------------------------------------------------------------------------
src/hotspot/os/linux/os_linux.cpp  
4097      flags |= (os::large_page_size() > (1 << (MAP_HUGE_2MB >> MAP_HUGE_SHIFT)))
4098                 ? MAP_HUGE_1GB
4099                 : MAP_HUGE_2MB;

Shouldn't this just be:

  flags |= (os::large_page_size() > (2 * M)) ? MAP_HUGE_1G : MAP_HUGE_2MB;

(Though there's also Thomas's comments to account for.)

There's no particular reason here to think the encoding used for
MAP_HUGE_2MB has any direct relation to a size in bytes.

Similarly here:
4173      flags |= (os::large_page_size() > (1 << (MAP_HUGE_2MB >> MAP_HUGE_SHIFT)))
4174                 ? MAP_HUGE_1GB
4175                 : MAP_HUGE_2MB;

------------------------------------------------------------------------------
src/hotspot/os/linux/os_linux.cpp 
4096   if (!FLAG_IS_DEFAULT(LargePageSizeInBytes)) {

Why is this checking for non-default value for the option? It seems
like this might do the wrong thing if an explicit command line option
was “rejected" because it had an unsupported value.

Similarly here:
4172   if (!FLAG_IS_DEFAULT(LargePageSizeInBytes)) {

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