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

[Bengt Rutisson]

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/2c5238cb/attachment.htm>