RFR: NMT: Don't record uncommits unconditionally

[Per Lidén]

> On 11 Jan 2018, at 16:31, Stefan Karlsson <stefan.karlsson at oracle.com> wrote:
> 
>> On 2018-01-11 15:15, Zhengyu Gu wrote:
>>> 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.
> 
> OK. Will you take care of it?
> 
> If you want, you can take over this RFE:
> https://bugs.openjdk.java.net/browse/JDK-8194926
> 
> Here's the updated ZGC patch.
> 
> http://cr.openjdk.java.net/~stefank/zgc/zNMTBugFix/webrev.02

Looks good!

/Per

> 
> The move of the nmt_uncommit call is not strictly necessary, because both the "unmap" and the "alloc" of the physical memory is protected by the same lock, but it makes the the code a bit more future proof.
> 
> Thanks,
> StefanK
> 
>> -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