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

[Vitaly Davidovich]

Naive question - could this be converted into a numeric state field that
indicates the lifecycle (e.g 1 = in progress, 2 = started)?

sent from my phone
On Jun 23, 2015 5:42 PM, "bill pittore" <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/
>
> 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/20150623/8ab0c767/attachment.htm>