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

[Anton Kozlov]

On Tue, 6 Oct 2020 16:38:47 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Hi @tstuefe,
>> 
>> Recent refactors interfered with the previous version of the patch, I found it is a bit simpler to start from scratch.
>> https://github.com/openjdk/jdk/pull/294/commits/f8664ca7dcc1cfdfb9a1f032035f2cde77048649 is a minimal patch that allows
>> MAP_JIT. I cannot see any way to simplify the interface now. Please tell me if I miss something. Also, `VirtualSpace`
>> now choose from `reserve_memory_with_fd` and anything that is used for reserve with exec.   What I found in the AIX
>> implementation, the commit/uncommit are for memory previously reserved
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/os_aix.cpp#L2227. If we attribute reserved region with
>> exec, then uncommit can query bookkeeping info and get the type of memory.  From options of internal bookkeeping info
>> or an asking the system for the mmap flags, I prefer the first one. It is faster and we can bookkeep only minimal info,
>> like a list of executable regions. That should reduce the amount of data and make a check for the type in
>> commit/uncommit faster.  After looking around, it does look like every place where commit with exec is used, we know
>> enough to use reserve/uncommit with exec. Taking this to the extreme, I still think a specialized set of
>> reserve/commit/uncommit for executable regions would look natural. For example, commit with exec is used in less than
>> five places. I'll do a little research there.
>
>> Hi @tstuefe,
>> 
>> Recent refactors interfered with the previous version of the patch, I found it is a bit simpler to start from scratch.
>> [f8664ca](https://github.com/openjdk/jdk/commit/f8664ca7dcc1cfdfb9a1f032035f2cde77048649) is a minimal patch that
>> allows MAP_JIT. I cannot see any way to simplify the interface now. Please tell me if I miss something. Also,
>> `VirtualSpace` now choose from `reserve_memory_with_fd` and anything that is used for reserve with exec.  What I found
>> in the AIX implementation, the commit/uncommit are for memory previously reserved
>> https://github.com/openjdk/jdk/blob/master/src/hotspot/os/aix/os_aix.cpp#L2227. If we attribute reserved region with
>> exec, then uncommit can query bookkeeping info and get the type of memory.
> 
> On AIX there is no explicit commit, committing happens on touch and uncommitting unfortunately does not really work
> (there is a disclaim() but its buggy). All commit does is to check the input parameters and maybe explictly touch the
> area.
>> 
>> From options of internal bookkeeping info or an asking the system for the mmap flags, I prefer the first one. It is
>> faster and we can bookkeep only minimal info, like a list of executable regions. That should reduce the amount of data
>> and make a check for the type in commit/uncommit faster.  After looking around, it does look like every place where
>> commit with exec is used, we know enough to use reserve/uncommit with exec. Taking this to the extreme, I still think a
>> specialized set of reserve/commit/uncommit for executable regions would look natural. For example, commit with exec is
>> used in less than five places. I'll do a little research there.
> 
> I thought a lot about this.
> 
> As you said, I believe now that specifying exec (or other parameters, see below) is not necessary on a per-commit; on
> per-mapping level it should be enough.
> I also see at least three separate cases where we establish a mapping and later need mapping-specific information
> somewhere until the next interaction - be it commit/uncommit or release:
> 1) On AIX, where we decide at os::reserve_memory time to use either SystemV or mmap and later need to know
> 2) On Linux when TPH are active the information "Use TPH" is handed down to os::commit via the weird alignment_hint
> parameter and that os::realign_memory() function. Following that parameter flow, it is just basically a way to flag the
> code to do one extra madvice at commit time. 3) Your case now, where we need to know if the mapping was supposed to be
> executable at commit time.  I really think a generic solution would be best. One simple variant would be to return not
> a pointer but a handle and let the platform store behind that handle whatever it needs to keep on a per-mapping base:
> handle_t os::reserve_memory(size, input flags);
> void* start_address = os::get_reserved_memory_start_address(handle_t)
> bool os::commit_memory(handle_t, address, size)
> 
> and so on. The only problem with that is that it would cause a lot of call sites to change, and the callers need to
> hold on to that mapping handle.
> To be clear, I do not think you should do this with this patch, but I would like your opinion, since you looked at the
> code closely.
> I'll review the current version presently.

Sorry, I had not highlighted that was a proof-of-concept patch to show API changes. I've pushed another PoC with
bookkeeping and no API changes at all. But I don't like the new one either. In the new patch, there is a list of
(potentially) executable regions that is updated on commit, when the actual desired (non)exec mode become known. If we
support mixed exec/non-exec commits in a mapping, then after non-exec commit a part of the mapping cannot be reversed
to a potentially executable one (as we've lost MAP_JIT). Then it can produce some unexpected results under _some_
conditions in runtime, while API users can be unconscious about potential issues. Good API should not allow that.

>  specifying exec ... on per-mapping level it should be enough.

With this, it is possible to simplify the implementation without API changes. But it will still be 1) reserve and be
prepare for the first exec or non-exec commit 2) on commit, finish reserve and turn the mapping to the exec or
non-exec. All this instead of taking direct parameter "this is a executable mapping" on reserve.

The current "commit only knows about exec" is just a leak of implementation details, as before it was only required to
know executable mode. Providing exec parameter to reserve will just bring consistency to the interface. Or, a separate
interface for exec (code) mappings will serve the same and will be better, as it will simplify the general non-code
reserve/commit interface.

> I also see at least three separate cases where we establish a mapping and later need mapping-specific information
> somewhere until the next interaction - be it commit/uncommit or release: [ AIX SystemV or mmap, Linux THP, macOS
> MAP_JIT for code ]

Could you explain how the choice between SysV and mmap is made on AIX? It looks like
  
develop(uintx, Use64KPagesThreshold, 0,                                           \
          "4K/64K page allocation threshold.")                                      \
...
  if (os::vm_page_size() == 4*K) {
    return reserve_mmaped_memory(bytes, NULL /* requested_addr */);
  } else {
    if (bytes >= Use64KPagesThreshold) {
      return reserve_shmated_memory(bytes, NULL /* requested_addr */);
    } else {
      return reserve_mmaped_memory(bytes, NULL /* requested_addr */);
    }
  }
(there only two calls to reserve_shmated_memory and both of them are like above. Is SysV SHM used in product builds?)
For now, the AIX case looks a bit different. The choice is made by the platform and the shared code cannot control
this. So yes, I cannot see how to avoid handle_t or similar.

In contrast, THP and MAP_JIT are the way to implement a request from the shared code. Even for THP, shared code seems
to know why it should "realign" (not sure why commit has an alignment_hint parameter, while it is possible to realign
after a regular commit). I assume there is enough context in the shared code that can be provided for platform
functions, without a handle_t. And the same context should anyway be provided to reserve function, so handle_t can be
filled with all necessary information.

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

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