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

List:       openjdk-hotspot-runtime-dev
Subject:    Re: RFR(XS) 8234539 ArchiveRelocationTest.java failed: Archive mapping should always succeed
From:       Calvin Cheung <calvin.cheung () oracle ! com>
Date:       2019-11-26 19:12:02
Message-ID: e4743fc8-10b0-f7e8-c208-4d6e01084990 () oracle ! com
[Download RAW message or body]


On 11/25/19 3:43 PM, Ioi Lam wrote:
> Hi Calvin,
>
> Thanks for the review.
>
> On 11/25/19 3:22 PM, Calvin Cheung wrote:
>> Hi Ioi,
>>
>> This seems good.
>>
>> Just wondering are the following 'if' checks necessary in 
>> metaspaceShared.cpp?
>>
>> 2155       if (static_result == MAP_ARCHIVE_SUCCESS) {
>> 2156         static_result = MAP_ARCHIVE_MMAP_FAILURE;
>> 2157       }
>> 2158       if (dynamic_result == MAP_ARCHIVE_SUCCESS) {
>> 2159         dynamic_result = MAP_ARCHIVE_MMAP_FAILURE;
>> 2160       }
>>
>
> The checks for (static_result == MAP_ARCHIVE_SUCCESS) is to make sure 
> we aren't in the MAP_ARCHIVE_OTHER_FAILURE state, which could happen 
> if archive CRC check failed, classpath validation failed, etc.
>
>
>> The checks weren't there in filemap.cpp. 
>
> The the old code (removed by this patch) was at a point that no error 
> has appended, so we are implicitly in the MAP_ARCHIVE_SUCCESS state.
>
>> Also, the caller won't try map_archives() again if the result is not 
>> MAP_ARCHIVE_MMAP_FAILURE.
>
> That's the intended behavior. If CRC check has failed, for example, 
> even if we retry mapping, we will get the same failure again.

Thanks for the explanations.

Looks good.

thanks,

Calvin

>
> Thanks
> - Ioi
>
>>
>> thanks,
>>
>> Calvin
>>
>> On 11/22/19 5:46 PM, Ioi Lam wrote:
>>> Hi Calvin,
>>>
>>> Thanks for the review. It turned out that I needed to fix another 
>>> (addr_delta == 0) bug in the code. I've also moved the handling of 
>>> ArchiveRelocationMode==1 in debug builds to 
>>> MetaspaceShared::map_archives(). This way, we can simulate the 
>>> "mapping failure" after all archives have been mapped. This way, we 
>>> can better test the code that unmap the archives after the initial 
>>> mapping failures.
>>>
>>> Here's the updated patch.
>>> http://cr.openjdk.java.net/~iklam/jdk14/8234539-mapping-should-always-succeed.v02/ 
>>>
>>>
>>> I am running tier4-rt-cds-relocation multiple times to make sure 
>>> 8234539 is no longer triggered on Windows.
>>>
>>> Thanks
>>> - Ioi
>>>
>>> On 11/22/2019 11:23 AM, Calvin Cheung wrote:
>>>> Hi Ioi,
>>>>
>>>> The fix looks good.
>>>>
>>>> thanks,
>>>>
>>>> Calvin
>>>>
>>>> On 11/21/19 2:58 PM, Ioi Lam wrote:
>>>>> https://bugs.openjdk.java.net/browse/JDK-8234539
>>>>> http://cr.openjdk.java.net/~iklam/jdk14/8234539-mapping-should-always-succeed.v01/ 
>>>>>
>>>>>
>>>>> This bug happens only on Windows. The fix is one-line -- in order 
>>>>> to check
>>>>> whether "This is the second time we try to map the archive(s)", 
>>>>> instead of
>>>>> using (addr_delta != 0), the correct condition is 
>>>>> (rs.is_reserved()). Please
>>>>> see the bug report for details.
>>>>>
>>>>> I also improve the log messages when error happens.
>>>>>
>>>>> Thanks
>>>>> - Ioi
>>>
>
[prev in list] [next in list] [prev in thread] [next in thread]