RFR (XS): JDK-8025728 Missing volatile specifier for field G1AllocRegion::_alloc_region

[Axel Siebenborn]

Hi Thomas,
changing G1AllocRegion::_alloc_region to be a volatile pointer solves our problem with the xlC compiler.
   
I agree, that the change in G1AllocRegion::get() is necessary too.

The change looks good.

Thanks,
Axel

On 11.10.2013 10:38, Thomas Schatzl wrote:

> Hi all,
>
>    can I have review for the following small change? It changes G1AllocRegion::_alloc_region to be a volatile pointer, to avoid compiler optimizations that defeat the intent of the code.
>
> I.e. the code loads _alloc_region, and if it's not NULL uses it further; the compiler may reload the variable after the NULL check, leading to races with other code that changes it to NULL in the meantime.
> (The race is not a problem, the code just needs a consistent view on the value of the variable).
>
> E.g. in G1CollectedHeap::max_unsafe_tlab_alloc()
>
>    HeapRegion* hr = _mutator_alloc_region.get();
>    if (hr == NULL) {
>      return 0;
>    }
>    return hr->free();
>
> At the same time G1AllocRegion::retire(bool fill_up) may change _alloc_region.
>
> The fix is to make sure that the compiler does not reload the value of _alloc_region by making it volatile.
>
> This analysis and the suggested fix has been proposed from the original bug reporters.
>
> There is one catch with doing only this: G1AllocRegion::get() also contains a similar race where it is possible (and with that change guaranteed) that the code reloads the value.
> This is even more serious, as this would make the code return an internal HeapRegion instance. This screws up some internal logic.
>
> Bug:
> https://bugs.openjdk.java.net/browse/JDK-8025728
>
> Webrev:
> http://cr.openjdk.java.net/~tschatzl/8025728/webrev
>
> Testing:
> jprt
>
> @Volker: can you try the change? We do not have the compiler/platform to check.
>
> Thanks,
> Thomas