[16] RFR(T) 8248426: NMT: VirtualMemoryTracker::split_reserved_region() does not properly update summary counting

[Zhengyu Gu]

Thanks, Yumin.

-Zhengyu

On 7/9/20 11:43 AM, Yumin Qi wrote:
> HI, Zhengyu
> 
>    Looks good to me!
> 
> 
> Thanks
> 
> Yumin
> 
> On 7/7/20 10:13 AM, Zhengyu Gu wrote:
>> The change is no longer trivial, may I get a second review?
>>
>> Thanks,
>>
>> -Zhengyu
>>
>> On 7/7/20 9:34 AM, Thomas Stüfe wrote:
>>> Hi Zhengyu,
>>>
>>> okay, I get it now. Thank you. Reviewed from my side.
>>>
>>> Cheers, Thomas
>>>
>>> On Tue, Jul 7, 2020 at 1:53 PM Zhengyu Gu <zgu at redhat.com 
>>> <mailto:zgu at redhat.com>> wrote:
>>>
>>>     Hi Thomas,
>>>
>>>     On 7/4/20 1:35 AM, Thomas Stüfe wrote:
>>>      > Hi Zhengyu,
>>>      >
>>>      > sorry for the wait.
>>>
>>>     No problem.
>>>
>>>      >
>>>      >
>>>      > This looks good, but why did you remove the "!same region"
>>>     condition? I
>>>      > believe that is needed for the case when CDS' first mapping
>>>     encounters
>>>      > errors, so before it rebuilds CDS at another location it removes
>>>     (a) all
>>>      > mappings and then (b) the enclosing reservation. (a) should be
>>>     ignored
>>>      > by NMT but (b) should not. I may be wrong, I have had no 
>>> coffee yet.
>>>      > Cheers, Thomas
>>>
>>>     In new version, same region is handled in line #470
>>>
>>>        470   if (reserved_rgn->same_region(addr, size)) {
>>>        471     return remove_released_region(reserved_rgn);
>>>        472   }
>>>
>>>     so, !same region is always true.
>>>
>>>     Thanks,
>>>
>>>     -Zhengyu
>>>
>>>      >
>>>      >     Thanks,
>>>      >
>>>      >     -Zhengyu
>>>      >
>>>      >      >
>>>      >      > But if you are still unconvinced, I won't hold you up. The
>>>     change
>>>      >      > certainly works as it is now and is okay for me, it 
>>> just would
>>>      >     not be my
>>>      >      > preferred solution.
>>>      >      >
>>>      >      > Cheers, Thomas
>>>      >      >
>>>      >      >     Thanks,
>>>      >      >
>>>      >      >     -Zhengyu
>>>      >      >
>>>      >      >
>>>      >      >      >
>>>      >      >      >     Thanks, Thomas
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >     On Fri, Jun 26, 2020 at 10:54 PM Zhengyu Gu
>>>      >     <zgu at redhat.com <mailto:zgu at redhat.com>
>>>     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>
>>>      >      >     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>
>>>     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>>
>>>      >      >      >     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>
>>>     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>
>>>      >     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>
>>>     <mailto:zgu at redhat.com <mailto:zgu at redhat.com>>>>> wrote:
>>>      >      >      >
>>>      >      >      >         Hi,
>>>      >      >      >
>>>      >      >      >         Please review this trivial patch that fixes
>>>     summary
>>>      >      >     counting in
>>>      >      >      >  VirtualMemoryTracker::split_reserved_region().
>>>      >      >      >
>>>      >      >      >         The method uses internal method to remove a
>>>      >     reserved region,
>>>      >      >      >         which does
>>>      >      >      >         not update counting information. It 
>>> should use
>>>      >     high level
>>>      >      >     tracking
>>>      >      >      >         method
>>>     VirtualMemoryTracker::remove_released_region()
>>>      >      >     instead.
>>>      >      >      >
>>>      >      >      >         Without patch, NMT summary reports
>>>     uncategorized
>>>      >     memory, e.g.
>>>      >      >      >
>>>      >      >      >         -  Unknown
>>>     (reserved=1060416KB,
>>>      >      >     committed=0KB)
>>>      >      >      >    (mmap:
>>>      >     reserved=1060416KB,
>>>      >      >      >         committed=0KB)
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >         Bug:
>>>     https://bugs.openjdk.java.net/browse/JDK-8248426
>>>      >      >      >         Webrev:
>>>      >      > http://cr.openjdk.java.net/~zgu/JDK-8248426/webrev.00/
>>>      >      >      >
>>>      >      >      >         Test:
>>>      >      >      >             hotspot_nmt
>>>      >      >      >             Submit test in progress
>>>      >      >      >
>>>      >      >      >
>>>      >      >      >         Thanks,
>>>      >      >      >
>>>      >      >      >         -Zhengyu
>>>      >      >      >
>>>      >      >
>>>      >
>>>
>>