RFR: 8271195: Use largest available large page size smaller than LargePageSizeInBytes when available [v10]

[Stefan Johansson]

On Wed, 23 Feb 2022 08:38:28 GMT, Swati Sharma <duke at openjdk.java.net> wrote:

>> Hi Team,
>> 
>> In this patch I have fixed two issues related to large pages, following is the summary of changes :-
>> 
>> 1. Patch fixes existing large page allocation functionality where if a commit over 1GB pages fails allocation should happen over next small page size i.e. 2M where as currently its happening over 4kb pages resulting into significant TLB miss penalty.
>> Patch includes new JTREG Test case covering various scenarios for checking the correct explicit page allocation according ​to the 1G, 2M, 4K priority.
>> 2. While attempting commit over larger pages we first try to reserve requested bytes over the virtual address space, in case commit to large page fails we should be un reserving entire reservation to avoid leaving any leaks in virtual address space.
>> 
>> ​
>> Please find below the performance data with and without patch for the JMH benchmark included with the patch.
>> 
>> ![image](https://user-images.githubusercontent.com/96874289/152189587-4822a4ca-f5e2-4621-b405-0da941485143.png)
>> 
>> 
>> Please review and provide your valuable comments.
>> 
>> 
>> 
>> Thanks,
>> Swati Sharma
>> Runtime Software Development Engineer 
>> Intel
>
> Swati Sharma has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8271195: TestCase issue resolved.

Test is now working but I still have some additional comments.

test/hotspot/jtreg/runtime/os/TestExplicitPageAllocation.java line 50:

> 48: 
> 49:     private static final String DIR_HUGE_PAGES = "/sys/kernel/mm/hugepages/";
> 50:     private static final long HEAP_SIZE_IN_KB = 1 * 1024 * 1024;

This heap size is used to calculate the required number of pages needed but it is not used when creating the process, so the calculation will be incorrect. I suggest you use this value in the process creation as well, to avoid accidentally change one value but not the other.

test/hotspot/jtreg/runtime/os/TestExplicitPageAllocation.java line 70:

> 68:                 if (pageSizes[i]) {
> 69:                     testCase(i);
> 70:                     break;

Can you please motivate this break? Why do we only want to test the largest page size? And if this is the case why not just record the largest page size during the setup-phase?

test/hotspot/jtreg/runtime/os/TestExplicitPageAllocation.java line 93:

> 91:            return Long.toString(sizeInBytes / m) + "M";
> 92:         else
> 93:            return Long.toString(sizeInBytes / g) + "G";

Please add { } to the if-statements.

test/hotspot/jtreg/runtime/os/TestExplicitPageAllocation.java line 171:

> 169:         if (errorMessage != "") {
> 170:             throw new AssertionError(errorMessage);
> 171:         }

So this now works, but I think it is pretty hard to follow the logic with the continues and breaks and I don't see any benefit from not just throwing the `AssertionError()` when a failure occurs. 

What do you think about something like this:

for (int i = index; i >= vmPageSizeIndex; i--) {
    if (pageSizes[i]) {
        String size = sizeFromIndex(i);
        System.out.println("Checking allocation for " + size);
        if (checkOutput(output, size)) {
            System.out.println("TestCase Passed for pagesize: " + pageSize + ", allocated pagesize: " + size);
            // Page allocation succeeded no need to check any more page sizes.
            return;
        } else {
            // Only consider this a test failure if there are enough configured
            // pages to allow this reservation to succeeded.
            if (requiredPageCount(i) <= pageCount[i]) {
                throw new AssertionError("TestCase Failed for " + size + " page allocation. " +
                                         "Required pages: " + requiredPageCount(i) + ", " +
                                         "Configured pages: " + pageCount[i]);
            }
        }
    }
}


I ran this through our testing and it also works.

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

Changes requested by sjohanss (Reviewer).

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