RFR (S): 8147087: Race when reusing PerRegionTable bitmaps may result in dropped remembered set entries

[Mikael Gerdin]

Hi,

On 2016-01-20 11:47, Thomas Schatzl wrote:
> Hi Mikael,
>
> On Tue, 2016-01-19 at 17:47 +0100, Mikael Gerdin wrote:
>> Hi Thomas,
>>
>> On 2016-01-19 17:09, Thomas Schatzl wrote:
>>> Hi all,
>>>
> [...]
>>> The solution is to delay publishing the value of PRT::_hr after the
>>> bitmap clearing, i.e.
>>>
>>> void PerRegionTable::init(HeapRegion* hr, bool
>>> clear_links_to_all_list)
>>>      {
>>>        if (clear_links_to_all_list) {
>>>          set_next(NULL);
>>>          set_prev(NULL);
>>>        }
>>> -    _hr = hr;
>>>        _collision_list_next =
>>> NULL;
>>>        _occupied = 0;
>>>        _bm.clear();
>>> +    // Make sure that the
>>> bitmap clearing above has been finished before publishing
>>> +    // this
>>> PRT to concurrent threads.
>>> +    OrderAccess::release_store_ptr(&_hr,
>>> hr);
>>>      }
>>>
>>> Which is the whole relevant change. :)
>>
>> Shouldn't the corresponding load of _hr in the ::hr() accessor be a
>> load_acquire to mirror the release semantics on the store?
>> Otherwise, even if the stores are performed in the correct order a
>> load-reordering machine could see an inconsistent state and
>> experience issues with missing remembered set entries.
>
> All code following the use of hr() seems load-dependent (dereferencing
> _hr), so ARM* should be okay.
>
> You are right though, and I might have missed something (also something
> about what this load-dependency means), I added this in
>
> http://cr.openjdk.java.net/~tschatzl/8147087/webrev.1_to_2/
> http://cr.openjdk.java.net/~tschatzl/8147087/webrev.2/

Looks good!
/Mikael

>
> Thanks for the review,
>    Thomas
>