RFR(XS) 8191369: NMT: Enhance thread stack tracking

[yumin qi]

Hi, Zhenyu

  It looks good to me.  A minor suggest that  in
  test_threadstack_tracking.cpp: 74
  In fact you can move the
  i++ above into the if  block.
  Sine address of i only belongs to one committed region, if found the
region, should only increased one time.

Thanks
Yumin

On Fri, Feb 23, 2018 at 8:54 AM, Zhengyu Gu <zgu at redhat.com> wrote:

> Thanks, Coleen.
>
> -Zhengyu
>
>
> On 02/23/2018 11:52 AM, coleen.phillimore at oracle.com wrote:
>
>>
>> This looks good but I hope someone else can review the os_linux and
>> os_windows parts.
>>
>> I will sponsor this once the reviews are complete.
>>
>> thanks,
>> Coleen
>>
>> On 2/21/18 10:49 AM, Zhengyu Gu wrote:
>>
>>> Hi,
>>>
>>> I rebased the patch after JDK-8196405[1]. Stefan's patch fixed virtual
>>> memory tracking issues I tried to address in early patch.
>>>
>>>
>>> In additions, I removed os::pd_committed_stack_size() methods as Coleen
>>> suggested. Also added new gtest.
>>>
>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8191369
>>> Webrev: http://cr.openjdk.java.net/~zgu/8191369/webrev.03/
>>>
>>> Test:
>>>   hotspot_runtime, test-hotspot-gtest on Linux 64 with fastdebug and
>>> release builds.
>>>
>>>
>>> [1] http://cr.openjdk.java.net/~stefank/8196405/webrev.02/
>>>
>>> Thanks,
>>>
>>> -Zhengyu
>>>
>>>
>>> On 02/12/2018 09:33 AM, Zhengyu Gu wrote:
>>>
>>>>
>>>>
>>>>> No, what you have is better.   I dislike the delegation to the
>>>>> duplicate function signature in the pd_* layer.   Can you make the
>>>>> os::committed_stack_size() functions in the platforms where this isn't
>>>>> supported just be empy functions?
>>>>>
>>>> Okay.
>>>>
>>>>
>>>>> If there are platforms that don't have the ability to track committed
>>>>> regions of the mtThread stack, don't you still need the concept of
>>>>> all_committed?
>>>>>
>>>> No, it exists for convenience. However, stack tracking code does need
>>>> adjustment.
>>>>
>>>> Thanks,
>>>>
>>>> -Zhengyu
>>>>
>>>>
>>>>
>>>>> thanks,
>>>>> Coleen
>>>>>
>>>>>
>>>>>> Thanks,
>>>>>>
>>>>>> -Zhengyu
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>
>>>>>>> Thanks,
>>>>>>> Coleen
>>>>>>>
>>>>>>> On 1/11/18 4:05 PM, Zhengyu Gu wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> Can I get second (R)eviewer for this? and I also need a sponsor.
>>>>>>>>
>>>>>>>> This is JDK11 bug.
>>>>>>>>
>>>>>>>> Webrev: http://cr.openjdk.java.net/~zgu/8191369/webrev.02/
>>>>>>>> Bug: Bug: https://bugs.openjdk.java.net/browse/JDK-8191369
>>>>>>>>
>>>>>>>> Thanks,
>>>>>>>>
>>>>>>>> -Zhengyu
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 11/20/2017 01:46 PM, Zhengyu Gu wrote:
>>>>>>>>
>>>>>>>>> Hi Andrew,
>>>>>>>>>
>>>>>>>>> Thanks for the review.
>>>>>>>>>
>>>>>>>>> I'm not sure about the logic of the test when you hit the the
>>>>>>>>>>> ENOMEM/uncommited case.
>>>>>>>>>>>
>>>>>>>>>>> +    mincore_return_value = mincore(test_addr, page_sz, vec);
>>>>>>>>>>> +
>>>>>>>>>>> +    if (mincore_return_value == -1 || (committed_only &&
>>>>>>>>>>> (vec[0] &
>>>>>>>>>>> 0x01) == 0)) {
>>>>>>>>>>> +      // Page is not mapped/committed go up
>>>>>>>>>>> +      // to find first mapped/committed page
>>>>>>>>>>> +      if (errno != EAGAIN || (committed_only && (vec[0] & 0x01)
>>>>>>>>>>> == 0)) {
>>>>>>>>>>> +        assert(mincore_return_value != -1 || errno == ENOMEM,
>>>>>>>>>>> "Unexpected mincore errno");
>>>>>>>>>>>
>>>>>>>>>>> Should that not be
>>>>>>>>>>>
>>>>>>>>>>>    +      if ((mincore_return_value != -1 && errno != EAGAIN) ||
>>>>>>>>>>> (committed_only && (vec[0] & 0x01) == 0)) {
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Sorry, that correction should have been:
>>>>>>>>>>
>>>>>>>>>>    +      if ((mincore_return_value == -1 && errno != EAGAIN) ||
>>>>>>>>>>    (committed_only && (vec[0] & 0x01) == 0)) {
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Thanks for pointing out. Updated accordingly:
>>>>>>>>>
>>>>>>>>> Webrev: http://cr.openjdk.java.net/~zgu/8191369/webrev.02/
>>>>>>>>>
>>>>>>>>> -Zhengyu
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>> A successful call to mincore is not guaranteed to reset errno and
>>>>>>>>>>> it
>>>>>>>>>>> might by chance already have value EAGAIN before the call to
>>>>>>>>>>> mincore. If
>>>>>>>>>>> we happened to hit a mapped, committed page first time then the
>>>>>>>>>>> vec bit
>>>>>>>>>>> will be 1 and the code will loop and retest the same address.
>>>>>>>>>>> Unlikely
>>>>>>>>>>> but possible?
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> The adjustment to the edge case logic now looks correct.
>>>>>>>>>>>
>>>>>>>>>>> The test case for verifying fixes for above two issues can be
>>>>>>>>>>>> found
>>>>>>>>>>>> here: http://cr.openjdk.java.net/~zgu/8191369/test_mmap.c
>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Yes, that verifies the edge case handling e.g. I ran it with 256
>>>>>>>>>>> pages
>>>>>>>>>>> and with 127/8/9 mapped and it gave the right results.
>>>>>>>>>>>
>>>>>>>>>>> As mentioned in early email, new patch also contains
>>>>>>>>>>>> implementation for
>>>>>>>>>>>> Windows.
>>>>>>>>>>>>
>>>>>>>>>>>> Updated Webrev: http://cr.openjdk.java.net/~zg
>>>>>>>>>>>> u/8191369/webrev.01/
>>>>>>>>>>>>
>>>>>>>>>>>> Test:
>>>>>>>>>>>>
>>>>>>>>>>>>    Reran hotspot_tier1 tests on Windows x64 and Linux x64
>>>>>>>>>>>> (fastdebug and
>>>>>>>>>>>> release)
>>>>>>>>>>>>
>>>>>>>>>>> I have no idea about the validity of the Windows fix but the
>>>>>>>>>>> Linux one
>>>>>>>>>>> looks good modulo that if condition.
>>>>>>>>>>>
>>>>>>>>>>> regards,
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Andrew Dinn
>>>>>>>>>>> -----------
>>>>>>>>>>> Senior Principal Software Engineer
>>>>>>>>>>> Red Hat UK Ltd
>>>>>>>>>>> Registered in England and Wales under Company Registration No.
>>>>>>>>>>> 03798903
>>>>>>>>>>> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric
>>>>>>>>>>> Shander
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>
>>>>>
>>