RFR(XS) 8191369: NMT: Enhance thread stack tracking
yumin qi
yumin.qi at gmail.com
Wed Feb 28 16:50:45 UTC 2018
Then it is OK to me too.
Thanks
YUmin
On Wed, Feb 28, 2018 at 5:08 AM, Zhengyu Gu <zgu at redhat.com> wrote:
> Hi Yumin,
>
> Thanks for reviewing.
>
> On 02/27/2018 11:05 PM, yumin qi wrote:
>
>> 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.
>>
> This is done on purpose, to verify that snapshot only finds one committed
> region here, which is mapped stack segment.
>
> Thanks,
>
> -Zhengyu
>
>
>> Thanks
>> Yumin
>>
>> On Fri, Feb 23, 2018 at 8:54 AM, Zhengyu Gu <zgu at redhat.com <mailto:
>> zgu at redhat.com>> wrote:
>>
>> Thanks, Coleen.
>>
>> -Zhengyu
>>
>>
>> On 02/23/2018 11:52 AM, coleen.phillimore at oracle.com
>> <mailto: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
>> <https://bugs.openjdk.java.net/browse/JDK-8191369>
>> Webrev: http://cr.openjdk.java.net/~zgu/8191369/webrev.03/
>> <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/
>> <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/~zg
>> u/8191369/webrev.02/
>> <http://cr.openjdk.java.net/~z
>> gu/8191369/webrev.02/>
>> Bug: Bug:
>> https://bugs.openjdk.java.net/
>> browse/JDK-8191369
>> <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/~zg
>> u/8191369/webrev.02/
>> <http://cr.openjdk.java.net/~z
>> gu/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
>> <
>> 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/
>> <
>> 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
>>
>>
>>
>>
>>
>>
>>
More information about the hotspot-runtime-dev
mailing list