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 11:48:37 GMT, Afshin Zafari <azafari at openjdk.org> wrote:

>> 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.
>
> Some facts: 
> - `MemTracker::record_virtual_memory_release()` has no `ThreadCritical` internally and therefore should be called inside a critical section.
> - When `os::release_memory()` returns, the `ThreadCritical` that is created there is destroyed and a new one should be created again here.
> - Releasing a sub-region that flagged for CDS and is contained in a larger CDS region is ignored at `MemTracker::record_virtual_memory_release()`. It is a valid case due to the way that CDS reserves and/or releases regions.
> - This exceptional case is notified to `MemTracker` by passing `true` as `extra_memory`.
> - Inside `MemTracker`, the `extra_memory == true` is used in the places where the exceptional case should/would be addressed.

There are a couple of "therefore", "should", and "is a valid case", above that makes it sound like this is the only way to implement things. My point is that I don't think it is, and I would like to see a step back where we try to think about a way to write this without adding these extra layers of code.

With that said, Thomas is thinking about ways to change how we keep track of the NMT flags, so I hope that if we make those changes we could skip having to add the code here.

>> The flags sent to the NMT subsystem is correct, but the flags recorded in the ReservedSpaces will be wrong, AFAIKT. You can probably verify that by adding asserts.
>
> If your comment refers only to these lines of code, they are already verified. Since, inside the split function, the sub-regions get the new flags and all the reserved and committed amounts are moved from the large region to the new ones. So, the accounting of memory is correct.
> 
> FWIW, if we trace down the call at line 1346 of `total_space_rs = Metaspace::reserve_address_space_for_compressed_classes(total_range_size, false /* optimize_for_zero_base */);` the region may get different flags of `mtClass` or `mtMetaspace` based on the checked criteria down there.
>  
> If you comment on all such cases, then I will double check for them and add assertion for.

My point is that the `archive_space_rs` and `class_space_rs` can get the wrong flags assigned to them. The split functions don't change them. Right?

I would like to see the code run through our testing with these checks:

  assert(archive_space_rs.nmt_flag() == mtClassShared, "Sanity");
  assert(class_space_rs.nmt_flag() == mtClass, "Sanity");

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

PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1616042193
PR Review Comment: https://git.openjdk.org/jdk/pull/19343#discussion_r1616028639