RFR: 8262291: Refactor reserve_memory_special_huge_tlbfs

[Stefan Johansson]

On Tue, 23 Mar 2021 16:58:19 GMT, Ivan Walulya <iwalulya at openjdk.org> wrote:

>> Please review this refactoring of the hugetlbfs reservation code.
>> 
>> **Summary**
>> In recent adventures in this area of the code I noticed a strange condition in `reserve_memory_special_huge_tlbfs` where we take the "mixed-mapping" route even if the size doesn't require any small pages to be used:
>>   if (is_aligned(bytes, os::large_page_size()) && alignment <= os::large_page_size()) {
>>     return reserve_memory_special_huge_tlbfs_only(bytes, req_addr, exec);
>>   } else {
>>     return reserve_memory_special_huge_tlbfs_mixed(bytes, alignment, req_addr, exec);
>>   }
>> 
>> The second condition here is needed because if the alignment is larger than the large page size, we needed to enforce this and can't just trust `mmap` to give us a properly aligned address. Doing this by using the mixed-function feels a bit weird and looking a bit more at this I found a way to refactor this function to avoid having the two helpers. 
>> 
>> Instead of only having the mixed path honor the passed down alignment, make sure that is always done. This will also have the side-effect that all large pages in a "mixed"-mapping will be at the start and then we will have a tail of small pages. This actually also ensures that we will use large pages for a mixed mapping, in the past there was a corner case where we could end up with just a head and tail of small pages and no large page in between (if the mapping was smaller than 2 large pages and there was no alignment constraint).
>> 
>> **Testing**
>> Mach5 tier1-3 and a lot of local testing with different large page configurations.
>
> src/hotspot/os/linux/os_linux.cpp line 3929:
> 
>> 3927: }
>> 3928: 
>> 3929: char* os::Linux::reserve_and_commit_special(size_t bytes,
> 
> method name `reserve_and_commit_`  implicitly suggests that other methods with just `reserve_memory_` do not commit (to me).

That is a good point, I've struggled a bit with this function name. Since we actually do the reservation using the call to `anon_mmap_aligned()` maybe this one should just be called: `commit_memory_special()`

> src/hotspot/os/linux/os_linux.cpp line 3981:
> 
>> 3979:   // and the given alignment. The larger of the two will be used.
>> 3980:   size_t required_alignment = MAX(os::large_page_size(), alignment);
>> 3981:   char* const aligned_start = anon_mmap_aligned(req_addr, bytes, required_alignment);
> 
> Do we need to add back the comment "// First reserve - but not commit"?

My hope was that the new comment would be sufficient, but if you think it is needed I could add that the reserved range is not committed here.

> src/hotspot/os/linux/os_linux.cpp line 3990:
> 
>> 3988:   char* large_mapping = reserve_and_commit_special(large_bytes, os::large_page_size(), aligned_start, exec);
>> 3989: 
>> 3990:   if (bytes == large_bytes) {
> 
> Shouldn't we do the check `if (large_mapping == NULL) {` before this?

We could, and I thought about adding a comment here. I probably should have. The reason we don't do the explicit check it is that if `large_mapping == NULL` then NULL will be returned as expected and there is no additional work to be done if `bytes == large_bytes`.

> src/hotspot/os/linux/os_linux.cpp line 3998:
> 
>> 3996:   char* small_start = aligned_start + large_bytes;
>> 3997:   size_t small_size = bytes - large_bytes;
>> 3998:   if (large_mapping == NULL) {
> 
> See comment above

I do the check here because I need the `small_start` and `small_size` values if `large_mapping == NULL` and there was additional memory reserved for small pages.

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

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