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

[Volker Simonis]

Hi Thomas,

is there any progress with this issue?

Can we please have an "official" review for this tiny change?

Thank you and best regards,
Volker


On Fri, Oct 11, 2013 at 10:38 AM, Thomas Schatzl
<thomas.schatzl at oracle.com> 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