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

[Zhengyu Gu]

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/~zgu/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
>>>>>>>>
>>>>>>>
>>>>
>>