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

[Thomas Stuefe]

On Mon, 7 Dec 2020 18:04:52 GMT, Ioi Lam <iklam at openjdk.org> wrote:

>> Yumin Qi has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 35 additional commits since the last revision:
>> 
>>  - Merge branch 'master' into jdk-8255917
>>  - Add total_space_rs, total reserved space to release_reserved_spaces and reserve_address_space_for_archives, made changes to check failed output on test.
>>  - 8253762: JFR: getField(String) should be able to access subfields
>>    
>>    Reviewed-by: mgronlun
>>  - 8257670: sun/security/ssl/SSLSocketImpl/SSLSocketLeak.java reports leaks
>>    
>>    Reviewed-by: jnimeh
>>  - 8257796: [TESTBUG] TestUseSHA512IntrinsicsOptionOnSupportedCPU.java fails on x86_32
>>    
>>    Reviewed-by: kvn
>>  - 8257211: C2: Enable call devirtualization during post-parse phase
>>    
>>    Reviewed-by: kvn, neliasso, thartmann
>>  - 8257572: Deprecate the archaic signal-chaining interfaces: sigset and signal
>>    
>>    Reviewed-by: ihse, alanb, dcubed, erikj
>>  - 8257718: LogCompilation: late_inline doesnt work right for JDK 8 logs
>>    
>>    Reviewed-by: redestad, kvn
>>  - 8257799: Update JLS cross-references in java.compiler
>>    
>>    Reviewed-by: jjg
>>  - 8254939: macOS: unused function 'replicate4_imm'
>>    
>>    Reviewed-by: redestad, thartmann
>>  - ... and 25 more: https://git.openjdk.java.net/jdk/compare/8bc10d13...7b93584f
>
> 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

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

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