RFR: JDK-8129626: G1: set_in_progress() and clear_started() needs a barrier on non-TSO platforms
Bengt Rutisson
bengt.rutisson at oracle.com
Thu Jun 25 06:11:33 UTC 2015
Hi Bill,
On 2015-06-24 16:58, bill pittore wrote:
> Hi Bengt,
> This seems to eliminate the TSO issues rather cleanly.
Thanks for looking at this!
> One small nit is that the comments in concurrentMarkThread.hpp and
> CollectedHeap.cpp talk about setting/clearing 'flags' whereas now it's
> setting a state variable.
Agreed. I've updated the comment this way:
diff --git a/src/share/vm/gc/g1/concurrentMarkThread.hpp
b/src/share/vm/gc/g1/concurrentMarkThread.hpp
--- a/src/share/vm/gc/g1/concurrentMarkThread.hpp
+++ b/src/share/vm/gc/g1/concurrentMarkThread.hpp
@@ -81,11 +81,11 @@
void set_in_progress() { assert(_state == Started, "must be
starting a cycle"); _state = InProgress; }
bool in_progress() { return _state == InProgress; }
- // This flag returns true from the moment a marking cycle is
+ // Returns true from the moment a marking cycle is
// initiated (during the initial-mark pause when started() is set)
// to the moment when the cycle completes (just after the next
// marking bitmap has been cleared and in_progress() is
- // cleared). While this flag is true we will not start another cycle
+ // cleared). While during_cycle() is true we will not start another cycle
// so that cycles do not overlap. We cannot use just in_progress()
// as the CM thread might take some time to wake up before noticing
// that started() is set and set in_progress().
I'll push my change with this this updated comment.
Thanks again for reviewing this!
Bengt
>
> bill
>
> On 6/24/2015 3:28 AM, 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/
>>
>> 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/20150625/5632113b/attachment.htm>
More information about the hotspot-gc-dev
mailing list