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

[Stefan Karlsson]

On Fri, 24 May 2024 08:57:57 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> 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`?
>
> I have already tried to move `ThreadCritical` into the `MemTracker` (in another PR), but it failed. AFAIR, the unmapping/releasing the memory should be in critical section too. The current implementation follows this order: 1) create critical section 2) unmap/release 3) if successful, call MemTracker. The step 2) should be in critical section.

Hmm. os::release_memory also calls `record_virtual_memory_release`, and then this code calls it again with a second ThreadCritical, but then it is called again with `extra_memory`. I still find this addition of `extra_memory` highly dubious.

>> 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?
>
> TBH, I don't like it too. Unfortunately, chopping extra memory is done at `os:xxx` layer and reporting the case back to CDS would need to pass all the chopping info up to CDS. In addition, it is valid in CDS that a region is partitioned into sub regions and releasing sub regions can be silently and correctly ignored.

I'd like to take an extra look at that before this PR gets integrated.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1613259695
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1613262335