RFR: 8262291: Refactor reserve_memory_special_huge_tlbfs [v4]

[Thomas Stuefe]

On Thu, 25 Mar 2021 08:20:05 GMT, Stefan Johansson <sjohanss 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.
>
> Stefan Johansson has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Self review.
>   
>   Update helper name to better match commit_memory_special().

Hi Stefan,

this is a welcome cleanup!

Remarks inline.

Cheers, Thomas

src/hotspot/os/linux/os_linux.cpp line 3932:

> 3930:                                        size_t page_size,
> 3931:                                        char* req_addr,
> 3932:                                        bool exec) {

I'd prefer if this were file scope static and not exported (don't think this needs anything from the os::Linux namespace, or?).

Also, mid term we could probably merge this with commit_memory. AFAICS the only differences to the latter is that this can do huge pages, and the error handling is a bit different.

I am also not sure the return type makes a lot of sense. Either we handle commit errors inside this function, then we should return void. Or, we return a boolean and handle it in the caller.

Long term I would prefer to handle allocation errors not by aborting but by leaving it up to the caller what to do. Because it makes sense to have the option to "reserve if we get large pages, otherwise just use small pages". Eg this I would like to do in a future Metaspace.

src/hotspot/os/linux/os_linux.cpp line 3965:

> 3963:                                                    size_t alignment,
> 3964:                                                    char* req_addr,
> 3965:                                                    bool exec) {

So the contract is that this function will allocate huge paged memory with whatever page size is the default (controlled with UseLargePages and LargePageSizeInBytes).

And with Markus future changes we will mix-and-match page sizes best as we can. So, control of page size is out of the hands of the caller. Are there callers misusing alignment for page size?

Otherwise this seems fine to me.

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

Changes requested by stuefe (Reviewer).

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