RFR: 8146523: Incorrect memory tracker used by os::unmap_memory()
Ioi Lam
ioi.lam at oracle.com
Thu Jan 7 20:23:24 UTC 2016
On 1/7/16 11:51 AM, Coleen Phillimore wrote:
>
>
> On 1/7/16 2:34 PM, Ioi Lam wrote:
>> Hi Jiangli,
>>
>> I looked at the implementatiion of os::pd_unmap_memory on windows but
>> I can't figure out how it would affect the memory tracker:
>>
>> os.cpp:
>>
>> bool os::unmap_memory(char *addr, size_t bytes) {
>> bool result;
>> if (MemTracker::tracking_level() > NMT_minimal) {
>> + #ifndef _WINDOWS
>> + Tracker tkr =
>> MemTracker::get_virtual_memory_uncommit_tracker();
>> + #else
>> + // Windows pd_unmap_memory also does memory release, so use
>> + // the virtual_memory_release_tracker for windows
>> Tracker tkr =
>> MemTracker::get_virtual_memory_release_tracker();
>> + #endif
>>
>> os_windows.cpp:
>>
>> bool os::pd_unmap_memory(char* addr, size_t bytes) {
>> MEMORY_BASIC_INFORMATION mem_info;
>> if (VirtualQuery(addr, &mem_info, sizeof(mem_info)) == 0) {
>> if (PrintMiscellaneous && Verbose) {
>> DWORD err = GetLastError();
>> tty->print_cr("VirtualQuery() failed: GetLastError->%ld.",
>> err);
>> }
>> return false;
>> }
>>
>> // Executable memory was not mapped using
>> CreateFileMapping/MapViewOfFileEx.
>> // Instead, executable region was allocated using
>> VirtualAlloc(). See
>> // pd_map_memory() above.
>> //
>> // The following flags should match the 'exec_access' flages
>> used for
>> // VirtualProtect() in pd_map_memory().
>> if (mem_info.Protect == PAGE_EXECUTE_READ ||
>> mem_info.Protect == PAGE_EXECUTE_READWRITE) {
>> return pd_release_memory(addr, bytes);
>> }
>>
>> BOOL result = UnmapViewOfFile(addr);
>> if (result == 0) {
>> if (PrintMiscellaneous && Verbose) {
>> DWORD err = GetLastError();
>> tty->print_cr("UnmapViewOfFile() failed: GetLastError->%ld.",
>> err);
>> }
>> return false;
>> }
>> return true;
>> }
>>
>> bool os::pd_release_memory(char* addr, size_t bytes) {
>> return VirtualFree(addr, 0, MEM_RELEASE) != 0;
>> }
>>
>> Note this code doesn't use MemTracker at all.
>
> Right, because the MemTracker call is done at the higher level so
> pd_unmap_memory just does the unmapping.
>
>>
>> So I have the fear that this fix doesn't really address the problem,
>> and potentially it could cause other issues for the non-windows
>> platforms, since you're unconditionally switching the code to use a
>> different tracker.
>>
>> I noticed the different usage of MemTracker in the os_<xxx>.cpp files:
>>
>> hotspot/src/os$ grep MemTracker:: */*/os_*.cpp | sed -e
>> 's/[(][^(]*//g' | sort | uniq
>>
>> ./bsd/vm/os_bsd.cpp:
>> MemTracker::record_virtual_memory_reserve_and_commit
>> ./bsd/vm/os_bsd.cpp: Tracker tkr =
>> MemTracker::get_virtual_memory_release_tracker
>> ./linux/vm/os_linux.cpp:
>> MemTracker::record_virtual_memory_reserve_and_commit
>> ./linux/vm/os_linux.cpp: Tracker tkr =
>> MemTracker::get_virtual_memory_release_tracker
>> ./windows/vm/os_windows.cpp:
>> MemTracker::record_virtual_memory_reserve
>> ./windows/vm/os_windows.cpp:
>> MemTracker::record_virtual_memory_reserve_and_commit
>>
>> So ONLY windows calls MemTracker::record_virtual_memory_reserve
>> (under allocate_pages_individually). Perhaps this is a better
>> explanation why NMT behaves differently on Windows vs other platforms?
>
> No, I think it's because on Linux, you call os::reserve_memory()
> allocation on the whole CDS archive and call os::map_memory() to map
> them to the archive file. On windows, the os::reserve_memory() isn't
> done.
>
> There are some workarounds (hacks) to MemTracker in the os specific
> files. They are to handle large pages (the os::reserve_memory_special
> file is pushed to the platform specific files so needs to have NMT
> tracking there), and for windows, a special case because it has to
> reserve and unreserve memory.
>
> ie.
> MemTracker::record_virtual_memory_reserve((address)p_buf,
> size_of_reserve, CALLER_PC);
> os::release_memory(p_buf, bytes + chunk_size);
>
> os::release_memory() calls virtual_memory_release so we have to make
> sure NMT counts it as reserved first. This should probably call
> pd_release_memory.
>
> The function allocate_pages_individually looks like it's to handle
> NUMA/large pages also. I think the else case in
> os::pd_reserve_memory() is odd, but I think this is unrelated to
> Jiangli's problem.
>>
>> Without understand how NMT works under all conditions, I would
>> hesitate to make the change you're proposing.
>>
>
> I think the change is perfectly safe. It doesn't change existing
> behavior for windows and fixes a known crash for a reason we
> understand for Linux.
OK, I trust you -- although I suspect nightly tests may find cases where
we call os::unmap_memory() on a block that wasn't returned by
os::map_memory, and NMT would complain that the block wasn't marked as
committed. But that would be a bug of the caller of os::unmap_memory,
but an NMT bug.
In any case, the comment needs to be fixed to explain why the #ifdef is
needed. The current comment is wrong.
Thanks
- Ioi
> Coleen
>
>> Thanks
>> - Ioi
>>
>> On 1/7/16 10:50 AM, Jiangli Zhou wrote:
>>> Hi Coleen,
>>>
>>>> On Jan 6, 2016, at 6:04 PM, Coleen Phillimore
>>>> <coleen.phillimore at oracle.com> wrote:
>>>>
>>>>
>>>>
>>>> On 1/6/16 7:54 PM, Jiangli Zhou wrote:
>>>>> Hi David,
>>>>>
>>>>>> On Jan 5, 2016, at 10:33 PM, David Holmes
>>>>>> <david.holmes at oracle.com> wrote:
>>>>>>
>>>>>> Hi Jiangli,
>>>>>>
>>>>>> On 6/01/2016 8:30 AM, Jiangli Zhou wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> Please review the small fix for JDK-8146523
>>>>>>> <https://bugs.openjdk.java.net/browse/JDK-8146523>:
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~jiangli/8146523/webrev.00/
>>>>>>>
>>>>>>> The os::map_memory() and os:unmapp_memory() functions use the
>>>>>>> memory tracker asymmetrically. The os::map_memory() function
>>>>>>> calls MemTracker::record_virtual_memory_reserve_and_commit() to
>>>>>>> track the mapped memory, while os::unmap_memory() uses the
>>>>>>> 'virtual_memory_release_tracker' to track the memory being
>>>>>>> unmapped. In NMT, there are different types of tracker for
>>>>>>> releasing and uncommitting memory. The
>>>>>>> ‘virtual_memory_uncommit_tracker' is for 'uncommit’ and
>>>>>>> ‘virtual_memory_release_tracker’ is for 'release'. The
>>>>>>> os::unmap_memory() should use the
>>>>>>> ‘virtual_memory_uncommit_tracker' to be balanced with
>>>>>>> os::map_memory().
>>>>>> I find this very confusing. Is there some document that describes
>>>>>> the relationship between mapping, reserving and committing
>>>>>> virtual memory and the related NMT functionality?
>>>>> I agree. This is very confusing. I haven’t come across any
>>>>> document explaining the usage of the NMT APIs in different
>>>>> scenarios. My understand is based on reading the related code.
>>>>>
>>>>>> It is not at all clear to me why reserve_and_commit pairs with
>>>>>> uncommit (unless committed memory must first be reserved, and
>>>>>> uncommit both uncommits and "unreserves" ??)
>>>>> I dug deeper into it and found more issues. It appears
>>>>> MemTracker::record_virtual_memory_reserve_and_commit() should
>>>>> record the memory as both ‘reserved’ and ‘committed’. However,
>>>>> that only happens if the memory region is not known by the
>>>>> VirtualMemoryTracker. If the memory overlaps with a known region
>>>>> in the VirtualMemoryTracker,
>>>>> MemTracker::record_virtual_memory_reserve_and_commit() could be a
>>>>> ‘nop' in certain case. In the bug revealed by the specific test
>>>>> scenario, the memory region causing the issue is the CDS shared
>>>>> memory. Here are the call sequences related to the shared memory
>>>>> region:
>>>>>
>>>>> 1. ReservedSpace::ReservedSpace - reserve the memory for shared
>>>>> region. VirtualMemorySummary::record_reserved_memory is called and
>>>>> the memory is recored as ‘reserved’.
>>>>> 2.os::map_memory - each shared space within the shared memory
>>>>> region is mapped.
>>>>> MemTracker::record_virtual_memory_reserve_and_commit is called,
>>>>> the memory is not recounted. So the shared memory region is only
>>>>> counted as ‘reserved’ in NMT by previous
>>>>> VirtualMemorySummary::record_reserved_memory call.
>>>>> 3.os::unmap_memory - record each shared space within the shared
>>>>> memory region with virtual_memory_release_tracker
>>>>> 4 ReservedSpace::release (calls os::release_memory) - the
>>>>> virtual_memory_release_tracker is used again for the shared memory
>>>>> region.
>>>>>
>>>>> On linux and all other non-windows platform, os::unmap_memory only
>>>>> unmap the memory (by calling system munmap) without ‘release’ the
>>>>> memory. On windows, unmap_memory also does ‘release’.
>>>> Yes, this is the difference between windows and unix machines. I
>>>> think NMT was developed on windows.
>>>>> So, there are at lease two issues:
>>>>>
>>>>> The shared memory region is not recorded as ‘committed’.
>>>> File a P4 bug on this because I don't think it causes any crashes.
>>>> I think it may under count the amount of committed memory. We can
>>>> investigate.
>>> I filed JDK-8146637 <https://bugs.openjdk.java.net/browse/JDK-8146637>.
>>>
>>>>> The shared memory region is counted twice as ‘released’.
>>>> Your change conditionalized for Windows in os.cpp would be the best
>>>> thing. Making the NMT call at a lower level ie. pd_unmap_memory
>>>> runs into the problem that there are duplicates for the xnix
>>>> platforms. Also, there are multiple returns that would need NMT
>>>> logic so it wouldn't look good. Also, there is risk because some of
>>>> the os platforms call pd_unmap_memory by
>>>> os::pd_attempt_reserve_memory_at() which would get the tracking
>>>> wrong (I think).
>>> Thanks for the suggestion. Here is the updated webrev:
>>>
>>> http://cr.openjdk.java.net/~jiangli/8146523/webrev.01/
>>>
>>> Thanks!
>>>
>>> Jiangli
>>>
>>>> Coleen
>>>>
>>>>> Thanks,
>>>>> Jiangli
>>>>>
>>>>>> Thanks,
>>>>>> David
>>>>>> -----
>>>>>>
>>>>>>> Tested with test/runtime/NMT.
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Jiangli
>>
>
More information about the hotspot-runtime-dev
mailing list