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

[Afshin Zafari]

On Thu, 11 Apr 2024 16:15:04 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   fixed missing change.
>
> src/hotspot/os/bsd/gc/x/xPhysicalMemoryBacking_bsd.cpp line 81:
> 
>> 79: 
>> 80:   // Reserve address space for backing memory
>> 81:   _base = (uintptr_t)os::reserve_memory(max_capacity, false, mtJavaHeap);
> 
> I think this should be using !MemExec and not a raw 'false' argument.
> 
> With that said, I really think it would be best if we actually split os::reserve_memory into two distinct functions:
> 1) os::reserve_memory(max_capacity, mtJavaHeap) // Non-executable memory
> 2) os::reserve_executable_memory(max_capacity, mtCode) // Executable
> 
> Maybe we could think about that in separate RFE.

`false/true` constants are not used in executable args.
separate reserve_memory functions can be left for another RFE.

> src/hotspot/os/bsd/gc/z/zPhysicalMemoryBacking_bsd.cpp line 82:
> 
>> 80: 
>> 81:   // Reserve address space for backing memory
>> 82:   _base = (uintptr_t)os::reserve_memory(max_capacity, false, mtJavaHeap);
> 
> Use !MemExec - this goes for all other places in the patch that fills in the `exec` parameter.

Fixed.

> src/hotspot/os/windows/os_windows.cpp line 3137:
> 
>> 3135:   // If reservation failed, return null
>> 3136:   if (p_buf == nullptr) return nullptr;
>> 3137:   MemTracker::record_virtual_memory_reserve((address)p_buf, size_of_reserve, CALLER_PC, mtNone);
> 
> Why is this (and the other places in this file) using `mtNone`? Shouldn't it at least be using `mtInternal`?

Fixed.

> src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp line 1414:
> 
>> 1412:   assert(SafepointSynchronize::is_at_safepoint(), "safe iteration is only available during safepoints");
>> 1413: 
>> 1414:   if (!_aux_bitmap_region_special && !os::commit_memory((char*)_aux_bitmap_region.start(), _aux_bitmap_region.byte_size(), false, mtJavaHeap)) {
> 
> Why is this `mtJavaHeap` and not `mtGC`?

Fixed.

> src/hotspot/share/gc/z/zMarkStackAllocator.cpp line 107:
> 
>> 105: 
>> 106:     const uintptr_t shrink_start = _end - shrink_size;
>> 107:     os::uncommit_memory((char*)shrink_start, shrink_size, mtGC, false /* executable */);
> 
> `uncommit_memory` places the order of executable and flags differently to what we have for `commmit_memory_or_exit`. We might want to consider doing something about the order here.

Fixed.

> src/hotspot/share/memory/virtualspace.cpp line 71:
> 
>> 69: ReservedSpace::ReservedSpace(size_t size,
>> 70:                              size_t alignment,
>> 71:                              size_t page_size, MEMFLAGS flag,
> 
> I think this function was written to have one argument per line. You should probably keep the style.
> 
> I'm also unsure why this param is put as the next to last param instead of the last, as we do in many other places.

Put in separate line.
the last param is optional, and flag is to be mandatory.

> src/hotspot/share/memory/virtualspace.cpp line 693:
> 
>> 691:   _special                = false;
>> 692:   _executable             = false;
>> 693:   _nmt_flag                = mtNone;
> 
> Weird indentation.

Fixed.

> src/hotspot/share/memory/virtualspace.hpp line 45:
> 
>> 43:   bool   _special;
>> 44:   int    _fd_for_heap;
>> 45:   MEMFLAGS _nmt_flag;
> 
> Indentation is now off.

Fixed.

> src/hotspot/share/memory/virtualspace.hpp line 72:
> 
>> 70: 
>> 71:   inline MEMFLAGS nmt_flag() { return _nmt_flag; }
>> 72:   inline void set_nmt_flag(MEMFLAGS flag) { _nmt_flag = flag; }
> 
> No need for the inline specifier here.

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561408632
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561409363
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561410373
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561412485
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561413236
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561418594
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561421020
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561423257
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1561424255