Request for review (S): 8008382: Remove redundant use of Atomic::add(jlong, jlong *) in create_new_gc_id()

[Bengt Rutisson]

Thanks Erik, Stefan and David for the reviews!

And thanks Mikael for helping detecting the CMS race. Pushing this now.

Bengt

On 2/19/13 1:35 PM, Erik Helin wrote:
> Bengt,
>
> On 02/19/2013 11:15 AM, Bengt Rutisson wrote:
>> Erik Helin pointed out a potential race in CMS where the concurrent
>> cycle may be trying to increment the GC id in parallel with a young
>> collection.
>
> I just would like to add that Mikael Gerdin also investigated the 
> race, thanks Mikael!
>
> On 02/19/2013 11:15 AM, Bengt Rutisson wrote:
>> This is scary for other reasons as well but removing the
>> atomic call exposes it. We concluded that the safe change is to move the
>> call to register_gc_start() in to the CMS VM operation. This way we get
>> the VM operation synchronization and can be sure that two threads are
>> never incrementing the GC id in parallel.
>>
>> Updated webrev here:
>> http://cr.openjdk.java.net/~brutisso/8008382/webrev.01/
>
> Looks good!
>
> Thanks,
> Erik
>
>> Bengt
>>
>> On 2/18/13 2:21 PM, Bengt Rutisson wrote:
>>>
>>> Hi all,
>>>
>>> Could I have a couple of reviews for this change?
>>> http://cr.openjdk.java.net/~brutisso/8008382/webrev.00/
>>>
>>> There is no need to use atomics in create_new_gc_id() since it is not
>>> called by multiple threads in parallel. Also, Atomic::add(jlong, jlong
>>> *) is broken for ARM.
>>>
>>> We should remove the use of Atomic::add in create_new_gc_id() for now.
>>> If we need this to be called by multiple threads in the future we have
>>> to reconsider how this should be done. Either adding a lock or doing
>>> some kind of 32 bit atomic work.
>>>
>>> Thanks,
>>> Bengt
>>
>>
>