RFR: NMT: Don't record uncommits unconditionally

[Zhengyu Gu]

On 01/11/2018 08:33 AM, Zhengyu Gu wrote:
> On 01/11/2018 05:28 AM, Stefan Karlsson wrote:
>> If we decide that the below change is valid and an improvement, then I 
>> have a patch for this:
>>
>> http://cr.openjdk.java.net/~stefank/8194926/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8194926
>> 8194926: NMT: Clean up usage of 
>> get_record_virtual_memory_uncommit/release
> 
> Thanks for the cleanup. Looks good to me.

Oops.

You do need

    static inline void record_virtual_memory_uncommit(void* addr, size_t 
size) { }
  235   static inline void record_virtual_memory_release(void* addr, 
size_t size) { }

in #if !INCLUDE_NMT section.

Thanks,

-Zhengyu

> 
> -Zhengyu
> 
>>
>> While running the NMT jtreg tests with ZGC, I found that one of the 
>> tests used ints to parse size_t values. Here's and upstream patch for 
>> that.
>>
>> http://cr.openjdk.java.net/~stefank/8194925/webrev.01/
>> https://bugs.openjdk.java.net/browse/JDK-8194925
>> 8194925: NMT: SummarySanityCheck test can't parse values > max_jint
>>
>> Thanks,
>> StefanK
>>
>> On 2018-01-11 10:09, Stefan Karlsson wrote:
>>> [Looping in Zhengyu]
>>>
>>> Inlined:
>>>
>>> On 2018-01-11 07:56, Per Liden wrote:
>>>> Hi,
>>>>
>>>> On 01/10/2018 05:01 PM, Stefan Karlsson wrote:
>>>>> Hi all,
>>>>>
>>>>> Please review this patch to fix a bug in the recent NMT 
>>>>> implementation for ZGC.
>>>>>
>>>>> http://cr.openjdk.java.net/~stefank/zgc/zNMTBugFix/webrev.01
>>>>
>>>> Change looks good, but the memTracker.cpp part should probably be 
>>>> split out into an upstream patch. Also, it feels like we should 
>>>> adjust the other user of this in os.cpp to use the new function.
>>>
>>>
>>> Looking at the os.cpp function:
>>>
>>> bool os::uncommit_memory(char* addr, size_t bytes) {
>>>    bool res;
>>>    if (MemTracker::tracking_level() > NMT_minimal) {
>>>      Tracker tkr = MemTracker::get_virtual_memory_uncommit_tracker();
>>>      res = pd_uncommit_memory(addr, bytes);
>>>      if (res) {
>>>        tkr.record((address)addr, bytes);
>>>      }
>>>    } else {
>>>      res = pd_uncommit_memory(addr, bytes);
>>>    }
>>>    return res;
>>> }
>>>
>>> Is there a reason why the Tracker object is created before the 
>>> pd_uncommit_memory, while the record() and ~Tracker is called after?
>>>
>>> Could we change the code above to:
>>>
>>> bool os::uncommit_memory(char* addr, size_t bytes) {
>>>    bool res = pd_uncommit_memory(addr, bytes);
>>>    if (res && MemTracker::tracking_level() > NMT_minimal) {
>>>      Tracker tkr = MemTracker::get_virtual_memory_uncommit_tracker();
>>>      tkr.record((address)addr, bytes);
>>>    }
>>>    return res;
>>> }
>>>
>>> or even:
>>>
>>> bool os::uncommit_memory(char* addr, size_t bytes) {
>>>    bool res = pd_uncommit_memory(addr, bytes);
>>>    if (res) {
>>>      MemTracker::record_virtual_memory_uncommit(addr, bytes)
>>>    }
>>>    return res;
>>> }
>>>
>>> Then these functions would look very similar to the commit functions:
>>>
>>> bool os::commit_memory(char* addr, size_t size, size_t alignment_hint,
>>>                                bool executable) {
>>>    bool res = os::pd_commit_memory(addr, size, alignment_hint, 
>>> executable);
>>>    if (res) {
>>>      MemTracker::record_virtual_memory_commit((address)addr, size, 
>>> CALLER_PC);
>>>    }
>>>    return res;
>>> }
>>>
>>>
>>> I see that Tracker() and ~Tracker() locks and unlocks ThreadCritical. 
>>> Do we need to be in "thread critical" while unmapping memory?
>>>
>>> If we can do the change above, then I think we can get rid of 
>>> get_virtual_memory_uncommit_tracker and 
>>> get_virtual_memory_release_tracker.
>>>
>>> Thanks,
>>> StefanK
>>>
>>>>
>>>> cheers,
>>>> Per
>>>>
>>>>>
>>>>> The uncommit path was executed even when NMT was turned off. 
>>>>> Because of this we hit the following assertion when pages were 
>>>>> detached:
>>>>>
>>>>> #  Internal Error (.../services/memTracker.hpp:231), pid=12751, 
>>>>> tid=2738
>>>>> #  assert(tracking_level() >= NMT_summary) failed: Check by caller
>>>>>
>>>>> Thanks,
>>>>> StefanK