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

[stefan.johansson at oracle.com]

Hi Ivan,

On 2020-05-13 15:56, Ivan Walulya wrote:
> Thanks Kim,
> 
> Suggested modifications have been made in a new webrev:
> 
> https://cr.openjdk.java.net/~iwalulya/8244752/01/
Looks good. We could file a new RFE to investigate if it would make 
sense to support fallback to the default large-page size if we fail the 
mapping with the given size.

Thanks,
Stefan

> 
> //Ivan
> 
>> On 12 May 2020, at 13:39, Kim Barrett <kim.barrett at oracle.com> wrote:
>>
>>> 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)) {
>>
>> ------------------------------------------------------------------------------
>>
>