From 8e53927b6739a935dc833f7e9527dacb71bae1a8 Mon Sep 17 00:00:00 2001
Date: Thu, 30 May 2024 07:02:40 +0000
Subject: [PATCH] [Backport]8210706: G1 may deadlock when starting a concurrent cycle at
 shutdown
---
 .../gc_implementation/g1/concurrentMark.cpp   | 20 +++++++++++--------
 .../gc_implementation/g1/concurrentMark.hpp   |  4 ++++
 .../g1/concurrentMarkThread.cpp               |  8 +++++++-
 .../gc_implementation/g1/g1CollectedHeap.cpp  | 14 +++++++------
 .../shared/concurrentGCThread.hpp             |  2 ++
 5 files changed, 33 insertions(+), 15 deletions(-)

diff --git a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp
index 1347a7e16..cad474b83 100644
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp
@@ -477,6 +477,16 @@ HeapRegion* CMRootRegions::claim_next() {
   return res;
 }
 
+void CMRootRegions::notify_scan_done() {
+  MutexLockerEx x(RootRegionScan_lock, Mutex::_no_safepoint_check_flag);
+  _scan_in_progress = false;
+  RootRegionScan_lock->notify_all();
+}
+
+void CMRootRegions::cancel_scan() {
+  notify_scan_done();
+}
+
 void CMRootRegions::scan_finished() {
   assert(scan_in_progress(), "pre-condition");
 
@@ -486,11 +496,7 @@ void CMRootRegions::scan_finished() {
   }
   _next_survivor = NULL;
 
-  {
-    MutexLockerEx x(RootRegionScan_lock, Mutex::_no_safepoint_check_flag);
-    _scan_in_progress = false;
-    RootRegionScan_lock->notify_all();
-  }
+  notify_scan_done();
 }
 
 bool CMRootRegions::wait_until_scan_finished() {
@@ -1224,13 +1230,11 @@ public:
 };
 
 void ConcurrentMark::scanRootRegions() {
-  // Start of concurrent marking.
-  ClassLoaderDataGraph::clear_claimed_marks();
-
   // scan_in_progress() will have been set to true only if there was
   // at least one root region to scan. So, if it's false, we
   // should not attempt to do any further work.
   if (root_regions()->scan_in_progress()) {
+    assert(!has_aborted(), "Aborting before root region scanning is finished not supported.");
     _parallel_marking_threads = calc_parallel_marking_threads();
     assert(parallel_marking_threads() <= max_parallel_marking_threads(),
            "Maximum number of marking threads exceeded");
diff --git a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp
index bbd5d590a..172caef29 100644
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp
@@ -333,6 +333,8 @@ private:
   volatile bool        _should_abort;
   HeapRegion* volatile _next_survivor;
 
+  void notify_scan_done();
+
 public:
   CMRootRegions();
   // We actually do most of the initialization in this method.
@@ -352,6 +354,8 @@ public:
   // all have been claimed.
   HeapRegion* claim_next();
 
+  void cancel_scan();
+
   // Flag that we're done with root region scanning and notify anyone
   // who's waiting on it. If aborted is false, assume that all regions
   // have been claimed.
diff --git a/hotspot/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp b/hotspot/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp
index 9b0452f92..3c4553bf7 100644
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp
@@ -23,6 +23,7 @@
  */
 
 #include "precompiled.hpp"
+#include "classfile/classLoaderData.hpp"
 #include "gc_implementation/g1/concurrentMarkThread.inline.hpp"
 #include "gc_implementation/g1/g1CollectedHeap.inline.hpp"
 #include "gc_implementation/g1/g1CollectorPolicy.hpp"
@@ -100,6 +101,10 @@ void ConcurrentMarkThread::run() {
       HandleMark   hm;
       double cycle_start = os::elapsedVTime();
 
+      {
+        ClassLoaderDataGraph::clear_claimed_marks();
+      }
+
       // We have to ensure that we finish scanning the root regions
       // before the next GC takes place. To ensure this we have to
       // make sure that we do not join the STS until the root regions
@@ -109,7 +114,7 @@ void ConcurrentMarkThread::run() {
       // correctness issue.
 
       double scan_start = os::elapsedTime();
-      if (!cm()->has_aborted()) {
+      {
         if (G1Log::fine()) {
           gclog_or_tty->gclog_stamp(cm()->concurrent_gc_id());
           gclog_or_tty->print_cr("[GC concurrent-root-region-scan-start]");
@@ -297,6 +302,7 @@ void ConcurrentMarkThread::run() {
     }
   }
   assert(_should_terminate, "just checking");
+  _cm->root_regions()->cancel_scan();
 
   terminate();
 }
diff --git a/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp b/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
index 5b156f99d..3ff5586c1 100644
--- a/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
+++ b/hotspot/src/share/vm/gc_implementation/g1/g1CollectedHeap.cpp
@@ -1346,8 +1346,7 @@ bool G1CollectedHeap::do_collection(bool explicit_gc,
       ref_processor_cm()->verify_no_references_recorded();
 
       // Abandon current iterations of concurrent marking and concurrent
-      // refinement, if any are in progress. We have to do this before
-      // wait_until_scan_finished() below.
+      // refinement, if any are in progress.
       concurrent_mark()->abort();
 
       // Make sure we'll choose a new allocation region afterwards.
@@ -4032,10 +4031,13 @@ G1CollectedHeap::do_collection_pause_at_safepoint(double target_pause_time_ms) {
   verify_region_sets_optional();
   verify_dirty_young_regions();
 
-  // This call will decide whether this pause is an initial-mark
-  // pause. If it is, during_initial_mark_pause() will return true
-  // for the duration of this pause.
-  g1_policy()->decide_on_conc_mark_initiation();
+  // We should not be doing initial mark unless the conc mark thread is running
+  if (!_cmThread->should_terminate()) {
+    // This call will decide whether this pause is an initial-mark
+    // pause. If it is, during_initial_mark_pause() will return true
+    // for the duration of this pause.
+    g1_policy()->decide_on_conc_mark_initiation();
+  }
 
   // We do not allow initial-mark to be piggy-backed on a mixed GC.
   assert(!g1_policy()->during_initial_mark_pause() ||
diff --git a/hotspot/src/share/vm/gc_implementation/shared/concurrentGCThread.hpp b/hotspot/src/share/vm/gc_implementation/shared/concurrentGCThread.hpp
index 1e16bf726..ceb65b029 100644
--- a/hotspot/src/share/vm/gc_implementation/shared/concurrentGCThread.hpp
+++ b/hotspot/src/share/vm/gc_implementation/shared/concurrentGCThread.hpp
@@ -71,6 +71,8 @@ public:
 
   // Tester
   bool is_ConcurrentGC_thread() const          { return true;       }
+
+  bool should_terminate() { return _should_terminate; }
 };
 
 // The SurrogateLockerThread is used by concurrent GC threads for
-- 
2.23.0