RFR: 8244551: Shenandoah: Fix racy update of update_watermark

[Aleksey Shipilev]

On 5/7/20 1:37 PM, 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

Looks fine. As long as it compiles and passes the tests :)


-- 
Thanks,
-Aleksey