RFR: 8146523: Incorrect memory tracker used by os::unmap_memory()
Coleen Phillimore
coleen.phillimore at oracle.com
Thu Jan 7 19:51:41 UTC 2016
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.
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