RFR: 8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure

[Thomas Schatzl]

On Mon, 5 Oct 2020 13:19:00 GMT, Stefan Johansson <sjohanss at openjdk.org> wrote:

>> Hi all,
>> 
>>   can I have reviews for this change that fixes a benign race between giving the signal to start a new concurrent cycle
>>   and G1ConcurrentMarkThread waiting for that signal?
>> 
>> With the changes from JDK-8240556 there is a small window where after the concurrent mark thread sets itself to idle
>> (at the end of the `while (wait_for_next_cycle())` loop in `G1ConcurrentMarkThread::run_service()`), a GC pause may
>> request a new gc, setting the `in-progress` flag, and the concurrent mark thread waiting for a new cycle in
>> `wait_for_next_cycle()` where this assert fails.  This ordering can be observed in the log message output:
>> [342.714s][info ][gc] GC(5240) Concurrent Mark Cycle
>> [342.979s][info ][gc] GC(5240) Pause Remark 75M->75M(132M) 171.410ms
>> [343.059s][info ][gc] GC(5240) Pause Cleanup 122M->122M(132M) 0.174ms
>> [343.075s][info ][gc] GC(5241) Pause Young (Concurrent Start) (G1 Humongous Allocation) 123M->123M(132M) 14.023ms <---
>> sets in-pogress flag [343.078s][info ][gc] GC(5240) Concurrent Mark Cycle 363.855ms <--- occurs before the next
>> wait_for_next_cycle() call
>> Previously this problem with the assert did not occur because there has been another "pre-in-progress" state
>> implemented in a somewhat complicated way with nested states. JDK-8240556 removed that one. The messages could be out
>> of order before that change too.  The suggested fix is to remove this assert.
>> 
>> Testing: a few thousand times for that test (not to try to observe the assert which is gone now, but double-check if
>> there is any deadlock etc hiding), tier1-5
>
> Looks good.

Thanks for your reviews @kimbarrett @kstefanj

-------------

PR: https://git.openjdk.java.net/jdk/pull/501