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

[Thomas Stuefe]

On Tue, 15 Dec 2020 14:53:11 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 pd_commit_memory on bsd

> Hi Thomas,
> 
> Thank you for review!
> 
> > 1. can you please make the executable parameter on os::uncommit() default false?
> 
> Ok, fixed.

Thanks!

> 
> > 1. Personal nit, I really find this `ExecMem` jarring.
> >    If you still prefer to use it, could you leave at least those places unchanged which are unaffected by your patch?
> 
> The only two places where I had to replace false with !ExecMem are https://github.com/openjdk/jdk/pull/294/files#diff-80d6a105c4da7337cbc8c4602c8a1582c6a1beb771797e1a84928bd864afe563R105
> 
> It would be really inconsistent to maintain them as
> 
> ```
> char* base = os::reserve_memory(size, !ExecMem, mtThreadStack);
> bool result = os::commit_memory(base, size, false);
> ```
> 
> (Please note that the false without comment didn't obey any style, so I had to fix these anyway)

Okay.

> 
> > 1. 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?
> 
> Interesting and funny, it does work. I would expected it does not (due to security reasons I could image behind forbidding MAP_JIT|MAP_FIXED). But since it works for now, I've added the executable parameter to attemp_reserve_memory_at as well.

I am not that surprised. Wish address != NULL with MAP_FIXED=0 just establishes a brand new mapping, does not change an existing mapping. I think they just forbid to modify existing mappings with MAP_JIT once established.

> 
> > 1. 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.
> 
> Ok, it is possible to do the clean up after. Thanks for your comment, I'll account them in the future clean-up RFR.
> 

Great, thanks, the change looks much cleaner now.

> Thanks,
> Anton

There are two remaining very minor nits, I leave it up to you if you fix them. From my eyes this is fine. 

-----

I think we should shake up that coding at some point and improve the API. I imagine something along the line of

struct mappinginfo_t { 
   pagesize, // may be dynamically chosen by the OS layer but caller wants to know
   exec, 
   base, size, ... // maybe
   + maybe opaque OS specific information
};
address os::reserve_xxxx(size, ..., reservation_info_t* p_info = NULL);
bool os::commit_memory(addr, size, const reservation_info_t* info);
bool os::commit_memory(addr, size, const reservation_info_t* info);

The concrete form can be shaped however, but the base idea is to return more information than just the reservation pointer:
- some attributes may be chosen or adapted by os::reserve_... (eg PageSize), but caller could just be told to spare him having to second-guess  os::reserve_memory_special().
- some attributes may be only interesting to the os layer, so they can be opaque for the caller - but caller could hold onto that information until commit/uncommit. Examples for that are AIX: mmap-or-shmat , or Windows: NUMA-striped-allocation or not.
- some information may be caller specified (eg exec) but returning this in a handle-like structure relieves the caller from holding on to that particular information, and having to pass each argument separately-

I know this is crossing territory into what ReservedSpace does today, but that class is quite polluted with VM specific stuff, eg that noaccess zone. Well, lets see how this goes.

Thanks alot for your perseverance!

Cheers, Thomas

test/hotspot/gtest/runtime/test_committed_virtualmemory.cpp line 172:

> 170:     const size_t num_pages = 4;
> 171:     const size_t size = num_pages * page_sz;
> 172:     char* base = os::reserve_memory(size, !ExecMem, mtTest);

This test function seems to leak this reservation. I leave it up to you if you want to fix it, has nothing to do with your test. If you don't could you please open a issue for this? 

(Took me some minutes to figure out that all these tests are NMT related)

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

> 3269: 
> 3270: char* os::reserve_memory_aligned(size_t size, size_t alignment, bool exec) {
> 3271:   // exec can be ignored

Change to "Support for exec not implemented" ? (Maybe even with an assert) (Or - leave that up to you - leave this parameter out of os::reserve_memory_aligned completely.)

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

Marked as reviewed by stuefe (Reviewer).

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