8387265: G1: Shutdown during concurrent cycle leaves SATB queues in inconsistent state

Reviewed-by: aboldtch, ayang

src/hotspot/share/gc/g1/g1CollectedHeap.cpp

@@ -1654,6 +1654,9 @@ void G1CollectedHeap::stop() {
   // that are destroyed during shutdown.
   _cr->stop();
   _service_thread->stop();
+  VM_G1StopMarking op;
+  VMThread::execute(&op);
+
   _cm->stop();
 }
 

src/hotspot/share/gc/g1/g1ConcurrentMark.cpp

@@ -2036,6 +2036,26 @@ void G1ConcurrentMark::print_stats() {
   }
 }
 
+bool G1ConcurrentMark::shutdown_cleanup_needed() const {
+  // Cleanup (aborting threads, setting abort flags) is needed throughout the whole cycle before
+  // stopping the CM thread.
+  return is_fully_initialized() && is_in_concurrent_cycle();
+}
+
+void G1ConcurrentMark::shutdown_concurrent_cycle() {
+  assert_at_safepoint_on_vm_thread();
+
+  abort_root_region_scan_at_safepoint();
+  abort_marking_threads();
+
+  SATBMarkQueueSet& satb_mq_set = G1BarrierSet::satb_mark_queue_set();
+  satb_mq_set.abandon_partial_marking();
+  // This can be called either during or outside marking, we'll read
+  // the expected_active value from the SATB queue set.
+  satb_mq_set.set_active_all_threads(false, /* new active value */
+                                     satb_mq_set.is_active() /* expected_active */);
+}
+
 bool G1ConcurrentMark::concurrent_cycle_abort() {
   assert_at_safepoint_on_vm_thread();
   assert(_g1h->collector_state()->is_in_full_gc(), "must be");

src/hotspot/share/gc/g1/g1ConcurrentMark.hpp

@@ -608,6 +608,8 @@ public:
   bool mark_stack_empty() const                 { return _global_mark_stack.is_empty(); }
 
   void concurrent_cycle_start();
+  bool shutdown_cleanup_needed() const;
+  void shutdown_concurrent_cycle();
   // Abandon current marking iteration due to a Full GC.
   bool concurrent_cycle_abort();
   void concurrent_cycle_end(bool mark_cycle_completed);

src/hotspot/share/gc/g1/g1ConcurrentMarkThread.hpp

@@ -50,7 +50,8 @@ class G1ConcurrentMarkThread: public ConcurrentGCThread {
 
   Atomic<ServiceState> _state;
 
-  ServiceState state() const { return _state.load_relaxed(); }
+  ServiceState state() const { return _state.load_acquire(); }
+  void set_state(ServiceState new_state) { _state.release_store(new_state); }
 
   // Returns whether we are in a "Full" cycle.
   bool is_in_full_concurrent_cycle() const;

src/hotspot/share/gc/g1/g1ConcurrentMarkThread.inline.hpp

@@ -48,31 +48,31 @@ inline bool G1ConcurrentMarkThread::is_in_full_concurrent_cycle() const {
 inline void G1ConcurrentMarkThread::set_idle() {
   // Concurrent cycle may be aborted any time.
   assert(!is_idle(), "must not be idle");
-  _state.store_relaxed(Idle);
+  set_state(Idle);
 }
 
 inline void G1ConcurrentMarkThread::start_full_cycle() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be");
   assert(is_idle(), "cycle in progress");
-  _state.store_relaxed(FullCycleMarking);
+  set_state(FullCycleMarking);
 }
 
 inline void G1ConcurrentMarkThread::start_undo_cycle() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be");
   assert(is_idle(), "cycle in progress");
-  _state.store_relaxed(UndoCycleResetForNextCycle);
+  set_state(UndoCycleResetForNextCycle);
 }
 
 inline void G1ConcurrentMarkThread::set_full_cycle_rebuild_and_scrub() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be");
   assert(state() == FullCycleMarking, "must be");
-  _state.store_relaxed(FullCycleRebuildOrScrub);
+  set_state(FullCycleRebuildOrScrub);
 }
 
 inline void G1ConcurrentMarkThread::set_full_cycle_reset_for_next_cycle() {
   assert(SafepointSynchronize::is_at_safepoint(), "must be");
   assert(state() == FullCycleRebuildOrScrub, "must be");
-  _state.store_relaxed(FullCycleResetForNextCycle);
+  set_state(FullCycleResetForNextCycle);
 }
 
 inline bool G1ConcurrentMarkThread::is_in_marking() const {

src/hotspot/share/gc/g1/g1SATBMarkQueueSet.cpp

@@ -114,12 +114,5 @@ public:
 };
 
 void G1SATBMarkQueueSet::filter(SATBMarkQueue& queue) {
-  G1CollectedHeap* g1h = G1CollectedHeap::heap();
-  if (g1h->collector_state()->is_in_marking()) {
-    apply_filter(G1SATBMarkQueueFilterFn(), queue);
-  } else {
-    // is_in_marking() covers both the concurrent marking and the Remark pause. Outside
-    // of that, there can be no entry that requires SATB marking.
-    queue.set_empty();
-  }
+  apply_filter(G1SATBMarkQueueFilterFn(), queue);
 }

