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

[Zhengyu Gu]

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