[14] RFR: 8235982: AArch64: gc/stress/jfr/TestStressAllocationGCEventsWithParallel.java assertion failure

[Thomas Schatzl]

Hi,

On 17.12.19 14:53, Stefan Johansson wrote:
> Hi Nick,
> 
> Thanks for fixing this issue.

   yes :)

> 
> On 2019-12-17 07:22, Nick Gasson wrote:
>> Hi,
>>
>> Bug: https://bugs.openjdk.java.net/browse/JDK-8235982
>> Webrev: http://cr.openjdk.java.net/~ngasson/8235982/webrev.01/
>>
>> The above test has been intermittently failing with an assertion error 
>> on AArch64 since the changes in "8220465: Use shadow regions for 
>> faster ParallelGC full GCs".
>>
>> PSParallelCompact::decrement_destination_counts assumes that 
>> shadow_region() is valid if mark_copied() succeeded (i.e. 
>> _shadow_state was FilledShadow before). 
>> MoveAndUpdateShadowClosure::complete_region sets these with the 
>> following code:
>>
>>    // Record the shadow region index
>>    region_ptr->set_shadow_region(_shadow);
>>    // Mark the shadow region as filled to indicate the data is ready 
>> to be
>>    // copied back
>>    region_ptr->mark_filled();
>>
>> set_shadow_region() does an ordinary store to a volatile variable and 
>> mark_filled() uses a CAS to set _shadow_state to FilledShadow with 
>> relaxed memory ordering. On a non-TSO architecture like AArch64 these 
>> two stores may be reordered when observed from another thread.
>>
>> Fix by making the CAS to set _shadow_state use the default 
>> conservative memory order which inserts a full barrier (see the 
>> discussion on the JBS entry).
> I agree with the conclusions in the JBS entry and this fix looks good,
> StefanJ
> 

Looks good. If you need a sponsor to push the change I could do that.

Thomas