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

Wed Apr 29 09:09:07 UTC 2015

Hi Stefan,

great, thank you!

I think Coleen only looked at the NMT related changes, so we will need
another reviewer (and a sponsor.

Kind Regards, Thomas

On Wed, Apr 29, 2015 at 10:19 AM, Stefan Karlsson <
stefan.karlsson at oracle.com> wrote:

>  Hi Thomas,
>
>
> On 2015-04-28 15:52, Thomas Stüfe wrote:
>
> Hi Stefan,
>
>  webrev #5:
> http://cr.openjdk.java.net/~stuefe/webrevs/8077276/webrev.05/webrev/
>
>  See comments inline
>
> On Tue, Apr 28, 2015 at 11:17 AM, Stefan Karlsson <
> stefan.karlsson at oracle.com> wrote:
>
>>  Hi Thomas,
>>
>> On 2015-04-27 16:47, Thomas Stüfe wrote:
>>
>> Hi Stefan,
>>
>>  here you go:
>> http://cr.openjdk.java.net/~stuefe/webrevs/8077276/webrev.04/webrev/
>>
>>
>>  You still have:
>> +        if (p) {
>>
>> and
>>
>> i ++
>>
>> in the tests.
>>
>>
>  fixed.
>
>
>>  ---
>> The formatting is broken:
>>    2097152 0x      1000 ->  0x0000000000000000 (failed)
>>
>> "0x      1000" is missing zeros.
>>
>
>  The macro SIZE_FORMAT_HEX_W from globalDefinitions.hpp is broken. We
> should fix this. For now I switch to the non-width variant.
>
>
>>
>> ---
>> Is there a reason why the size is written in dec, while align is written
>> in hex? I'd prefer if both were written as hex.
>>
>
>  Switched all to hex.
>
>
>>
>> ---
>> Regarding the test_log output:
>>
>> Test case 1: (p != NULL ? "" : "(failed)")
>> Test case 2: (p != NULL ? (p == req_addr ? "(exact match)" : "") :
>> "(failed)")
>> Test case 3: (p != NULL ? "" : "failed")
>>
>>
>> 1) The third test case prints "failed" instead of "(failed)".
>>
>>
> fixed
>
>
>
>>  2) The second test case prints "" when p != req_addr, which is wrong. If
>> we get a p != req_addr, then that's a failure.
>>
>> 3)  The third test case always print "failed", since the test case is
>> setup to always fail mmaping the requested address. Does the "failed"
>> string denote a failed mapping or that the test failed?
>>
>>
>  The tests were written for a more lenient version of
> os::Linux::reserve_memory_special_huge_tlbfs_mixed(). I changed them
> accordingly.
>
>  Note that the log output describes the output of the reservation
> function, and "failed" means it failed to map. In that sense, if it returns
> an address != req_addr, it did not fail.
>
>  I test for the additional requirements (like, req_addr == addr)
> afterwards with assertions.
>
>
> I'm fine with your latest patch. I want somone from the Runtime team to
> also review the changes. I'm not sure if Coleen looked at the non-NMT parts
> of the patch.
>
> Thanks,
> StefanK
>
>
>
>  Thanks, Thomas
>
>
>
>>  Thanks,
>> StefanK
>>
>>
>>
>>  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
>>>
>>>>
>>>>>>
>>
>
>