RFR: JDK-8129626: G1: set_in_progress() and clear_started() needs a barrier on non-TSO platforms

[Bengt Rutisson]

On 2015-06-24 10:34, Per Liden wrote:
> On 2015-06-24 10:15, Bengt Rutisson wrote:
>>
>> Hi Per,
>>
>> Thanks for looking at this!
>>
>> On 2015-06-24 10:09, Per Liden wrote:
>>> Hi Bengt,
>>>
>>> On 2015-06-24 09:28, Bengt Rutisson wrote:
>>>>
>>>>
>>>> Hi Vitaly,
>>>>
>>>> On 2015-06-23 23:53, Vitaly Davidovich wrote:
>>>>>
>>>>> Naive question - could this be converted into a numeric state field
>>>>> that indicates the lifecycle (e.g 1 = in progress, 2 = started)?
>>>>>
>>>>
>>>> Good question! That's a much simpler and more stable solution.
>>>>
>>>> New webrev:
>>>> http://cr.openjdk.java.net/~brutisso/8129626/webrev.02/
>>>
>>> This looks much nicer, and I can't think of any reason why that
>>> wouldn't work. One little request though, can we name the states to
>>> match the function names, like Idle/Started/InProgress? And
>>> clear_in_progress() should probably be set_idle() to align with the 
>>> rest.
>>
>> Yes, I was struggling a bit with the naming. What do you think about 
>> this?
>>
>> cr.openjdk.java.net/~brutisso/8129626/webrev.03/
>
> Looks good!

Thanks Per!

Bengt

>
> cheers,
> /Per
>
>>
>> Thanks,
>> Bengt
>>>
>>> cheers,
>>> /Per
>>>
>>>>
>>>> Thanks,
>>>> Bengt
>>>>
>>>>
>>>>
>>>>> sent from my phone
>>>>>
>>>>> On Jun 23, 2015 5:42 PM, "bill pittore" <bill.pittore at oracle.com
>>>>> <mailto:bill.pittore at oracle.com>> wrote:
>>>>>
>>>>>     Generally when you have a storestore on the write side you need a
>>>>>     loadload on the read side to prevent the second read from 
>>>>> floating
>>>>>     above the first one. The reading thread could read in_progress as
>>>>>     0 before it reads starting. Meanwhile the write thread writes 
>>>>> 1 to
>>>>>     in_progress, issues storestore, clears starting. Reading thread
>>>>>     then reads starting as 0. I don't know if the CGC mutex somehow
>>>>>     eliminates this issue as I'm not familiar with the code in 
>>>>> detail.
>>>>>
>>>>>     bill
>>>>>
>>>>>     On 6/23/2015 4:25 PM, Bengt Rutisson wrote:
>>>>>>
>>>>>>     Hi everyone,
>>>>>>
>>>>>>     Could I have a couple of reviews for this change?
>>>>>>
>>>>>>     https://bugs.openjdk.java.net/browse/JDK-8129626
>>>>>> http://cr.openjdk.java.net/~brutisso/8129626/webrev.00/
>>>>>> <http://cr.openjdk.java.net/%7Ebrutisso/8129626/webrev.00/>
>>>>>>
>>>>>>     We need to add a barrier between the calls to set_in_progress()
>>>>>>     and clear_started() to make sure that other threads sees the
>>>>>>     correct value when they use 
>>>>>> ConcurrentMarkThread::during_cycle().
>>>>>>
>>>>>>     Thanks to Per and Bertrand for helping out identifying and
>>>>>>     sorting this out.
>>>>>>
>>>>>>     From the bug report:
>>>>>>
>>>>>>     ConcurrentMarkThread::during_cycle() is implemented as:
>>>>>>
>>>>>>     bool during_cycle() { return started() || in_progress(); }
>>>>>>
>>>>>>     So, it checks both ConcurrentMarkThread::_started and
>>>>>>     ConcurrentMarkThread::_in_progress and they are meant to 
>>>>>> overlap.
>>>>>>     That is, we should not set _started to false until after we have
>>>>>>     set _in_progress to true. This is done in 
>>>>>> sleepBeforeNextCycle():
>>>>>>
>>>>>>     void ConcurrentMarkThread::sleepBeforeNextCycle() {
>>>>>>       // We join here because we don't want to do the
>>>>>>     "shouldConcurrentMark()"
>>>>>>       // below while the world is otherwise stopped.
>>>>>>       assert(!in_progress(), "should have been cleared");
>>>>>>
>>>>>>       MutexLockerEx x(CGC_lock, Mutex::_no_safepoint_check_flag);
>>>>>>       while (!started() && !_should_terminate) {
>>>>>> CGC_lock->wait(Mutex::_no_safepoint_check_flag);
>>>>>>       }
>>>>>>
>>>>>>       if (started()) {
>>>>>>         set_in_progress();
>>>>>>         clear_started();
>>>>>>       }
>>>>>>     }
>>>>>>
>>>>>>     On non-TSO platforms there is a risk that the write to
>>>>>>     _in_progress (from set_in_progress()) is seen by other threads
>>>>>>     after the write to _started (in clear_started()). In that case
>>>>>>     there is a window when during_cycle() may return false even
>>>>>>     though we are in a concurrent cycle.
>>>>>>
>>>>>>
>>>>>>     Thanks,
>>>>>>     Bengt
>>>>>
>>>>
>>