RFR: JDK-8312018: Improve reservation of class space and CDS [v5]

[Ioi Lam]

On Wed, 16 Aug 2023 06:33:48 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> This PR rewrites memory reservation of class space and CDS to have ASLR and a much better chance of low-address placement.
>> 
>> ------
>> 
>> Motivation:
>> 
>> (I was advised to keep PR text short lest I spam the mailing lists with Skara generated mails. So, for motivation, see JBS issue)
>> 
>> -------
>> 
>> The patch introduces a new API to reserve memory within an address range at a randomized location, while trying to be smart about it. The API is generic, and future planned uses of this API could include replacing the zero-based heap allocation and the zero-based reservation of Shenandoah Collection Sets, thereby allowing us to consolidate coding.
>> 
>> This PR complements @iklam 's current work that rewrites archive heap initialization at runtime. Once his work is in, we will be able to recalculate narrow Klass IDs for objects loaded from the archive, and that will allow us to reap the benefits of this patch for the CDS runtime case too.
>> 
>> -------
>> 
>> Noteworthy functional changes:
>> 
>> - class space is now likely to be reserved at a random location in low address ranges; this now includes ranges below 2 GB, which had been excluded before due to the use of HeapBaseMinAddress as minimal attach point.
>>      -  Note that this makes no problem with sbrk() - the only platform still having this problem is AIX, and we solved it differently there, see comment in AIX implementation of `os::vm_min_address()`
>> - I removed the PPC/AARCH64 specific coding that attempted to map at 4G/32G aligned addresses. That section has a complex history - it was originally introduced to deal with AARCH64 immediate-loading shortcomings, but PPC piggybacked on it for its perceived ability to allocate for zero-based, which got subsequently lost, so its broken now (see https://bugs.openjdk.org/browse/JDK-8313669). The new code is a better replacement for this coding.
>> 
>> -------
>> 
>> Example (linux amd64):
>> 
>> We start the JVM with a 30GB heap. 
>> 
>> In the stock JVM, the JVM will place the heap in the lower address ranges starting at 2G (0x8000_0000). But then it is unable to place the class space in lower regions too, so it placed it at 32 GB (0x8_0000_0000), and we don't have zero-based encoding (Narrow klass base: 0x0000000800000000). This scenario repeats for every iteration, so we will always use these two addresses (no ASLR): 
>> 
>> 
>> thomas at starfish $ ./images/jdk/bin/java -Xshare:off -Xmx30g -Xlog:gc+heap+exit -Xlog:gc+metaspace -version
>> [0.019s][info][gc,metaspace] ...
>
> Thomas Stuefe has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - Roman: better comment in metaspaceShared
>  - Changes to os::vm_min_addr

src/hotspot/share/memory/metaspace.cpp line 636:

> 634:     log_debug(metaspace, map)("Trying between " UINT64_FORMAT_X " and " UINT64_FORMAT_X
> 635:                               " with " SIZE_FORMAT_X " alignment", min, max, alignment);
> 636:     result = os::attempt_reserve_memory_between((char*)min, (char*)max, size, alignment, randomize);

If `try_in_low_address_ranges` is true, shouldn't `min` be no less than `zerobased_max`. Otherwise you may be searching some ranges twice.

src/hotspot/share/memory/metaspace.cpp line 649:

> 647:   if (result != nullptr) {
> 648:     rs = ReservedSpace::space_for_range(result, size, Metaspace::reserve_alignment(),
> 649:                                                       os::vm_page_size(), false, false);

Should we check that result is aligned by both `Metaspace::reserve_alignment()` and `os::vm_page_size()`?

Not directly part of this change, but I think the existing function `ReservedSpace::space_for_range()` should add 


assert(is_aligned(alignment, page_size), ....);

src/hotspot/share/runtime/os.cpp line 1818:

> 1816: 
> 1817:   // Number of mmap attemts we will undertake.
> 1818:   constexpr unsigned max_attempts = 32;

Is this sufficient?

src/hotspot/share/runtime/os.cpp line 1896:

> 1894:     for (unsigned i = 0; i < num_attempts; i++) {
> 1895:       const unsigned deviation = stepsize > 1 ? (frand.next() % stepsize) : 0;
> 1896:       points[i] = (i * stepsize) + deviation;

Can the last point be out of range? Maybe add an assert of something like:


range_calculated with points[i] must be within [min ... max]

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

PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1300888438
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1300890672
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1300905871
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1300903642