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

[Vitaly Davidovich]

Looks good Bengt (not a Reviewer though).

sent from my phone
On Jun 24, 2015 6:29 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com> wrote:

>
> 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
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>
>>>
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150624/0ddebf6f/attachment.htm>