RFR: 8255917: runtime/cds/SharedBaseAddress.java failed "assert(reserved_rgn != 0LL) failed: No reserved region" [v4]

[Ioi Lam]

On Tue, 8 Dec 2020 10:07:21 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> src/hotspot/share/memory/metaspaceShared.cpp line 1655:
>> 
>>> 1653:     archive_space_rs = total_rs.first_part(ccs_begin_offset,
>>> 1654:                                            (size_t)os::vm_allocation_granularity(),
>>> 1655:                                            /*split=*/false);
>> 
>> Since `/*split=*/false` is passed,  the region is no longer split using `os::split_reserved_memory()`. This means on Windows, we  cannot use `os::release_memory()` to free the individual regions of `archive_space_rs` and `class_space_rs`.
>> 
>> The Windows API of [VirtualFree](https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-virtualfree) says:
>> 
>>> Releases the specified region of pages, or placeholder [...], `dwSize` must be 0 (zero), and `lpAddress` must point to the base address returned by the `VirtualAlloc` function when the region is reserved. The function fails if either of these conditions is not met.
>> 
>> I suggest we do this:
>> 
>> - Add an extra `ReservedSpace& total_rs` parameter to `MetaspaceShared::reserve_address_space_for_archives()`. Return the `total_rs` when we go through this path of the code.
>> - Also pass `total_rs` to `MetaspaceShared::release_reserved_spaces`. If `total_rs.is_reserverd()` is true, release `total_rs` instead of the two smaller spaces.
>> 
>> To make sure this PR is correct, we should add something like the following in `os::release_memory()`, and check for this log in test/hotspot/jtreg/runtime/cds/SharedBaseAddress.java:
>> 
>> if (!res) {
>>   log_info(os)("os::release_memory(" PTR_FORMAT ", " SIZE_FORMAT ") failed", p2i(addr), bytes);
>> }
>> 
>> Perhaps, in a separate RFE, we should add an assert in `os::release_memory()`, or at least change the `log_info` to `log_warning`.
>
> Hi Ioi,
> 
> Where is the release you worry about? Since on Windows, since 8255978, os::release_memory() notices if the region the caller wants release does not correspond exactly to a memory mapping at the OS level, and will assert. Do you see that assert?
> 
> I try to understand:
> 
> if useBaseAddress==true, Yumin now creates two separate mappings, and can release them individually
> if useBaseAddress==false, there is one mapping as before, but we split now shallow. But we don't release it since we use file IO to read into it. If someone were to release one of those, we should see an assert on Windows.
> (I am a tiny bit unhappy about the increasing complexity of the patch, since it negates some of the work done to simplify it back in June.)
> 
> About tracing, since 8256864 we trace Virtualxxx calls, so the tracing is already there - we trace VirtualFree() errrors for "os=info". If you only care for windows, that tracing could suffice.
> 
> Thanks, Thomas

If `useBaseAddress==false`, after the total_space is successfully mapped, subsequent operations mail fail. For example,

- `mapinfo->map_regions()` may fail to commit the necessary memory for doing `os::read()`.
- `mapinfo->validate_shared_path_table()` may fail because the runtime classpath is not compatible

In these cases, we need to call MetaspaceShared::release_reserved_spaces().

> Do you see that assert?

Hmm, I think we should add a new test for this specifically: `java -XX:ArchiveRelocationMode=1 -cp mispatched.jar` to force the failure in `mapinfo->validate_shared_path_table()`.

>  If you only care for windows, that tracing could suffice.

Since we are changing the `split` parameter for all platforms, I think we should test for all platforms, not just windows.

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

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