RFR: NMT: Don't record uncommits unconditionally

[Zhengyu Gu]

On 01/11/2018 10:31 AM, Stefan Karlsson 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?
> 
Yes. https://bugs.openjdk.java.net/browse/JDK-8194934

Thanks,

-Zhengyu


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