RFR: 8331539: [REDO] NMT: add/make a mandatory MEMFLAGS argument to family of os::reserve/commit/uncommit memory API [v2]

[Thomas Stuefe]

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

Good analysis, @afshin-zafari. 

However, I keep thinking we are stop-gap fixing holes in a badly designed system.

The problem is two-fold:

1) NMT assumes reserves and commits to be different layers and, e.g., for committed regions to be fully contained in a reserved region. This is wrong and does not reflect the realities of mmap. We can overlay and overlap any reservation/committing/uncommitting/releasing in any way we want.

The right way to track virtual memory regions is what we do now in https://github.com/openjdk/jdk/pull/18289 with src/hotspot/share/nmt/vmatree.hpp. See [1] for the (simple) theory behind it. Not only would this be a lot faster and simpler, but it would also be less error-prone since it does not assume any kind of layering between reservations and committing memory. With the VMATree, releasing a whole region would remove all containing regions automatically.

With Johan's PR we now will do this for ZGC memory file allocations. However, we should also use the same technique to track VirtualMemory in NMT. Then, errors like this will disappear.

2) Another problem is that ReservedSpace assumes ownership of the underlying memory. On Windows, we cannot split regions allocated with VirtualAlloc. So ReservedRegions are assigned a MEMFLAG at construction, and we can never split up the region because Windows. Therefore, to have a contiguous region with different regions and different flags in NMT, NMT forces us to allocate them in two steps, side by side, with all that can go wrong. This is suboptimal. NMT is a simple tracker; it should not dictate how we allocate memory but be able to accommodate any way we want.

I don't have a good solution in my head for (2). It is also the less urgent problem, I think.


[1] https://gist.github.com/tstuefe/d9682b7f11b3375da27faa100f45e621

src/hotspot/share/cds/metaspaceShared.cpp line 1169:

> 1167:           // Set up compressed Klass pointer encoding: the encoding range must
> 1168:           //  cover both archive and class space.
> 1169:           assert(class_space_rs.is_reserved(), "Memory region should be reserved.");

Not necessary. Checked in reserve_address_space_for_archives, and in Metaspace::initialize_class_space

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

PR Review: https://git.openjdk.org/jdk/pull/19343#pullrequestreview-2076383403
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1613180561