8334482: Shenandoah: Deadlock when safepoint is pending during nmethods iteration

Reviewed-by: wkemper, xpeng, rkennke

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

@@ -103,11 +103,6 @@ public:
     WorkerTask("Shenandoah Disarm NMethods"),
     _iterator(ShenandoahCodeRoots::table()) {
     assert(SafepointSynchronize::is_at_safepoint(), "Only at a safepoint");
-    _iterator.nmethods_do_begin();
-  }
-
-  ~ShenandoahDisarmNMethodsTask() {
-    _iterator.nmethods_do_end();
   }
 
   virtual void work(uint worker_id) {
@@ -175,13 +170,7 @@ public:
   ShenandoahUnlinkTask(bool unloading_occurred) :
     WorkerTask("Shenandoah Unlink NMethods"),
     _cl(unloading_occurred),
-    _iterator(ShenandoahCodeRoots::table()) {
-    _iterator.nmethods_do_begin();
-  }
-
-  ~ShenandoahUnlinkTask() {
-    _iterator.nmethods_do_end();
-  }
+    _iterator(ShenandoahCodeRoots::table()) {}
 
   virtual void work(uint worker_id) {
     _iterator.nmethods_do(&_cl);

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

@@ -767,16 +767,9 @@ public:
     _vm_roots(phase),
     _cld_roots(phase, ShenandoahHeap::heap()->workers()->active_workers(), false /*heap iteration*/),
     _nmethod_itr(ShenandoahCodeRoots::table()),
-    _phase(phase) {
-    if (ShenandoahHeap::heap()->unload_classes()) {
-      _nmethod_itr.nmethods_do_begin();
-    }
-  }
+    _phase(phase) {}
 
   ~ShenandoahConcurrentWeakRootsEvacUpdateTask() {
-    if (ShenandoahHeap::heap()->unload_classes()) {
-      _nmethod_itr.nmethods_do_end();
-    }
     // Notify runtime data structures of potentially dead oops
     _vm_roots.report_num_dead();
   }
@@ -878,17 +871,7 @@ public:
     _phase(phase),
     _vm_roots(phase),
     _cld_roots(phase, ShenandoahHeap::heap()->workers()->active_workers(), false /*heap iteration*/),
-    _nmethod_itr(ShenandoahCodeRoots::table()) {
-    if (!ShenandoahHeap::heap()->unload_classes()) {
-      _nmethod_itr.nmethods_do_begin();
-    }
-  }
-
-  ~ShenandoahConcurrentRootsEvacUpdateTask() {
-    if (!ShenandoahHeap::heap()->unload_classes()) {
-      _nmethod_itr.nmethods_do_end();
-    }
-  }
+    _nmethod_itr(ShenandoahCodeRoots::table()) {}
 
   void work(uint worker_id) {
     ShenandoahConcurrentWorkerSession worker_session(worker_id);

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

@@ -31,6 +31,7 @@
 #include "gc/shenandoah/shenandoahOopClosures.inline.hpp"
 #include "memory/resourceArea.hpp"
 #include "runtime/continuation.hpp"
+#include "runtime/safepointVerifiers.hpp"
 
 ShenandoahNMethod::ShenandoahNMethod(nmethod* nm, GrowableArray<oop*>& oops, bool non_immediate_oops) :
   _nm(nm), _oops(nullptr), _oops_count(0), _unregistered(false), _lock(), _ic_lock() {
@@ -475,21 +476,40 @@ void ShenandoahNMethodTableSnapshot::concurrent_nmethods_do(NMethodClosure* cl)
 }
 
 ShenandoahConcurrentNMethodIterator::ShenandoahConcurrentNMethodIterator(ShenandoahNMethodTable* table) :
-  _table(table), _table_snapshot(nullptr) {
-}
-
-void ShenandoahConcurrentNMethodIterator::nmethods_do_begin() {
-  MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
-  _table_snapshot = _table->snapshot_for_iteration();
-}
+  _table(table),
+  _table_snapshot(nullptr),
+  _started_workers(0),
+  _finished_workers(0) {}
 
 void ShenandoahConcurrentNMethodIterator::nmethods_do(NMethodClosure* cl) {
-  assert(_table_snapshot != nullptr, "Must first call nmethod_do_begin()");
-  _table_snapshot->concurrent_nmethods_do(cl);
-}
+  // Cannot safepoint when iteration is running, because this can cause deadlocks
+  // with other threads waiting on iteration to be over.
+  NoSafepointVerifier nsv;
 
-void ShenandoahConcurrentNMethodIterator::nmethods_do_end() {
-  MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
-  _table->finish_iteration(_table_snapshot);
-  CodeCache_lock->notify_all();
+  MutexLocker ml(CodeCache_lock, Mutex::_no_safepoint_check_flag);
+
+  if (_finished_workers > 0) {
+    // Some threads have already finished. We are now in rampdown: we are now
+    // waiting for all currently recorded workers to finish. No new workers
+    // should start.
+    return;
+  }
+
+  // Record a new worker and initialize the snapshot if it is a first visitor.
+  if (_started_workers++ == 0) {
+    _table_snapshot = _table->snapshot_for_iteration();
+  }
+
+  // All set, relinquish the lock and go concurrent.
+  {
+    MutexUnlocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
+    _table_snapshot->concurrent_nmethods_do(cl);
+  }
+
+  // Record completion. Last worker shuts down the iterator and notifies any waiters.
+  uint count = ++_finished_workers;
+  if (count == _started_workers) {
+    _table->finish_iteration(_table_snapshot);
+    CodeCache_lock->notify_all();
+  }
 }

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

@@ -181,13 +181,13 @@ class ShenandoahConcurrentNMethodIterator {
 private:
   ShenandoahNMethodTable*         const _table;
   ShenandoahNMethodTableSnapshot*       _table_snapshot;
+  uint                                  _started_workers;
+  uint                                  _finished_workers;
 
 public:
   ShenandoahConcurrentNMethodIterator(ShenandoahNMethodTable* table);
 
-  void nmethods_do_begin();
   void nmethods_do(NMethodClosure* cl);
-  void nmethods_do_end();
 };
 
 #endif // SHARE_GC_SHENANDOAH_SHENANDOAHNMETHOD_HPP