RFR: NMT: Don't record uncommits unconditionally

[Stefan Karlsson]

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

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