RFR: NMT: Don't record uncommits unconditionally

[Stefan Karlsson]

[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