Hi Bengt,
This seems to eliminate the TSO issues rather cleanly. 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.
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: <http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/attachments/20150624/49163ef9/attachment.html>