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

[Thomas Stüfe]

Hi Colleen,

thanks for looking at the change!

On Mon, Apr 27, 2015 at 5:59 PM, Coleen Phillimore <
coleen.phillimore at oracle.com> wrote:
>
>
> 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.


I took out this coding intentionally; it was a workaround which was not
needed anymore:

Before my change, os::reserve_memory_special() called
os::Linux::reserve_memory_special_huge_tlbfs_mixed() which eventually
called os::reserve_memory() on all its code paths.

os::reserve_memory_special() registers allocated memory with NMT, and so
does os::reserve_memory(). To prevent this memory being registered twice,
os::Linux::reserve_memory_special_huge_tlbfs_mixed() de-registered that
memory returned by os::reserve_memory(), because it expected the caller,
os::reserve_memory_special(), to register that memory again.

That workaround was imho a bit ugly, and its reason was that than outward
API calls an internal API which again calls an outward API, the outward
APIs being covered by NMT.

My change replaces the inner call to os::reserve_memory() with plain mmap()
calls - originally because of the mmap(MAP_FIXED) issue described in the
bug report, but the side effect was that this NMT related workaround could
be removed.

> Did you test with -XX:NativeMemoryTracking=detail (and
-XX:+PrintNMTStatistics will show you that it's tracking things).
>

I did test this and it works. Unfortunately not easy to test: you want to
test that a call to os::reserve_memory_special() correctly tracks with NMT
if it takes a code path which end in
os::Linux::reserve_memory_special_huge_tlbfs_mixed(). I played with options
but was not able to hit the code path, so I had to force the code path in
the code, which is not part of my hg change, of course.

You do can test os::Linux::reserve_memory_special_huge_tlbfs_mixed() in an
isolated way with -XX:+ExecuteInternalVMTests , but that test is "below"
NMT, therefore you don't see any tracking.

Kind Regards, Thomas



> 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
>>>
>>>
>