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

[Bengt Rutisson]

On 2015-06-24 11:27, Bertrand Delsart wrote:
> Looks good... and much simpler :-)

Thanks, Bertrand!

Bengt

>
> Bertrand.
>
> On 24/06/2015 10:31, Bengt Rutisson wrote:
>>
>>
>> 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
>>>>>>>
>>>>>>
>>>>
>>
>
>