RFR: NMT: Don't record uncommits unconditionally

[Stefan Karlsson]

Hi Zhengyu,

I think I found the reason why this uses the Tracker like this:

// Tracker is used for guarding 'release' semantics of virtual memory 
operation, to avoid
// the other thread obtains and records the same region that is just 
'released' by current
// thread but before it can record the operation.

So, my proposed patch would undo that. I'll throw away that upstream 
patch and only create a proper ZGC change.

Thanks,
StefanK

On 2018-01-11 14:38, Zhengyu Gu wrote:
> 
> 
> 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