RFR: 8255254: Split os::reserve_memory and os::map_memory_to_file interfaces [v2]

[Thomas Stuefe]

On Tue, 27 Oct 2020 14:30:48 GMT, Anton Kozlov <akozlov at openjdk.org> wrote:

>> src/hotspot/os/linux/os_linux.cpp line 4207:
>> 
>>> 4205: }
>>> 4206: 
>>> 4207: char* os::pd_attempt_map_memory_to_file(char* requested_addr, size_t bytes, int file_desc) {
>> 
>> This is fine, but I am not sure anymore what the point was of first reserving, then replacing the mapping in the first place. Seems to me one could call os::map_memory_to_file() directly, or am I missing something? 
>> 
>> I think this was somehow all related to https://openjdk.java.net/jeps/316 and allocating on NVDIMM.
>
> `os::map_memory_to_file` assumes the base strictly and uses `MAP_FIXED`, when it is not null https://github.com/openjdk/jdk/blob/ae72b5283b5b5eee0fbb6c9121494a4e65fb381c/src/hotspot/os/posix/os_posix.cpp#L275
> So, to treat the address as a hint only, pd_attempt_reserve_memory_at is called

Missed that. Okay, I see. It is the same then as `os::replace_existing_mapping_with_file_mapping` - which is actually just a wrapper with an assert - and those could be melded into one.
But as I said, I'm fine doing this in a separate cleanup.

>> src/hotspot/os/posix/os_posix.cpp line 329:
>> 
>>> 327: 
>>> 328:   if (end_offset > 0) {
>>> 329:       os::release_memory(extra_base + begin_offset + size, end_offset);
>> 
>> Not your patch, but the name "end_offset" is seriously confusing here...
>
> Probably yes. But if you don't mind, I would leave that as is. A nice picture above provides some clarification and I'd really like to avoid touching unrelated code

Sure.

>> src/hotspot/os/posix/os_posix.cpp line 335:
>> 
>>> 333: }
>>> 334: 
>>> 335: // Multiple threads can race in this code, and can remap over each other with MAP_FIXED,
>> 
>> I think you could either completely remove the comment - its somewhat obvious - or remove it down to the call of chop_extra_memory.
>
> I would like to persist the comment (as the race is not evident for the reader). As for the placement, the comment describes an alternative implementation of this function and provides rationale why the current one is taken. chop_extra_memory is a step of the implementation, that has no alternative. I think it would be wrong to move the comment there. But I've added a comment for chop_extra_memory for clarity.

Okay.

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

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