RFR: 8325133: Missing MEMFLAGS parameter in parts of os API [v2]

[Gerard Ziemski]

On Thu, 1 Feb 2024 15:54:24 GMT, Johan Sjölen <jsjolen at openjdk.org> wrote:

>> Hi,
>> 
>> Some parts of the `os` API calls NMT functions without taking a `MEMFLAGS` as a parameter. This PR extends the API of these functions to change this. No callers are changed.
>
> Johan Sjölen has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Missing executable argument

Looks good.

I only have one feedback that frankly I myself am not 100% sure we need to address.

src/hotspot/os/windows/os_windows.cpp line 3333:

> 3331: // virtual space to get requested alignment, like posix-like os's.
> 3332: // Windows prevents multiple thread from remapping over each other so this loop is thread-safe.
> 3333: static char* map_or_reserve_memory_aligned(size_t size, size_t alignment, int file_desc, MEMFLAGS flag = mtNone) {

Wouldn't it be better NOT to implement a default value here?

If the point in this fix is to make sure that we are passing the NMT flags, then we should make sure that we review all the callers, and force them to define the flags explicitly.

src/hotspot/share/runtime/os.hpp line 445:

> 443:   // Attempts to reserve the virtual memory at [addr, addr + bytes).
> 444:   // Does not overwrite existing mappings.
> 445:   static char*  attempt_reserve_memory_at(char* addr, size_t bytes, bool executable = false, MEMFLAGS flag = mtNone);

Same here, consider not implementing default value to force the callers to explicitly choose the value?

src/hotspot/share/runtime/os.hpp line 500:

> 498:   static char* map_memory_to_file_aligned(size_t size, size_t alignment, int fd, MEMFLAGS flag = mtNone);
> 499:   static char* map_memory_to_file(char* base, size_t size, int fd);
> 500:   static char* attempt_map_memory_to_file_at(char* base, size_t size, int fd, MEMFLAGS flag = mtNone);

And here...

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

Marked as reviewed by gziemski (Committer).

PR Review: https://git.openjdk.org/jdk/pull/17672#pullrequestreview-1859792391
PR Review Comment: https://git.openjdk.org/jdk/pull/17672#discussion_r1476368425
PR Review Comment: https://git.openjdk.org/jdk/pull/17672#discussion_r1476384872
PR Review Comment: https://git.openjdk.org/jdk/pull/17672#discussion_r1476403843