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

Fri Sep 25 13:24:55 UTC 2020

On Fri, 25 Sep 2020 12:13:27 GMT, Thomas Stuefe <stuefe 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
>
> 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.

Thanks.

> 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.

Thanks. I'm going to change the top-level functions as a separate patch.

> 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.

Yes. I tested this on ZGC. We have to provide the MAP_FIXED_NOREPLACE define, but otherwise it should work.

> 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().

I saw that as well, and I know there are more comments like that in other parts. Maybe we could take a clean-up round
and fix these.

> 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.

IMHO, the above code is a bug. The JVM could have been launched through JNI and we might have other threads
reserving/committing memory. I'm inclined to file a bug about that.

> 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.

I had the same thought, but found some paths were it wasn't obvious to me. That's why I left the comment above, hoping
to get some reaction out from it. Could we look into this as a separate patch?

> 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?

Yes, I was thinking about that, but wanted to slice this into easily reviewed parts.

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

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