RFR: JDK-8304954: SegmentedCodeCache fails when using large pages [v4]

[Thomas Stuefe]

On Mon, 24 Jul 2023 11:46:54 GMT, Damon Fenacci <dfenacci at openjdk.org> wrote:

>> # Issue
>> 
>> When large pages are enabled and segmented code cache is used, the VM tries to use one page for each segment. If the amount of reserved code cache is limited, this can make the total size of the code cache bigger than the reserved size, which in turn makes the VM fail, claiming that there is not enough space (e.g. this happens when running `java -XX:+UseLargePages -XX:+SegmentedCodeCache -XX:InitialCodeCacheSize=2g -XX:ReservedCodeCacheSize=2g -XX:LargePageSizeInBytes=1g -Xlog:pagesize*=debug -version`).
>> This behaviour is not correct as the VM should fall back and try with a smaller page size (and print a warning).
>> 
>> # Solution
>> 
>> When reserving heap space for code cache we give a minimum of 8 pages. Since the page size is already calculated right before for segment sizes, it is saved and passed as an argument instead.
>> 
>> https://github.com/openjdk/jdk/blob/67fbd87378a9b3861f1676977f9f2b36052add29/src/hotspot/share/code/codeCache.cpp#L315
>> 
>> Additionally a warning is printed if large pages are enabled and we end up using a smaller page sizes for code caching.
>> 
>> # Test
>> 
>> The regression test runs a new VM with `-XX:+UseLargePages -XX:LargePageSizeInBytes=1g`. The `main` method then checks if the two flags have been "taken". If so, another process is started that checks for a specific output, otherwise the test passes (i.e. the current system doesn't allow 1GB large pages)
>
> Damon Fenacci has updated the pull request incrementally with one additional commit since the last revision:
> 
>   JDK-8304954: merge ifs checking when to print warning

Basically good, some remarks inline.

src/hotspot/share/code/codeCache.cpp line 321:

> 319:                    "Reverting to smaller page size (" SIZE_FORMAT "%s).",
> 320:                    byte_size_in_exact_unit(LargePageSizeInBytes), exact_unit_for_byte_size(LargePageSizeInBytes),
> 321:                    byte_size_in_exact_unit(ps), exact_unit_for_byte_size(ps));

You could use EXACTFMT and EXACTFMTARGS to shorten this, see globalDefinitions.hpp

src/hotspot/share/code/codeCache.cpp line 324:

> 322:       log_warning(codecache)("%s", msg);
> 323:       warning("%s", msg);
> 324:   }

This "warn if page size is not as expected" topic is more complex.

For a start, the text is misleading. You don't reserve anything here. "reserve" has a very clear meaning. Here, you only calculated a page size that fits CodeCacheSize geometry, and then found it is smaller than what the user probably wanted, and you want to warn the user about this. Makes sense.

We also have the following cases:

- code cache could, in theory, be satisfied with large pages, but its size is not aligned with large page size. E.g. 2MB pages and CodeCacheSize=101m, would result in code cache using 4KB pages.
- The user may not have LPSIB specified, so it's 0, but he specified +UseLargePages. Then, he may not have a specific page size in mind but may still want to know if the page size of the code cache is not a large page. 

I think if you warn here when we divert from planned page size for geometry reasons, you should warn for these cases too. An acceptable minimum would be "ps < os::default_large_page_size()"

test/hotspot/jtreg/compiler/codecache/CheckLargePages.java line 27:

> 25:  * @test
> 26:  * @bug 8304954
> 27:  * @summary Test checks that if using large pages and code cache gets above the limit it tries to revert to smaller pages instead of failing

Proposal: "Code cache reservation should gracefully downgrade to using smaller pages if the code cache size is too small to host the requested page size."

test/hotspot/jtreg/compiler/codecache/CheckLargePages.java line 28:

> 26:  * @bug 8304954
> 27:  * @summary Test checks that if using large pages and code cache gets above the limit it tries to revert to smaller pages instead of failing
> 28:  * @requires vm.gc != "Z"

Why not Z?

test/hotspot/jtreg/compiler/codecache/CheckLargePages.java line 54:

> 52:                     "-XX:ReservedCodeCacheSize=2g",
> 53:                     "-XX:LargePageSizeInBytes=1g",
> 54:                     "-Xlog:pagesize*=debug",

If all you scan for is the "Failed to reserve" (please change the text :-), then you should *not* specify Xlog, since its a warning, and we expect to see this warning unconditionally (UL: everything above "log_info" is printed unconditionally if no Log options are present).

test/hotspot/jtreg/compiler/codecache/CheckLargePages.java line 61:

> 59:         } else {
> 60:             System.out.println("1GB large pages not supported: UseLargePages=" + largePages +
> 61:                     (largePages ? ", largePageSize=" + largePageSize : "") + ". Skipping");

It would be nice to have an actual test for the downgrade. Eg if system supports 1g and 2m and 4k, check that we downgrade to 2m.

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

Changes requested by stuefe (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/14903#pullrequestreview-1543381861
PR Review Comment: https://git.openjdk.org/jdk/pull/14903#discussion_r1272221249
PR Review Comment: https://git.openjdk.org/jdk/pull/14903#discussion_r1272196328
PR Review Comment: https://git.openjdk.org/jdk/pull/14903#discussion_r1272217249
PR Review Comment: https://git.openjdk.org/jdk/pull/14903#discussion_r1272197970
PR Review Comment: https://git.openjdk.org/jdk/pull/14903#discussion_r1272213945
PR Review Comment: https://git.openjdk.org/jdk/pull/14903#discussion_r1272219700