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

Reviewed-by: phh
Backport-of: 2aeb12ec03944c777d617d0be48982fd225b16e7

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

@@ -164,11 +164,6 @@ public:
     AbstractGangTask("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) {
@@ -268,13 +263,7 @@ public:
     AbstractGangTask("Shenandoah Unlink NMethods"),
     _cl(unloading_occurred),
     _verifier(verifier),
-    _iterator(ShenandoahCodeRoots::table()) {
-    _iterator.nmethods_do_begin();
-  }
-
-  ~ShenandoahUnlinkTask() {
-    _iterator.nmethods_do_end();
-  }
+    _iterator(ShenandoahCodeRoots::table()) {}
 
   virtual void work(uint worker_id) {
     ICRefillVerifierMark mark(_verifier);
@@ -326,13 +315,7 @@ public:
   ShenandoahNMethodPurgeTask() :
     AbstractGangTask("Shenandoah Purge NMethods"),
     _cl(),
-    _iterator(ShenandoahCodeRoots::table()) {
-    _iterator.nmethods_do_begin();
-  }
-
-  ~ShenandoahNMethodPurgeTask() {
-    _iterator.nmethods_do_end();
-  }
+    _iterator(ShenandoahCodeRoots::table()) {}
 
   virtual void work(uint worker_id) {
     _iterator.nmethods_do(&_cl);

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

@@ -749,16 +749,9 @@ public:
     _vm_roots(phase),
     _cld_roots(phase, ShenandoahHeap::heap()->workers()->active_workers()),
     _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();
   }
@@ -859,17 +852,7 @@ public:
     _phase(phase),
     _vm_roots(phase),
     _cld_roots(phase, ShenandoahHeap::heap()->workers()->active_workers()),
-    _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

@@ -29,6 +29,7 @@
 #include "gc/shenandoah/shenandoahHeap.inline.hpp"
 #include "gc/shenandoah/shenandoahNMethod.inline.hpp"
 #include "memory/resourceArea.hpp"
+#include "runtime/safepointVerifiers.hpp"
 
 ShenandoahNMethod::ShenandoahNMethod(nmethod* nm, GrowableArray<oop*>& oops, bool non_immediate_oops) :
   _nm(nm), _oops(NULL), _oops_count(0), _unregistered(false) {
@@ -543,21 +544,40 @@ void ShenandoahNMethodTableSnapshot::concurrent_nmethods_do(NMethodClosure* cl)
 }
 
 ShenandoahConcurrentNMethodIterator::ShenandoahConcurrentNMethodIterator(ShenandoahNMethodTable* table) :
-  _table(table), _table_snapshot(NULL) {
+  _table(table),
+  _table_snapshot(nullptr),
+  _started_workers(0),
+  _finished_workers(0) {}
+
+void ShenandoahConcurrentNMethodIterator::nmethods_do(NMethodClosure* cl) {
+  // Cannot safepoint when iteration is running, because this can cause deadlocks
+  // with other threads waiting on iteration to be over.
+  NoSafepointVerifier nsv;
+
+  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;
   }
 
-void ShenandoahConcurrentNMethodIterator::nmethods_do_begin() {
-  MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
+  // Record a new worker and initialize the snapshot if it is a first visitor.
+  if (_started_workers++ == 0) {
     _table_snapshot = _table->snapshot_for_iteration();
   }
 
-void ShenandoahConcurrentNMethodIterator::nmethods_do(NMethodClosure* cl) {
-  assert(_table_snapshot != NULL, "Must first call nmethod_do_begin()");
+  // All set, relinquish the lock and go concurrent.
+  {
+    MutexUnlocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
     _table_snapshot->concurrent_nmethods_do(cl);
   }
 
-void ShenandoahConcurrentNMethodIterator::nmethods_do_end() {
-  MutexLocker mu(CodeCache_lock, Mutex::_no_safepoint_check_flag);
+  // 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

@@ -187,13 +187,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