8205583: Crash in ConcurrentHashTable do_bulk_delete_locked_for
Reviewed-by: coleenp, gziemski
--- a/src/hotspot/share/classfile/stringTable.cpp Wed Jun 27 11:54:15 2018 +0200
+++ b/src/hotspot/share/classfile/stringTable.cpp Wed Jun 27 12:46:15 2018 +0200
@@ -499,7 +499,6 @@
StringTableDeleteCheck stdc;
StringTableDoDelete stdd;
- bool interrupted = false;
{
TraceTime timer("Clean", TRACETIME_LOG(Debug, stringtable, perf));
while(bdt.do_task(jt, stdc, stdd)) {
@@ -507,15 +506,8 @@
{
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);
--- a/src/hotspot/share/utilities/concurrentHashTableTasks.inline.hpp Wed Jun 27 11:54:15 2018 +0200
+++ b/src/hotspot/share/utilities/concurrentHashTableTasks.inline.hpp Wed Jun 27 12:46:15 2018 +0200
@@ -63,7 +63,8 @@
}
// 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 @@
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 @@
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 @@
if (!lock) {
return false;
}
- this->setup();
- this->thread_owns_resize_lock(thread);
+ this->setup(thread);
return true;
}
@@ -135,30 +147,8 @@
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 @@
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 @@
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);
--- a/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp Wed Jun 27 11:54:15 2018 +0200
+++ b/test/hotspot/gtest/utilities/test_concurrentHashtable.cpp Wed Jun 27 12:46:15 2018 +0200
@@ -213,7 +213,7 @@
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);
}