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

[Kim Barrett]

On Mon, 5 Oct 2020 09:44:53 GMT, Thomas Schatzl <tschatzl 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

Marked as reviewed by kbarrett (Reviewer).

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

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