RFR: 8310743: assert(reserved_rgn != nullptr) failed: Add committed region, No reserved region found

[Axel Boldt-Christmas]

On Fri, 30 Jun 2023 13:45:23 GMT, Albert Mingkun Yang <ayang at openjdk.org> wrote:

>> JDK-8309675 tried to solve the issue of NMT reporting to much committed memory by faking the virtual addresses registered when committing physical memory. This mapping was simply the `virtual address  = physical offset in backing file + heap base`. This works when ZGC gets a contiguous heap which starts at the heap base. However fails if it does not start at the heap base or is discontiguous.
>> 
>> This proposal fixes this by keeping track of all the reserved virtual address ranges and by creating a mapping from the physical offsets to the actual reserved virtual address ranges. If the physical offsets committed maps to multiple reserved ranges, it is processed as multiple commits.
>> 
>> The develop flag `ZForceDiscontiguousHeapReservations` is introduced to test the discontiguous heap feature and interactions.
>> 
>> Testing: 
>>  * Added test on all Oracle platforms
>>  * GHA
>>  * Running tier1-3
>
> src/hotspot/share/gc/z/zNMT.cpp line 44:
> 
>> 42:     const size_t reservation_size = _reservations[index]._size;
>> 43:     if (*offset_in_reservation < reservation_size) {
>> 44:       break;
> 
> I wonder if one can return `index` here. Then, after the for-loop, one can write `fatal(...)`.

It was purposely written like this avoiding a `fatal`/`guarantee`/`ShouldNotReachHere`, the intent is to have bugs only be caught in debug. If we return `_num_reservations` as an index all that happens is that we do not register some commit with nmt. Which seems more favourable than crashing if some bug were to slip through into production.

> src/hotspot/share/gc/z/zNMT.cpp line 79:
> 
>> 77:         Tracker tracker(Tracker::uncommit);
>> 78:         tracker.record((address)sub_range_addr, sub_range_size);
>> 79:       }
> 
> Sth like `record_virtual_memory_uncommit` would make this more symmetric. (Better be done in a followup PR though.)

I agree. Purposely kept this minimal as it needs to be backported to 21.

> src/hotspot/share/gc/z/zNMT.cpp line 91:
> 
>> 89:   }
>> 90: 
>> 91:   assert(left_to_process == 0, "everything was not commited");
> 
> When will this be reachable? I'd expect the if-check inside the for-loop to handle exit-condition.

If we run out of reservations. That is the virtual memory reservations range is less than the physical memory offset we are committing. Such a thing should not be reachable and is a bug. But if it does happen it should be caught here.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/14730#discussion_r1250291378
PR Review Comment: https://git.openjdk.org/jdk/pull/14730#discussion_r1250291779
PR Review Comment: https://git.openjdk.org/jdk/pull/14730#discussion_r1250291610