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

[Stefan Johansson]

On Thu, 17 Feb 2022 15:52:52 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: Review comments resolved

Thanks for updating the test. Here is a first set of comments. 

I also just realized that even though the test doesn't show as a failure on aarch64 now it is still not working correctly and I think my suggestion to not use arrays to store the page sizes would help. What I see is that we parse the 16G page size and get an AIOOBE: 

Number of available 16777216kB pages = 0
Exceptionjava.lang.ArrayIndexOutOfBoundsException: Index 32 out of bounds for length 32

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

> 52: class MyClass {
> 53:     public static void main(String args[]) {
> 54:     System.out.println("Inside MyClass");

Indent correctly or just remove. I would also prefer if the class had a name that make it belong more to the test. Something like: `ExplicitPageAllocation`. Another alternative would be to even skip this class and just run the test with `-version`.

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

> 69:     private static String errorMessage = null;
> 70: 
> 71:     private static final int SIZE=32;

Maybe a more descriptive name, like `MAX_NUMBER_OF_PAGESIZES` or something like that.

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

> 153:                     System.out.println("System does not support"+pageSize+"kB pages");
> 154:                     continue;
> 155:                 }

I find these calculations and logic a bit confusing. Some comments would help. I think what you are doing here is to verify that no-one else on the system is using the large pages which is good, but the output "System does not support" feels wrong if the case actually is that all huge pages are used. 

>From what I can tell, in the case when no pages are configured, both free and resv will be 0, thus available will be 0 and the page size will be considered "available", but with a 0 page count.

A simpler approach would just be to check `nr_hugepages` or `free_hugepages` and use their value as available. If it is 0 it is zero we could output something like: 
"No pages configured for page size X"

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

> 162:         // OS vm page size
> 163:         ProcessBuilder processBuilder = new ProcessBuilder();
> 164:         processBuilder.command("getconf", "PAGE_SIZE");

You could use the WhiteBox API to get the default page size to avoid having to start the extra process. You can search the code for `getVMPageSize()` to see other uses.

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

> 186: 
> 187:         for (int i = index;i >= vmPageSizeIndex;i--) {
> 188:             if(pageSizes[i]) {

An alternative to this would be to have something like `ArrayList<PageSizeConfig>` to which we only add the configured page sizes. And `PageSizeConfig` would hold both the page size and the number of pages configured for this size. I think that would be slightly easier to follow.

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

Changes requested by sjohanss (Reviewer).

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