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

Bengt Rutisson bengt.rutisson at oracle.com
Wed Jun 24 13:55:02 UTC 2015


On 24/06/15 13:39, Vitaly Davidovich wrote:
>
> Looks good Bengt (not a Reviewer though).
>

Thanks, Vitaly!

Bengt

> sent from my phone
>
> On Jun 24, 2015 6:29 AM, "Bengt Rutisson" <bengt.rutisson at oracle.com 
> <mailto: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/
>                             <http://cr.openjdk.java.net/%7Ebrutisso/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/
>                     <http://cr.openjdk.java.net/%7Ebrutisso/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>
>                                 <mailto: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/>
>                                     <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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20150624/02655710/attachment-0001.html>


More information about the hotspot-gc-dev mailing list