RFR(s): 8077276: allocating heap with UseLargePages and HugeTLBFS may trash existing memory mappings (linux)
Coleen Phillimore
coleen.phillimore at oracle.com
Mon Apr 27 20:45:39 UTC 2015
On 4/27/15, 1:47 PM, Thomas Stüfe wrote:
> Hi Colleen,
>
> thanks for looking at the change!
>
> On Mon, Apr 27, 2015 at 5:59 PM, Coleen Phillimore
> <coleen.phillimore at oracle.com <mailto: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.
Thanks for the quick reply. I see it all now. This is a lot nicer
1
>
> > 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.
>
Thanks for doing this extra verification. This doesn't look like it'll
break anything with NMT.
Coleen
(ps. no problem with my name, it's actually an alternate spelling anyway).
> 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/
> <http://cr.openjdk.java.net/%7Estuefe/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
>
>
>
More information about the hotspot-runtime-dev
mailing list