7099824: G1: we should take the pending list lock before doing the remark pause
Summary: Acquire the pending list lock in the prologue method of G1's concurrent VM_Operation and release the lock in the epilogue() method. The locking/unlocking order of the pending list lock and the Heap_lock should match that in the prologue and epilogue methods of VM_GC_Operation.
Reviewed-by: tonyp, ysr
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp Fri Oct 21 12:42:42 2011 -0400
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp Thu Oct 20 12:06:20 2011 -0700
@@ -147,12 +147,8 @@
}
}
} while (cm()->restart_for_overflow());
+
double counting_start_time = os::elapsedVTime();
-
- // YSR: These look dubious (i.e. redundant) !!! FIX ME
- slt()->manipulatePLL(SurrogateLockerThread::acquirePLL);
- slt()->manipulatePLL(SurrogateLockerThread::releaseAndNotifyPLL);
-
if (!cm()->has_aborted()) {
double count_start_sec = os::elapsedTime();
if (PrintGC) {
@@ -175,6 +171,7 @@
}
}
}
+
double end_time = os::elapsedVTime();
_vtime_count_accum += (end_time - counting_start_time);
// Update the total virtual time before doing this, since it will try
@@ -335,13 +332,15 @@
clear_started();
}
-// Note: this method, although exported by the ConcurrentMarkSweepThread,
-// which is a non-JavaThread, can only be called by a JavaThread.
-// Currently this is done at vm creation time (post-vm-init) by the
-// main/Primordial (Java)Thread.
-// XXX Consider changing this in the future to allow the CMS thread
+// Note: As is the case with CMS - this method, although exported
+// by the ConcurrentMarkThread, which is a non-JavaThread, can only
+// be called by a JavaThread. Currently this is done at vm creation
+// time (post-vm-init) by the main/Primordial (Java)Thread.
+// XXX Consider changing this in the future to allow the CM thread
// itself to create this thread?
void ConcurrentMarkThread::makeSurrogateLockerThread(TRAPS) {
+ assert(UseG1GC, "SLT thread needed only for concurrent GC");
+ assert(THREAD->is_Java_thread(), "must be a Java thread");
assert(_slt == NULL, "SLT already created");
_slt = SurrogateLockerThread::make(THREAD);
}
--- a/hotspot/src/share/vm/gc_implementation/g1/vm_operations_g1.cpp Fri Oct 21 12:42:42 2011 -0400
+++ b/hotspot/src/share/vm/gc_implementation/g1/vm_operations_g1.cpp Thu Oct 20 12:06:20 2011 -0700
@@ -23,6 +23,7 @@
*/
#include "precompiled.hpp"
+#include "gc_implementation/g1/concurrentMarkThread.inline.hpp"
#include "gc_implementation/g1/g1CollectedHeap.inline.hpp"
#include "gc_implementation/g1/g1CollectorPolicy.hpp"
#include "gc_implementation/g1/vm_operations_g1.hpp"
@@ -165,6 +166,20 @@
}
}
+void VM_CGC_Operation::acquire_pending_list_lock() {
+ // The caller may block while communicating
+ // with the SLT thread in order to acquire/release the PLL.
+ ConcurrentMarkThread::slt()->
+ manipulatePLL(SurrogateLockerThread::acquirePLL);
+}
+
+void VM_CGC_Operation::release_and_notify_pending_list_lock() {
+ // The caller may block while communicating
+ // with the SLT thread in order to acquire/release the PLL.
+ ConcurrentMarkThread::slt()->
+ manipulatePLL(SurrogateLockerThread::releaseAndNotifyPLL);
+}
+
void VM_CGC_Operation::doit() {
gclog_or_tty->date_stamp(PrintGC && PrintGCDateStamps);
TraceCPUTime tcpu(PrintGCDetails, true, gclog_or_tty);
@@ -180,12 +195,19 @@
}
bool VM_CGC_Operation::doit_prologue() {
+ // Note the relative order of the locks must match that in
+ // VM_GC_Operation::doit_prologue() or deadlocks can occur
+ acquire_pending_list_lock();
+
Heap_lock->lock();
SharedHeap::heap()->_thread_holds_heap_lock_for_gc = true;
return true;
}
void VM_CGC_Operation::doit_epilogue() {
+ // Note the relative order of the unlocks must match that in
+ // VM_GC_Operation::doit_epilogue()
SharedHeap::heap()->_thread_holds_heap_lock_for_gc = false;
Heap_lock->unlock();
+ release_and_notify_pending_list_lock();
}
--- a/hotspot/src/share/vm/gc_implementation/g1/vm_operations_g1.hpp Fri Oct 21 12:42:42 2011 -0400
+++ b/hotspot/src/share/vm/gc_implementation/g1/vm_operations_g1.hpp Thu Oct 20 12:06:20 2011 -0700
@@ -93,11 +93,17 @@
}
};
-// Concurrent GC stop-the-world operations such as initial and final mark;
+// Concurrent GC stop-the-world operations such as remark and cleanup;
// consider sharing these with CMS's counterparts.
class VM_CGC_Operation: public VM_Operation {
VoidClosure* _cl;
const char* _printGCMessage;
+
+protected:
+ // java.lang.ref.Reference support
+ void acquire_pending_list_lock();
+ void release_and_notify_pending_list_lock();
+
public:
VM_CGC_Operation(VoidClosure* cl, const char *printGCMsg)
: _cl(cl), _printGCMessage(printGCMsg) { }
--- a/hotspot/src/share/vm/gc_implementation/shared/concurrentGCThread.cpp Fri Oct 21 12:42:42 2011 -0400
+++ b/hotspot/src/share/vm/gc_implementation/shared/concurrentGCThread.cpp Thu Oct 20 12:06:20 2011 -0700
@@ -224,6 +224,8 @@
MutexLockerEx x(&_monitor, Mutex::_no_safepoint_check_flag);
assert(_buffer == empty, "Should be empty");
assert(msg != empty, "empty message");
+ assert(!Heap_lock->owned_by_self(), "Heap_lock owned by requesting thread");
+
_buffer = msg;
while (_buffer != empty) {
_monitor.notify();