8373468: Shenandoah: Mutator may block at _gc_waiters_lock after allocation failure even block parameter is false

src/hotspot/share/gc/shenandoah/shenandoahControlThread.cpp

@@ -66,10 +66,13 @@ void ShenandoahControlThread::run_service() {
     }
 
     // Figure out if we have pending requests.
-    const bool alloc_failure_pending = ShenandoahCollectorPolicy::is_allocation_failure(cancelled_cause);
     const bool is_gc_requested = _gc_requested.is_set();
-    const GCCause::Cause requested_gc_cause = _requested_gc_cause;
-
+    const GCCause::Cause requested_gc_cause = current_requested_gc_cause();
+    const bool alloc_failure_pending = ShenandoahCollectorPolicy::is_allocation_failure(cancelled_cause) ||
+                                       ShenandoahCollectorPolicy::is_allocation_failure(requested_gc_cause);
+    if (is_gc_requested) {
+      reset_requested_gc();
+    }
     // Choose which GC mode to run in. The block below should select a single mode.
     GCMode mode = none;
     GCCause::Cause cause = GCCause::_last_gc_cause;
@@ -169,8 +172,9 @@ void ShenandoahControlThread::run_service() {
         notify_gc_waiters();
       }
 
-      // If this cycle completed without being cancelled, notify waiters about it
-      if (!heap->cancelled_gc()) {
+      // If this cycle completed without being cancelled, or alloc_failure_pending is true(degen),
+      // notify waiters about it
+      if (!heap->cancelled_gc() || alloc_failure_pending) {
         notify_alloc_failure_waiters();
       }
 
@@ -230,9 +234,17 @@ void ShenandoahControlThread::run_service() {
       last_sleep_adjust_time = current;
     }
 
-    MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag);
-    ml.wait(sleep);
+    {
+      MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag);
+      if (current_requested_gc_cause() == GCCause::_no_gc) {
+        ml.wait(sleep);
+      }
+    }
   }
+
+  // In case any threads are waiting for a cycle to happen, notify them so they observe the shutdown.
+  notify_gc_waiters();
+  notify_alloc_failure_waiters();
 }
 
 void ShenandoahControlThread::service_concurrent_normal_cycle(GCCause::Cause cause) {
@@ -346,7 +358,8 @@ void ShenandoahControlThread::request_gc(GCCause::Cause cause) {
   }
 }
 
