RFR: 8253638: Cleanup os::reserve_memory and remove MAP_FIXED

[Thomas Stuefe]

On Fri, 25 Sep 2020 11:24:17 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

> There's a code path through os::reserve_memory that uses MAP_FIXED. This path is used if a requested address is given.
> It's very dangerous to use MAP_FIXED, since it causes pre-existing mappings to be replaced. Alternative, move safe,
> functions exists; named os_attempt_reserve_memory_at.  It's not only dangerous to use that API, the Mac AARCH64 port
> also can't use it in conjunction with its MAP_JIT.
> I started to split os::reserve_memory into two functions; one that doesn't care about were the memory is mapped
> (os::reserve_memory), and another that uses the dangerous MAP_FIXED flag (os::reserve_memory_at). However, I noticed
> that it's only windows code that actually use os::reserve_memory with a requested address. All other usages have been
> cleaned out and replaced with os::attempt_reserve_memory. And on windows os::attempt_reserve_memory uses
> os::reserve_memory with comments that it's safe because it only "attempts" to place the mapping.  So, this patch: 1)
> Removes the forcefully reserving of memory and its MAP_FIXED usages! 2) Changes the windows code to call the
> attempt_reserve_memory_at version, that it actually did anyway. 3) Flips the call order between pd_reserve_memory and
> pd_attempt_reserve_memory_at, on windows. 4) There's also some unification and split done to handle MEMFLAGS and fds.
> This part needs followup cleanups (IMHO). 5) The AIX implementation already refused to use MAP_FIXED, and now it
> doesn't need special code to handle that. 6) Small cleanups around anon_mmap and anon_mmap_aligned

Hi Stefan,

very nice cleanup! I had a similar change in work for some time now, but your work made that obsolete. See also
https://bugs.openjdk.java.net/browse/JDK-8079351, which now can be closed as duplicate of your bug.

Most of my remarks are idle nits; I leave it up to you what you take from them.

src/hotspot/os/aix/os_aix.cpp line 2362:

> 2360:   // just ignore the request address (release) or assert(debug).
> 2361:   assert0(requested_addr == NULL);
> 2362:

Funnily, the comment is by me, from 15ish years ago. Back then I did not feel confident enough to change the other
platforms, and we could nothing contribute upstream anyway since there was no OpenJDK yet.

AIX changes look all fine (and trivial). See remark below about alignment_hint.

src/hotspot/os/bsd/os_bsd.cpp line 2026:

> 2024: static char* anon_mmap(char* requested_addr, size_t bytes) {
> 2025:   int flags = MAP_PRIVATE | MAP_NORESERVE | MAP_ANONYMOUS;
> 2026:

Could you add a short oneliner comment here (and in front of the other mmap calls) that MAP_FIXED is explicitly left
out?

src/hotspot/os/linux/os_linux.cpp line 3670:

> 3668: // Returns address of memory or NULL. If req_addr was not NULL, will only return
> 3669: //  req_addr or NULL.
> 3670: static char* anon_mmap_aligned(char* req_addr, size_t bytes, size_t alignment) {

I like the corrected order of parameters.

src/hotspot/os/linux/os_linux.cpp line 3653:

> 3651: static char* anon_mmap(char* requested_addr, size_t bytes) {
> 3652:   int flags = MAP_PRIVATE | MAP_NORESERVE | MAP_ANONYMOUS;
> 3653:

On Linux, we have MAP_FIXED_NOREPLACE now. Just a thought. It would give us the behavior closest to what we want: use
exactly the provided address but if there is a pre-existing mapping already do not clobber it. That avoids an extra
turnaround where the caller has to unmap the mapping because its in the wrong place.

The question is whether that flag is available on all kernels we want to run on.

Maybe something for a future patch.

src/hotspot/os/linux/os_linux.cpp line 3667:

> 3665: //  - req_addr can be NULL. If not NULL, it must be a multiple of alignment.
> 3666: //  - alignment sets the alignment at which memory shall be allocated.
> 3667: //     It must be a multiple of allocation granularity.

(Nothing to do with your patch) but this comment is technically wrong; it should be a multiple of page size. And we
probably should assert this in anon_mmap_aligned().

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

> 3065:   release_memory(base, size);
> 3066:   attempt_reserve_memory_at(split, base);
> 3067:   attempt_reserve_memory_at(size - split, split_address);

(nothing to do with your patch) but an assert would be good here to make sure the re-reserving worked. AFAICS its only
used by CDS and they already assert. But still, would be cleaner.

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

> 315:   // Reserves virtual memory.
> 316:   // alignment_hint - currently only used by AIX
> 317:   static char*  reserve_memory(size_t bytes, size_t alignment_hint = 0, MEMFLAGS flags = mtOther);

I believe we can actually do away with the alignment hint for os::reserve_memory altogether. Which would be good
because it is strange and not well documented.

I believe the original intent was to let the OS "do its best" but no OS does; all AIX does (and only in mmap mode) is
the same as upper layers do, chopping excess memory at front and back away manually.

Moreover, that chopping does only work well when using mmap. When using win32 VirtualAlloc or sysv shmat, we cannot
unmap part of a mapping.

On AIX, I basically just implemented it to "do my best", because I wanted to fill in the os::xx API as best as
possible; but it is actually not needed, we don't care (there is some alignment magic going on in 64k paged mode with
mmap, but that is a different issue).

If I see it right, there are only two cases where caller request alignment from os::reserve_memory:

- at several places (eg ZMarkStackSpace) I see an alignment used of os::vm_allocation_granularity, which is unnecessary
  since that is the granularity the OS will reserve at anyway. Also, it only has any meaning on windows.
- I see os::reserve_memory_aligned(), used when creating a ReservedSpace with an alignment wish - which is a real thing,
  e.g. in metaspace - using the alignment parameter as a wish to os::reserve_memory, but then proceeds to do the
  alignment-chopping-dance anyway so I guess it is not needed and was just a hopeful optimization.

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

> 320:   // if file_desc != -1, also attaches the memory to the file.
> 321:   static char*  reserve_memory_with_fd(size_t bytes, size_t alignment_hint, int file_desc);
> 322:

I like that you split those two apart. The fd parameter in reserve_memory really bugged me. Should we do this also for
attempt_reserve_memory_at?

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

PR: https://git.openjdk.java.net/jdk/pull/357