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

[bill pittore]

Hi Bengt,
  This seems to eliminate the TSO issues rather cleanly.  One small nit 
is that the comments in concurrentMarkThread.hpp and CollectedHeap.cpp 
talk about setting/clearing 'flags' whereas now it's setting a state 
variable.

bill

On 6/24/2015 3:28 AM, 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/
>
> 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/49163ef9/attachment.htm>