RFR: 8146523: Incorrect memory tracker used by os::unmap_memory()
Jiangli Zhou
jiangli.zhou at oracle.com
Thu Jan 7 21:30:39 UTC 2016
Thanks Coleen for filling in all the details!
> On Jan 7, 2016, at 12:23 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>
>
>
> 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.
Could you please explain why the comment is wrong? On linux/unix platforms, we have extra code to do explicit memory ‘reserve’ and ‘release’, which calls NMT to track the reserve and release of the related memory region. Mapping and unmaping are performed on the memory regions that are already ‘reserved’ on those platform. On windows, there are no explicit calls to do memory ‘reserve’ before mapping and ‘release’ after unmapping. So the memory mapping/unmapping APIs have the implicit ‘reserve’/‘release’ behaviors. That’s why for windows the release_tracker is used to reflect the behavior of unmapping with implicit ‘release’, and the linux/unix only needs the uncommit_tracker.
Thanks,
Jiangli
>
> 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