RFR(s): 8077276: allocating heap with UseLargePages and HugeTLBFS may trash existing memory mappings (linux)
Stefan Karlsson
stefan.karlsson at oracle.com
Wed Apr 29 08:19:23 UTC 2015
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/
> <http://cr.openjdk.java.net/%7Estuefe/webrevs/8077276/webrev.05/webrev/>
>
> See comments inline
>
> On Tue, Apr 28, 2015 at 11:17 AM, Stefan Karlsson
> <stefan.karlsson at oracle.com <mailto: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/
>> <http://cr.openjdk.java.net/%7Estuefe/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
>>
>>
>
>
More information about the hotspot-runtime-dev
mailing list