RFR: 8323630: GenShen: Control thread may (still) ignore requests to start concurrent GC

[Y. Srinivas Ramakrishna]

On Fri, 12 Jan 2024 00:24:36 GMT, William Kemper <wkemper at openjdk.org> wrote:

> A race condition exists in which the control thread may clear the `_requested_gc_cause` immediately after a mutator requests an explicit gc. When this happens, the control thread will no longer accept requests from the regulator to start concurrent GC cycles. The mutator thread will never wakeup, eventually the application will run out of memory or no progress will be made.
> 
> The change here is intended to simplify the thread communication protocol by reducing the number of variables in play.

Looks good; some minor comments & suggestions. I wonder if the xchg and cmpxchg couldn't just be replaced with a condition variable protected by a lock, and that might simplify the code some more. But this looks fine.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 119:

> 117:     // Figure out if we have pending requests.
> 118:     bool alloc_failure_pending = _alloc_failure_gc.is_set();
> 119:     bool humongous_alloc_failure_pending = _humongous_alloc_failure_gc.is_set();

const them.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 123:

> 121: 
> 122:     bool explicit_gc_requested = is_explicit_gc(cause);
> 123:     bool implicit_gc_requested = is_implicit_gc(cause);

const them all.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 247:

> 245:     }
> 246: 
> 247:     bool gc_requested = (gc_mode() != none);

const the variable.

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 253:

> 251:       // Blow away all soft references on this cycle, if handling allocation failure,
> 252:       // either implicit or explicit GC request, or we are requested to do so unconditionally.
> 253:       if (generation == select_global_generation() && (alloc_failure_pending || implicit_gc_requested || explicit_gc_requested || ShenandoahAlwaysClearSoftRefs)) {

Not a change you made, but why do we clear all soft refs if it's an implicit gc? Is memory pressure supposed to be very high here?

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp line 388:

> 386:     }
> 387: 
> 388:     // Don't wait if there was an allocation failure or another request was made mid-cycle.

I'd rewrite the comment as "Wait for ShenandoahControlIntervalMax, unless there was an allocation failure or ..."

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

Marked as reviewed by ysr (Committer).

PR Review: https://git.openjdk.org/shenandoah/pull/382#pullrequestreview-1817248593
PR Review Comment: https://git.openjdk.org/shenandoah/pull/382#discussion_r1449702883
PR Review Comment: https://git.openjdk.org/shenandoah/pull/382#discussion_r1449698171
PR Review Comment: https://git.openjdk.org/shenandoah/pull/382#discussion_r1449699783
PR Review Comment: https://git.openjdk.org/shenandoah/pull/382#discussion_r1449704194
PR Review Comment: https://git.openjdk.org/shenandoah/pull/382#discussion_r1449705839