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

[Thomas Stuefe]

On Fri, 25 Sep 2020 13:19:20 GMT, Stefan Karlsson <stefank at openjdk.org> wrote:

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

Yes. But there is no other way to do this at this level (if the intent is to split reserved memory and then be able to
delete each mapping separately).

This is used in ReservedSpace::first/last_part, and most users of that API do not care about independent unmapping, for
most of them this is just a way to split an address range. Only exception is CDS which uses it to reserve a large range
and then place both CDS and metaspace into them; that coding would have to be rewritten if we do away with
split_reserved_memory().

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

Of course.

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

Great!

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

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