src/hotspot/share/gc/g1/g1VMOperations.cpp

@@ -130,8 +130,13 @@ void VM_G1CollectForAllocation::doit() {
 }
 
 void VM_G1PauseConcurrent::doit() {
-  GCIdMark gc_id_mark(_gc_id);
   G1CollectedHeap* g1h = G1CollectedHeap::heap();
+  if (_is_shutting_down) {
+    g1h->concurrent_mark()->shutdown_concurrent_cycle();
+    return;
+  }
+
+  GCIdMark gc_id_mark(_gc_id);
   GCTraceCPUTime tcpu(g1h->concurrent_mark()->gc_tracer_cm());
 
   // GCTraceTime(...) only supports sub-phases, so a more verbose version
@@ -150,12 +155,9 @@ void VM_G1PauseConcurrent::doit() {
 bool VM_G1PauseConcurrent::doit_prologue() {
   Heap_lock->lock();
   G1CollectedHeap* g1h = G1CollectedHeap::heap();
-  if (g1h->is_shutting_down()) {
+  _is_shutting_down = g1h->is_shutting_down();
+  if (_is_shutting_down && !g1h->concurrent_mark()->shutdown_cleanup_needed()) {
     Heap_lock->unlock();
-    // JVM shutdown has started. Abort concurrent marking to ensure that any further
-    // concurrent VM operations will not try to start and interfere with the shutdown
-    // process.
-    g1h->concurrent_mark()->abort_marking_threads();
     return false;
   }
   return true;
@@ -177,3 +179,19 @@ void VM_G1PauseCleanup::work() {
   G1ConcurrentMark* cm = G1CollectedHeap::heap()->concurrent_mark();
   cm->cleanup();
 }
+
+bool VM_G1StopMarking::doit_prologue() {
+  G1CollectedHeap* g1h = G1CollectedHeap::heap();
+#ifdef ASSERT
+  {
+    MutexLocker ml(Heap_lock);
+    assert(g1h->is_shutting_down(), "must be");
+  }
+#endif
+  return g1h->concurrent_mark()->shutdown_cleanup_needed();
+}
+
+void VM_G1StopMarking::doit() {
+  G1ConcurrentMark* cm = G1CollectedHeap::heap()->concurrent_mark();
+  cm->shutdown_concurrent_cycle();
+}

src/hotspot/share/gc/g1/g1VMOperations.hpp

@@ -80,11 +80,12 @@ public:
 // Concurrent G1 stop-the-world operations such as remark and cleanup.
 class VM_G1PauseConcurrent : public VM_Operation {
   uint         _gc_id;
+  bool         _is_shutting_down;
   const char*  _message;
 
 protected:
   VM_G1PauseConcurrent(const char* message) :
-    _gc_id(GCId::current()), _message(message) { }
+    _gc_id(GCId::current()), _is_shutting_down(false), _message(message) { }
   virtual void work() = 0;
 
   // Does this concurrent pause affect the memory pools? If so, update the collectionUsage()
@@ -116,4 +117,15 @@ public:
   void work() override;
 };
 
+class VM_G1StopMarking : public VM_Operation {
+public:
+  VM_G1StopMarking() : VM_Operation() { }
+  VMOp_Type type() const override { return VMOp_G1StopMarking; }
+
+  bool doit_prologue() override;
+  void doit() override;
+
+  bool is_gc_operation() const override { return true; }
+};
+
 #endif // SHARE_GC_G1_G1VMOPERATIONS_HPP

src/hotspot/share/runtime/vmOperation.hpp

@@ -59,6 +59,7 @@
   template(G1PauseCleanup)                        \
   template(G1TryInitiateConcMark)                 \
   template(G1RendezvousGCThreads)                 \
+  template(G1StopMarking)                         \
   template(ZMarkEndOld)                           \
   template(ZMarkEndYoung)                         \
   template(ZMarkFlushOperation)                   \