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

[Afshin Zafari]

On Wed, 17 Apr 2024 12:57:43 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

>> Afshin Zafari has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   alignment in coding style changed.
>
> src/hotspot/share/runtime/os.cpp line 1817:
> 
>> 1815: 
>> 1816: char* os::reserve_memory(size_t bytes, bool executable, MEMFLAGS flags) {
>> 1817:   char* result = pd_reserve_memory(bytes, executable, flags);
> 
> Doesn't it look weird that we pass in flags here and then still call MemTracker::record_ below? I think this is an artifact from mixing if we put the NMT calls in shared or in platform dependent code. I understand that you need this for this patch, but I also think there needs to be some RFE to figure out if this can be reworked.

The flag is needed on Windows for this call hierarchy:

reserve_memory
  pd_reserve_memory
    pd_attempt_reserve_memory_at
      allocate_pages_individually(..., MEMFLAGS flag)

Other platforms ignore the flag.
Agreed on a new RFE for handling this.

> src/hotspot/share/runtime/os.cpp line 2187:
> 
>> 2185:                      MEMFLAGS flags,
>> 2186:                      bool read_only,
>> 2187:                      bool allow_exec) {
> 
> The function was written with multiple parameters per line here, and then you changed it so that some of the params where placed on individual lines. This should likely be reverted.

Fixed.

> src/hotspot/share/runtime/os.hpp line 233:
> 
>> 231:                            char *addr, size_t bytes,
>> 232:                            MEMFLAGS flag,
>> 233:                            bool read_only = false,
> 
> Mixes param layout style. (Plus earlier comment that the default values should probably be removed so that MEMFLAGS can be put last).

Fixed.

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

PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570302144
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570305926
PR Review Comment: https://git.openjdk.org/jdk/pull/18745#discussion_r1570306322