[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(XS) 8191369: NMT: Enhance thread stack tracking
From:       Zhengyu Gu <zgu () redhat ! com>
Date:       2018-01-11 21:05:38
Message-ID: 93767041-ef59-4c80-681b-28bcf4aca597 () redhat ! com
[Download RAW message or body]

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
>>>
>>
[prev in list] [next in list] [prev in thread] [next in thread]