RFR: 8146523: Incorrect memory tracker used by os::unmap_memory()

[Coleen Phillimore]

The latest webrev looks good.  Thank you for filing a bug on the other 
issue.

Thanks,
Coleen

On 1/7/16 1:50 PM, Jiangli Zhou wrote:
> Hi Coleen,
>
>> On Jan 6, 2016, at 6:04 PM, Coleen Phillimore 
>> <coleen.phillimore at oracle.com <mailto: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 
>>>> <mailto: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/ 
>>>>> <http://cr.openjdk.java.net/%7Ejiangli/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/ 
> <http://cr.openjdk.java.net/%7Ejiangli/8146523/webrev.01/>
>
> Thanks!
>
> Jiangli
>
>>
>> Coleen
>>
>>> Thanks,
>>> Jiangli
>>>
>>>> Thanks,
>>>> David
>>>> -----
>>>>
>>>>> Tested with test/runtime/NMT.
>>>>>
>>>>> Thanks,
>>>>> Jiangli
>