RFR: NMT: Don't record uncommits unconditionally

[Zhengyu Gu]

On 01/11/2018 08:51 AM, Stefan Karlsson wrote:
> 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.
Ah, Thanks!

But I still want to throw away 
MemTracker::get_virtual_memory_uncommit_tracker() and create Tracker in 
place.

-Zhengyu

> 
> 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