RFR: 8244551: Shenandoah: Fix racy update of update_watermark

[Zhengyu Gu]

On 5/7/20 9:12 AM, Roman Kennke wrote:
> Atomic::load/store doesn't do anything wrt to ordering, it only ensures
> atomicity.
> 
> Allocating thread would first update _top, and then _update_watermark,
> however, an observing thread that doesn't take the lock can observe the
> values updated in reverse order. By doing load_acquire() and
> release_store() we prevent that. 

Yes, I understand that.

Yes, it is more work (appropriate
> barriers), but that's ok. 

Okay, then. Looks good to me.

-Zhengyu

We should not see stale values anymore. I've
> run the test many times in a row without a failure, and it failed fairly
> reliably before.
> 
> Roman
> 
>> I did not follow this _update_watermark stuff closely. My experiment
>> with Atomic::load/store(_update_watermark), I still saw stalled values.
>> So I am not sure load_acquire/release_store() fix the stalled value
>> problem, or stalled value just means more work, but not correctness?
>>
>> -Zhengyu
>>
>> On 5/7/20 8:48 AM, Roman Kennke wrote:
>>>> So, the patch fixes the ordering of _top and _update_watermark to avoid
>>>> assertion failure, but adds unnecessary penalty to production build?
>>>
>>> I wouldn't call that unnecessary. And the cost of it seems ok too, we
>>> don't update so very often, and we don't query it so very often either.
>>> I made the update-at-safepoint not use barriers, so that critical path
>>> should be ok.
>>>
>>> Roman
>>>
>>>
>>>> -Zhengyu
>>>>
>>>>
>>>> On 5/7/20 7:37 AM, Roman Kennke wrote:
>>>>>>>> *) What's the point of this cast?
>>>>>>>>
>>>>>>>>     133   *const_cast<HeapWord**>(&_update_watermark) =  w;
>>>>>>>
>>>>>>> Make access non-volatile. You think it's too much?
>>>>>>
>>>>>> I don't think that is relevant. As long as it is not the atomic store
>>>>>> (with fences), we are fine.
>>>>>
>>>>> Ok. I will push the fix with this additional change then:
>>>>>
>>>>> diff -r 7910475d5001
>>>>> src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp
>>>>> --- a/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp
>>>>> Thu May 07 07:03:25 2020 -0400
>>>>> +++ b/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.inline.hpp
>>>>> Thu May 07 07:36:26 2020 -0400
>>>>> @@ -128,9 +128,8 @@
>>>>>     // Fast version that avoids synchronization, only to be used at
>>>>> safepoints.
>>>>>     inline void
>>>>> ShenandoahHeapRegion::set_update_watermark_at_safepoint(HeapWord* w) {
>>>>>       assert(bottom() <= w && w <= top(), "within bounds");
>>>>> -  assert(SafepointSynchronize::is_at_safepoint(),
>>>>> -         "only call set_update_watermark_at_safepoint at safepoint");
>>>>> -  *const_cast<HeapWord**>(&_update_watermark) =  w;
>>>>> +  assert(SafepointSynchronize::is_at_safepoint(), "Should be at
>>>>> Shenandoah safepoint");
>>>>> +  _update_watermark = w;
>>>>>     }
>>>>>
>>>>>     #endif // SHARE_GC_SHENANDOAH_SHENANDOAHHEAPREGION_INLINE_HPP
>>>>>
>>>>>
>>>>> Thanks for reviewing!
>>>>> Roman
>>>>>
>>>>
>>>
>>
>