RFR: JDK-8312018: Improve reservation of class space and CDS
Ioi Lam
iklam at openjdk.org
Fri Aug 11 04:41:28 UTC 2023
On Wed, 26 Jul 2023 11:34:18 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] Compressed class space mapped at: 0x0000000800000000-0x00000008400...
Some preliminary comments.
- I am not sure what "CDS" means in the PR title. It seems like CDS doesn't (yet) benefit from the improvements.
- I would suggest breaking this PR into smaller steps (see my comments inside `Metaspace::reserve_address_space_for_compressed_classes`)
src/hotspot/os/linux/os_linux.cpp line 4229:
> 4227: // good protection against NULL references while still leaving enough of the lower
> 4228: // 4G addressable.
> 4229: return (char*)(MAX2(os::vm_allocation_granularity(), 16 * M));
The code doesn't do what the comment says.
src/hotspot/share/cds/metaspaceShared.cpp line 1333:
> 1331: // Reserve anywhere, but mapping start address must be directly encodable as encoding base.
> 1332: const bool strict_base = true;
> 1333: total_space_rs = Metaspace::reserve_address_space_for_compressed_classes(total_range_size, true);
Did you want to pass strict_base as a parameter?
I found this API (and the name `strict_base`) kind of unobvious.
If I understand correctly, the reasoning is:
-----
The restriction by CDS (which may be addressed in [JDK-8312018](https://bugs.openjdk.org/browse/JDK-8312018) is implemented) is the archived heap objects require a non-zero base encoding for compressed klass pointer:
InstanceKlass* ik = base + (narrowKlass << shift);
But `base` can be any value. In practice it won't be zero, because the lowest narrowKlass is a very small integer, which would mean that this klass would need to be mapped at a very low address, which most OSes don't allow.
CDS would be perfectly OK if the CCS starts at a low, non-zero address. But the address space below 4G is somewhat precious and scarce. So if CDS cannot take advantage of it, we shouldn't even attempt to reserve below 4G.
-----
Instead of `strict_base`, maybe use `for_cds` instead? That way you can explain the reasoning in a single place inside `reserve_address_space_for_compressed_classes`
src/hotspot/share/memory/metaspace.cpp line 615:
> 613: // Failing zero-based allocation, or in strict_base mode, try to come up with
> 614: // an optimized start address that:
> 615: // - only has bits set in the third quadrant, to use a single 16-bit move
It's not clear what "bits set in the third quadrant" and "16-bit move" are.
Also, for removing the old AARCH64/PPC code, perhaps you should do this in a prerequisite PR (taking just this "if" block, plus `os::attempt_reserve_memory_between`). That way we can make sure there's no regression for AARCH64/PPC, before implementing this full PR.
src/hotspot/share/runtime/globals.hpp line 1422:
> 1420: "fails VM initialization (requires -Xshare=off.") \
> 1421: \
> 1422: develop(bool, RandomizeClassSpaceLocation, true, \
It's better to make this a diagnostic option for trouble-shooting product builds.
src/hotspot/share/runtime/os.cpp line 1790:
> 1788: // If randomize is true, the location will be randomized.
> 1789: char* os::attempt_reserve_memory_between(char* min, char* max, size_t bytes, size_t alignment, bool randomize) {
> 1790:
I would suggest breaking this function down into smaller functions for readability.
-------------
PR Review: https://git.openjdk.org/jdk/pull/15041#pullrequestreview-1572965226
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1290865958
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1290866478
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1290882890
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1290870231
PR Review Comment: https://git.openjdk.org/jdk/pull/15041#discussion_r1290871002
More information about the hotspot-runtime-dev
mailing list