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

[Igor Veresov]

Yup. Looks good.

igor

On Oct 23, 2013, at 12:13 AM, Volker Simonis <volker.simonis at gmail.com> wrote:

> 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