RFR: 8331539: [REDO] NMT: add/make a mandatory MEMFLAGS argument to family of os::reserve/commit/uncommit memory API [v2]
Stefan Karlsson
stefank at openjdk.org
Thu May 23 13:08:07 UTC 2024
On Wed, 22 May 2024 12:09:14 GMT, Afshin Zafari <azafari at openjdk.org> wrote:
>> This PR fixes the problems existed in the original PR (https://github.com/openjdk/jdk/pull/18745). There are two main fixes here:
>> 1- `ReservedSpace` class is changed so that the `_flag` member never changes after it is set in ctor. Since reserving memory regions may go thru a try and fail sequence of reserve-release pairs, changing the `_flag` member at failed releases would lead to incorrect flags in subsequent reserves.
>> Also, some assertion are added to the getters of a `ReservedSpace` to check if the region is successfully reserved.
>>
>> 2- In order to have adjacent regions with different flags, CDS reserves a (large) region `R` and then splits it into sub regions `R1` and `R2` (`R == <---R1---><--R2-->`). At release time, NMT tracks only `R` and ignores releasing `R1` and `R2`. This ignoring is problematic when a requested region `R` is size-aligned to `R1---R---R2` first and then the `R1` and `R2` are released (`chop_extra_memory` function is called for this). In this case, NMT ignores tracking `R1` and `R2` with false assumption that a containing `R` will be released. Therefore, `R1` and `R2` remain in the NMT reserved-regions-list and when a new reserve happens at that regions, NMT complains by raising an exception.
>>
>> Tests:
>> mach5 tiers 1-5, {linux-x64, macosx-aarch64, windows-x64, linux-aarch64 } x {debug, non-debug}
>
> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>
> fixed the missing parts of shenandoahHeap.cpp
I've left an initial set of comments.
src/hotspot/os/posix/os_posix.cpp line 386:
> 384: if (begin_offset > 0) {
> 385: if (os::release_memory(extra_base, begin_offset))
> 386: {
The `{` should be moved to the line above.
src/hotspot/os/posix/os_posix.cpp line 387:
> 385: if (os::release_memory(extra_base, begin_offset))
> 386: {
> 387: ThreadCritical tc;
In many of the functions we put the `ThreadCritical` inside the `MemTracker` after the `enabled()` check, but we don't do it here. Why is that? Shouldn't the `ThreadCritical` usage be hidden inside `MemTracker`?
src/hotspot/share/cds/metaspaceShared.cpp line 1088:
> 1086: #endif // ASSERT
> 1087:
> 1088: if (archive_space_rs.is_reserved()) {
We've already asserted that this should be true, so this if should not be needed.
src/hotspot/share/cds/metaspaceShared.cpp line 1092:
> 1090: p2i(archive_space_rs.base()), p2i(archive_space_rs.end()), archive_space_rs.size());
> 1091: }
> 1092: if (class_space_rs.is_reserved()) {
`class_space_rs.is_reserved()` is asserted if `if (Metaspace::using_class_space())` is taken. I think this could be changed to:
Suggestion:
if (Metaspace::using_class_space()) {
src/hotspot/share/cds/metaspaceShared.cpp line 1341:
> 1339: } else {
> 1340: if (use_archive_base_addr && base_address != nullptr) {
> 1341: total_space_rs = ReservedSpace(total_range_size, archive_space_alignment,
Can you explain why you changed this?
It's also interesting that after this change we only use `base_address_alignment` in asserts. I think this indicates that something should be cleaned up / fixed here.
src/hotspot/share/cds/metaspaceShared.cpp line 1370:
> 1368: ccs_begin_offset, mtClassShared, mtClass);
> 1369: }
> 1370: assert(archive_space_rs.is_reserved(), "Archive space is not reserved.");
Something is dubious about the code above:
archive_space_rs = total_space_rs.first_part(ccs_begin_offset,
(size_t)archive_space_alignment);
class_space_rs = total_space_rs.last_part(ccs_begin_offset);
MemTracker::record_virtual_memory_split_reserved(total_space_rs.base(), total_space_rs.size(),
ccs_begin_offset, mtClassShared, mtClass);
In one path `total_space_rs` gets initialized with `mtClass` and in another path it gets initialized with `mtClassShared`. This means that we always get the wrong flag in one of `archive_space_rs` and `class_space_rs`.
src/hotspot/share/memory/virtualspace.hpp line 63:
> 61: // it should not change after.
> 62: // * _alignment - Not to be changed after initialization
> 63: // * _executable - Not to be changed after initialization
I think this would be a good change to do in the future, but currently this isn't true. `clear_members` do clear these fields, so I think you should remove these two lines.
src/hotspot/share/memory/virtualspace.hpp line 76:
> 74:
> 75: MEMFLAGS nmt_flag() const { assert(is_reserved(), "Memory region is not reserved."); assert(_flag != mtNone, "Memory flag is not set."); return _flag; }
> 76:
Looking at this again, and realize that this function should probably be moved to the other accessors below.
src/hotspot/share/memory/virtualspace.hpp line 98:
> 96: bool special() const { assert(is_reserved(), "Memory region is not reserved."); return _special; }
> 97: bool executable() const { assert(is_reserved(), "Memory region is not reserved."); return _executable; }
> 98: size_t noaccess_prefix() const { assert(is_reserved(), "Memory region is not reserved."); return _noaccess_prefix; }
FWIW, this change comes from one of my debugging sessions. I think it is good to have these asserts, I just wish they could says something like `assert(is_initialized(), ...)` to more clearly convey why we are doing this check.
We are considering if there are ways to split ReservedSpace into two classes, one that handles reserving of memory and one that is a plain view of already reserved memory. If/when we do such a change we could consider updating these asserts to be more legible.
In the meantime, it would be nice to change the string to "Fields not initialized" (and get rid of the `.`).
src/hotspot/share/nmt/virtualMemoryTracker.cpp line 506:
> 504: return true;
> 505: assert(reserved_rgn->end() == rgn.end() || reserved_rgn->base() == rgn.base(), "extra memory should be at either end of the region.");
> 506: }
This seems like an extreme hack. I understand that this just follows the tradition of the rest of the hacks in this file, but can't this be better handled in the CDS layer above?
-------------
Changes requested by stefank (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19343#pullrequestreview-2073757670
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611620811
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611624784
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611566535
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611567731
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611582321
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611608926
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611629459
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611630735
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611641554
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1611657743
More information about the shenandoah-dev
mailing list