8205583: Crash in ConcurrentHashTable do_bulk_delete_locked_for

Reviewed-by: coleenp, gziemski
openjdk · Jun 27, 2018 · 1e4a26c · 1e4a26c
1 parent bcdf345
commit 1e4a26c
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 62 deletions.
diff --git a/src/hotspot/share/classfile/stringTable.cpp b/src/hotspot/share/classfile/stringTable.cpp
@@ -499,23 +499,15 @@ void StringTable::clean_dead_entries(JavaThread* jt) {
 
   StringTableDeleteCheck stdc;
   StringTableDoDelete stdd;
-  bool interrupted = false;
   {
     TraceTime timer("Clean", TRACETIME_LOG(Debug, stringtable, perf));
     while(bdt.do_task(jt, stdc, stdd)) {
       bdt.pause(jt);
       {
         ThreadBlockInVM tbivm(jt);
       }
-      if (!bdt.cont(jt)) {
-        interrupted = true;
-        break;
-      }
+      bdt.cont(jt);
     }
-  }
-  if (interrupted) {
-    _has_work = true;
-  } else {
     bdt.done(jt);
   }
   log_debug(stringtable)("Cleaned %ld of %ld", stdc._count, stdc._item);

diff --git a/src/hotspot/share/utilities/concurrentHashTableTasks.inline.hpp b/src/hotspot/share/utilities/concurrentHashTableTasks.inline.hpp
@@ -63,7 +63,8 @@ class ConcurrentHashTable<VALUE, CONFIG, F>::BucketsOperation {
   }
 
   // Calculate starting values.
-  void setup() {
+  void setup(Thread* thread) {
+    thread_owns_resize_lock(thread);
     _size_log2 = _cht->_table->_log2_size;
     _task_size_log2 = MIN2(_task_size_log2, _size_log2);
     size_t tmp = _size_log2 > _task_size_log2 ?
@@ -76,12 +77,6 @@ class ConcurrentHashTable<VALUE, CONFIG, F>::BucketsOperation {
     return OrderAccess::load_acquire(&_next_to_claim) >= _stop_task;
   }
 
-  // If we have changed size.
-  bool is_same_table() {
-    // Not entirely true.
-    return _size_log2 != _cht->_table->_log2_size;
-  }
-
   void thread_owns_resize_lock(Thread* thread) {
     assert(BucketsOperation::_cht->_resize_lock_owner == thread,
            "Should be locked by me");
@@ -100,6 +95,24 @@ class ConcurrentHashTable<VALUE, CONFIG, F>::BucketsOperation {
     assert(BucketsOperation::_cht->_resize_lock_owner != thread,
            "Should not be locked by me");
   }
+
+public:
+  // Pauses for safepoint
+  void pause(Thread* thread) {
+    // This leaves internal state locked.
+    this->thread_owns_resize_lock(thread);
+    BucketsOperation::_cht->_resize_lock->unlock();
+    this->thread_owns_only_state_lock(thread);
+  }
+
+  // Continues after safepoint.
+  void cont(Thread* thread) {
+    this->thread_owns_only_state_lock(thread);
+    // If someone slips in here directly after safepoint.
+    while (!BucketsOperation::_cht->_resize_lock->try_lock())
+      { /* for ever */ };
+    this->thread_owns_resize_lock(thread);
+  }
 };
 
 // For doing pausable/parallel bulk delete.
@@ -117,8 +130,7 @@ class ConcurrentHashTable<VALUE, CONFIG, F>::BulkDeleteTask :
     if (!lock) {
       return false;
     }
-    this->setup();
-    this->thread_owns_resize_lock(thread);
+    this->setup(thread);
     return true;
   }
 
@@ -135,30 +147,8 @@ class ConcurrentHashTable<VALUE, CONFIG, F>::BulkDeleteTask :
     BucketsOperation::_cht->do_bulk_delete_locked_for(thread, start, stop,
                                                       eval_f, del_f,
                                                       BucketsOperation::_is_mt);
-    return true;
-  }
-
-  // Pauses this operations for a safepoint.
-  void pause(Thread* thread) {
-    this->thread_owns_resize_lock(thread);
-    // This leaves internal state locked.
-    BucketsOperation::_cht->unlock_resize_lock(thread);
-    this->thread_do_not_own_resize_lock(thread);
-  }
-
-  // Continues this operations after a safepoint.
-  bool cont(Thread* thread) {
-    this->thread_do_not_own_resize_lock(thread);
-    if (!BucketsOperation::_cht->try_resize_lock(thread)) {
-      this->thread_do_not_own_resize_lock(thread);
-      return false;
-    }
-    if (BucketsOperation::is_same_table()) {
-      BucketsOperation::_cht->unlock_resize_lock(thread);
-      this->thread_do_not_own_resize_lock(thread);
-      return false;
-    }
-    this->thread_owns_resize_lock(thread);
+    assert(BucketsOperation::_cht->_resize_lock_owner != NULL,
+           "Should be locked");
     return true;
   }
 
@@ -183,8 +173,7 @@ class ConcurrentHashTable<VALUE, CONFIG, F>::GrowTask :
           thread, BucketsOperation::_cht->_log2_size_limit)) {
       return false;
     }
-    this->thread_owns_resize_lock(thread);
-    BucketsOperation::setup();
+    this->setup(thread);
     return true;
   }
 
@@ -202,23 +191,6 @@ class ConcurrentHashTable<VALUE, CONFIG, F>::GrowTask :
     return true;
   }
 
-  // Pauses growing for safepoint
-  void pause(Thread* thread) {
-    // This leaves internal state locked.
-    this->thread_owns_resize_lock(thread);
-    BucketsOperation::_cht->_resize_lock->unlock();
-    this->thread_owns_only_state_lock(thread);
-  }
-
-  // Continues growing after safepoint.
-  void cont(Thread* thread) {
-    this->thread_owns_only_state_lock(thread);
-    // If someone slips in here directly after safepoint.
-    while (!BucketsOperation::_cht->_resize_lock->try_lock())
-      { /* for ever */ };
-    this->thread_owns_resize_lock(thread);
-  }
-
   // Must be called after do_task returns false.
   void done(Thread* thread) {
     this->thread_owns_resize_lock(thread);

diff --git a/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp b/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp
@@ -213,7 +213,7 @@ static void cht_getinsert_bulkdelete_task(Thread* thr) {
   if (bdt.prepare(thr)) {
     while(bdt.do_task(thr, getinsert_bulkdelete_eval, getinsert_bulkdelete_del)) {
       bdt.pause(thr);
-      EXPECT_TRUE(bdt.cont(thr)) << "Uncontended continue should work.";
+      bdt.cont(thr);
     }
     bdt.done(thr);
   }