RFR: 8244551: Shenandoah: Fix racy update of update_watermark

[Roman Kennke]

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, it is more work (appropriate
barriers), but that's ok. 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
>>>>
>>>
>>
>