-void ShenandoahControlThread::notify_control_thread(GCCause::Cause cause) {
+void ShenandoahControlThread::notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation) {
+  assert(generation->is_global(), "Must be");
   // Although setting gc request is under _controller_lock, the read side (run_service())
   // does not take the lock. We need to enforce following order, so that read side sees
   // latest requested gc cause when the flag is set.
@@ -356,6 +369,10 @@ void ShenandoahControlThread::notify_control_thread(GCCause::Cause cause) {
   controller.notify();
 }
 
+void ShenandoahControlThread::notify_control_thread(GCCause::Cause cause) {
+  notify_control_thread(cause, ShenandoahHeap::heap()->global_generation());
+}
+
 void ShenandoahControlThread::handle_requested_gc(GCCause::Cause cause) {
   if (should_terminate()) {
     log_info(gc)("Control thread is terminating, no more GCs");
@@ -391,7 +408,22 @@ void ShenandoahControlThread::handle_requested_gc(GCCause::Cause cause) {
 }
 
 void ShenandoahControlThread::notify_gc_waiters() {
-  _gc_requested.unset();
   MonitorLocker ml(&_gc_waiters_lock);
   ml.notify_all();
 }
+
+GCCause::Cause ShenandoahControlThread::current_requested_gc_cause() {
+  if (_control_lock.owned_by_self()) return _requested_gc_cause;
+  {
+    MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag);
+    return _requested_gc_cause;
+  }
+}
+
+void ShenandoahControlThread::reset_requested_gc() {
+  {
+    MonitorLocker ml(&_control_lock, Mutex::_no_safepoint_check_flag);
+    _requested_gc_cause = GCCause::_no_gc;
+    _gc_requested.unset();
+  }
+}

src/hotspot/share/gc/shenandoah/shenandoahControlThread.hpp

@@ -41,8 +41,8 @@ class ShenandoahControlThread: public ShenandoahController {
     stw_full
   } GCMode;
 
-  ShenandoahSharedFlag _gc_requested;
-  GCCause::Cause       _requested_gc_cause;
+  ShenandoahSharedFlag               _gc_requested;
+  GCCause::Cause                     _requested_gc_cause;
   ShenandoahGC::ShenandoahDegenPoint _degen_point;
 
   // This lock is used to coordinate waking up the control thread
@@ -55,6 +55,8 @@ class ShenandoahControlThread: public ShenandoahController {
   void stop_service() override;
 
   void request_gc(GCCause::Cause cause) override;
+  // Sets the requested cause and flag and notifies the control thread
+  void notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation) override;
 
 private:
   // Sets the requested cause and flag and notifies the control thread
@@ -70,6 +72,10 @@ class ShenandoahControlThread: public ShenandoahController {
   // Handle GC request.
   // Blocks until GC is over.
   void handle_requested_gc(GCCause::Cause cause);
+
+  GCCause::Cause current_requested_gc_cause();
+
+  void reset_requested_gc();
 };
 
 #endif // SHARE_GC_SHENANDOAH_SHENANDOAHCONTROLTHREAD_HPP

src/hotspot/share/gc/shenandoah/shenandoahController.cpp

@@ -47,7 +47,7 @@ void ShenandoahController::handle_alloc_failure(const ShenandoahAllocRequest& re
   ShenandoahHeap* const heap = ShenandoahHeap::heap();
   if (heap->cancel_gc(cause)) {
     log_info(gc)("Failed to allocate %s, " PROPERFMT, req.type_string(), PROPERFMTARGS(req.size() * HeapWordSize));
-    request_gc(cause);
+    notify_control_thread(cause, heap->mode()->is_generational() ? reinterpret_cast<ShenandoahGeneration *>(heap->young_generation()) : heap->global_generation());
   }
 
   if (block) {

src/hotspot/share/gc/shenandoah/shenandoahController.hpp

@@ -30,6 +30,8 @@
 #include "gc/shenandoah/shenandoahAllocRequest.hpp"
 #include "gc/shenandoah/shenandoahSharedVariables.hpp"
 
+class ShenandoahGeneration;
+
 /**
  * This interface exposes methods necessary for the heap to interact
  * with the threads responsible for driving the collection cycle.
@@ -62,6 +64,9 @@ class ShenandoahController: public ConcurrentGCThread {
   // like System.gc and "implicit" gc requests, like metaspace oom.
   virtual void request_gc(GCCause::Cause cause) = 0;
 
+ // Sets the requested cause and flag and notifies the control thread
+  virtual void notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation) { }
+
   // This cancels the collection cycle and has an option to block
   // until another cycle completes successfully.
   void handle_alloc_failure(const ShenandoahAllocRequest& req, bool block);

src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.cpp

@@ -670,8 +670,9 @@ bool ShenandoahGenerationalControlThread::request_concurrent_gc(ShenandoahGenera
   if (gc_mode() == none) {
     const size_t current_gc_id = get_gc_id();
     while (gc_mode() == none && current_gc_id == get_gc_id()) {
-      if (_requested_gc_cause != GCCause::_no_gc) {
-        log_debug(gc, thread)("Reject request for concurrent gc because another gc is pending: %s", GCCause::to_string(_requested_gc_cause));
+      GCCause::Cause requested_gc_cause = _requested_gc_cause;
+      if (requested_gc_cause != GCCause::_no_gc) {
+        log_debug(gc, thread)("Reject request for concurrent gc because another gc is pending: %s", GCCause::to_string(requested_gc_cause));
         return false;
       }
 

src/hotspot/share/gc/shenandoah/shenandoahGenerationalControlThread.hpp

@@ -63,7 +63,7 @@ class ShenandoahGenerationalControlThread: public ShenandoahController {
 
   // Represents a normal (non cancellation) gc request. This can be set by mutators (System.gc,
   // whitebox gc, etc.) or by the regulator thread when the heuristics want to start a cycle.
-  GCCause::Cause  _requested_gc_cause;
+  GCCause::Cause _requested_gc_cause;
 
   // This is the generation the request should operate on.
   ShenandoahGeneration* _requested_generation;
@@ -137,7 +137,7 @@ class ShenandoahGenerationalControlThread: public ShenandoahController {
 
   // Takes the request lock and updates the requested cause and generation, then notifies the control thread.
   // The overloaded variant should be used when the _control_lock is already held.
-  void notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation);
+  void notify_control_thread(GCCause::Cause cause, ShenandoahGeneration* generation) override;
   void notify_control_thread(MonitorLocker& ml, GCCause::Cause cause, ShenandoahGeneration* generation);
 
   // Notifies the control thread, but does not update the requested cause or generation.