RFR: 8234930: Use MAP_JIT when allocating pages for code cache on macOS [v8]

[Thomas Stuefe]

On Fri, 11 Dec 2020 13:25:14 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> Please review an updated RFR from https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/041463.html
>> 
>> On macOS, MAP_JIT cannot be used with MAP_FIXED[1]. So pd_reserve_memory have to provide MAP_JIT for mmap(NULL, PROT_NONE), the function was made aware of exec permissions.
>> 
>> For executable and data regions, pd_commit_memory only unlocks the memory with mprotect, this should make no difference compared with old code.
>> 
>> For data regions, pd_uncommit_memory still uses a new overlapping anonymous mmap which returns pages to the OS and immediately reflects this in diagnostic tools like ps.  For executable regions it would require MAP_FIXED|MAP_JIT, so instead madvise(MADV_FREE)+mprotect(PROT_NONE) are used. They should also allow OS to reclaim pages, but apparently this does not happen immediately. In practice, it should not be a problem for executable regions, as codecache does not shrink (if I haven't missed anything, by the implementation and in principle).
>> 
>> Tested: 
>> * local tier1
>> * jdk-submit
>> * codesign[2] with hardened runtime and allow-jit but without 
>> allow-unsigned-executable-memory entitlements[3] produce a working bundle.
>> 
>> (adding GC group as suggested by @dholmes-ora)
>> 
>> 
>> [1] https://github.com/apple/darwin-xnu/blob/master/bsd/kern/kern_mman.c#L227
>> [2]
>>  
>>   codesign \
>>     --sign - \
>>     --options runtime \
>>     --entitlements ents.plist \
>>     --timestamp \
>>     $J/bin/* $J/lib/server/*.dylib $J/lib/*.dylib
>> [3]
>>   <?xml version="1.0" encoding="UTF-8"?>
>>   <!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
>>   <plist version="1.0">
>>     <dict>
>>       <key>com.apple.security.cs.allow-jit</key>
>>       <true/>
>>       <key>com.apple.security.cs.disable-library-validation</key>
>>       <true/>
>>       <key>com.apple.security.cs.allow-dyld-environment-variables</key>
>>       <true/>
>>     </dict>
>>   </plist>
>
> Anton Kozlov has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update style

Hi Anton,

1) can you please make the executable parameter on os::uncommit() default false? Lets minimize the impact for the vast majority of callers which do not need protected memory. The only caller needing this is ReservedSpace. Would also be more in sync with the default false for executable on os::reserve(). 

2) Personal nit, I really find this `ExecMem` jarring. We don't do this (pass named aliases for boolean flags) for any other arguments AFAICS. The usual way to emphasize arg names is with comments:
bool result = os::commit_memory(base, size, /*exec*/ false);
If you still prefer to use it, could you leave at least those places unchanged which are unaffected by your patch?

3) There is some code explicitly dealing with the supposed inability of attempt_reserve_memory_at() using MAP_JIT. I don't understand. I thought the problem was thjat MAP_JIT and MAP_FIXED don't mix. But attempt_reserve_memory_at() does explicitely not use MAP_FIXED, it just attempts to map at a non-null wish address. Does that also not work with MAP_JIT?

4) For my taste there are too many unrelated changes, especially in ReservedSpace. Making all those file scope static helpers members of ReservedSpace causes a lot of diffs. Makes sense cleanup-wise but will make backporting your patch more difficult later (I expect this will be a strong candidate for backporting). Please tone down the patch a bit. I pointed some parts out directly below. Beyond those, I leave it up to you how far you minimize the patch.

Thanks, Thomas

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

> 1688:   if (::mprotect(addr, size, prot) == 0) {
> 1689:     return true;
> 1690:   }

You need to handle mprotect failure here too. Probably just by returning false. There is no point in doing the mmap below as fallback. The same applies for the OpenBSD path too.

mprotect may, at least on Linux, fail if the new mapping introduced by changing the protection would bring the process above the system limit for numbrer of mappings. I strongly believe there must be a similar error scenario on Mac. At least on BSD there is (https://man.openbsd.org/mprotect.2), see ENOMEM.

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

> 324:   // Does not overwrite existing mappings.
> 325:   // It's intentionally cannot reserve executable mapping, as some platforms does not allow that
> 326:   // (e.g. macOS with proper MAP_JIT use).

This is a note to a future implementor, not to the user. I would move this out of the header to the posix implementation.

Also, see my question above, why would this not work?

src/hotspot/share/memory/virtualspace.cpp line 88:

> 86:   }
> 87:   assert(!_special, "should not call this");
> 88:   assert(!_executable, "unsupported");

Why is this unsupported? Could I not use MAP_JIT without MAP_FIXED but with a non-null attach address?

src/hotspot/share/memory/virtualspace.cpp line 311:

> 309:     _base -= _noaccess_prefix;
> 310:     _size += _noaccess_prefix;
> 311: 

Since you revert the steps taken at establish_noaccess_prefix, I'd move the _noaccess_prefix=0 up to here. For aestethic reasons mainly :) alternatively, I'd use temp variables like the code did before.

src/hotspot/share/memory/virtualspace.hpp line 61:

> 59:   char* reserve_memory(size_t size);
> 60:   char* reserve_memory_aligned(size_t size, size_t alignment);
> 61:   void  release_memory(char* base, size_t size);

I liked the old names (..._map_or_reserve_....) better. Can you please rename them back? Their whole point is multiplexing between anonymous and mmaped reservation calls. Also, in their current form they read identical to the os::... functions which is really confusing.
For release I propose `release_mapped_or_reserved_memory'. Its a mouthful but it clearly states what it does.

src/hotspot/share/memory/virtualspace.cpp line 399:

> 397:              p2i(base), alignment);
> 398:     } else {
> 399:       _special = false;

special->_special: This change has no connection to your patch. Can you leave this out please? I have nothing against cleanups but please in a separate RFE. Makes the patch clearer and easier to backport later.

src/hotspot/share/memory/virtualspace.cpp line 194:

> 192:              p2i(base), alignment);
> 193:     } else {
> 194:       _special = false;

Changes (special->_special) are Cleanup, please do them in a separate RFE if needed.

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

Changes requested by stuefe (Reviewer).

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