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 07:51:22 UTC 2015
Hi Bill,
Thanks for looking at this!
On 2015-06-23 23:41, bill pittore 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.
I think you are right. I would need a loadload() in during_cycle(). But
with the enum solution suggested by Vitaly I think all of this ordering
goes away.
Thanks,
Bengt
>
> 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/20150624/5f806ef4/attachment.htm>
More information about the hotspot-gc-dev
mailing list