RFR(s): 8077276: allocating heap with UseLargePages and HugeTLBFS may trash existing memory mappings (linux)

[Coleen Phillimore]

Hi Thomas,

I noticed that you took out NMT call to track reserved memory but I 
didn't see where the code was added back to track that the memory was 
reserved.  Did you test with -XX:NativeMemoryTracking=detail (and 
-XX:+PrintNMTStatistics will show you that it's tracking things).

Thanks,
Coleen

On 4/27/15, 10:47 AM, Thomas Stüfe wrote:
> Hi Stefan,
>
> here you go:
> http://cr.openjdk.java.net/~stuefe/webrevs/8077276/webrev.04/webrev/
>
> Comments follow inline.
>
>
>> So, I'm fine with your version, but could you change the name to
>> anon_mmap_align and  change this comment:
>> 3471 // Helper for os::Linux::reserve_memory_special_huge_tlbfs_mixed().
>> 3472 // Allocate (using mmap, NO_RESERVE, with small pages) either at a
>> given request address
>> 3473 //   (req_addr != NULL) or with a given alignment.
>> 3474 // Returns NULL if mmap failed.
>> 3475 static char* anon_mmap_at_or_aligned(size_t bytes, size_t alignment,
>> char* req_addr) {
>>
>> to look more like this comment:
>> 3510 // Reserve memory using mmap(MAP_HUGETLB).
>> 3511 //  - bytes shall be a multiple of alignment.
>> 3512 //  - req_addr can be NULL. If not NULL, it must be a multiple of
>> alignment.
>> 3513 //  - alignment sets the alignment at which memory shall be allocated.
>> 3514 //     It must be a multiple of allocation granularity.
>> 3515 // Returns address of memory or NULL. If req_addr was not NULL, will
>> only return
>> 3516 //  req_addr or NULL.
>> 3517 char* os::Linux::reserve_memory_special_huge_tlbfs_mixed(size_t bytes,
>>
>> so that it's clear the at we expect req_addr to already be aligned?
>>
>>
>>
> Done.
>
>
>> Yet another half-baked though from reading this code: I wonder if we
>> shouldn't, at some point, move the call to reserve memory so that we have:
>>
>> if (is_size_aligned(bytes, os::large_page_size()) && alignment <=
>> os::large_page_size()) {
>>    return reserve_memory_special_huge_tlbfs_only(bytes, req_addr, exec);
>> } else {
>>    char* reserved_addr = anon_mmap_aligned(bytes, req_addr, alignment);
>>    if (reserved_addr != NULL) {
>>      return reserve_memory_special_huge_tlbfs_mixed(bytes, reserved_addr,
>> exec);
>>    }
>> }
>>
>> That way we tlbfs_only and tlbfs_mixed have the same parameter list and
>> are only responsible for the "commit" part of getting the memory area. This
>> would have the nice affect that we could get rid of all comments and
>> asserts about 'alignment' and 'req_addr' from
>> reserve_memory_special_huge_tlbfs_mixed and only have that in
>> anon_mmap_aligned.
>>
>>
> Hm. I guess it would make sense to create a function which, given an
> arbitrary memory range, promotes as much space as possible to large pages.
> One would have to rename them to something like "commit_hugetlb_fs" to make
> clear that no-one should call this function as a reservation function.
>
> For me there are a number of things I'd like to change first
> - the complete lack of API documentation for any of the os::reserve_.. or
> os::commit_... functions. Makes it difficult to port them to a new platform
> because you have to parse every implementation to find out what the fine
> print for those APIs is, and the implementations also differ in their
> behaviour
> - Also the naming is often misleading (e.g. what is special about
> os::reserve_memory_special? :)
> - get rid of the req_addr parameter for os::reserve_memory() altogether
> (this change is a precondition), see
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2015-April/017823.html
>
>
> Kind Regards, and thanks for reviewing!
>
> Thomas
>
>
>> Thanks,
>> StefanK
>>