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

[Bertrand Delsart]

Looks good... and much simpler :-)

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
>>>>>>
>>>>>
>>>
>


-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38330 Montbonnot Saint Martin,                             FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~