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

[Stefan Karlsson]

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.

---
The formatting is broken:
    2097152 0x      1000 ->  0x0000000000000000 (failed)

"0x      1000" is missing zeros.

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

---
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)".

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?

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