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: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20150624/02655710/attachment.htm>
More information about the hotspot-gc-dev
mailing list