Thu, 20 Oct 2011 12:06:20 -0700
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
1.1 --- a/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp Fri Oct 21 12:42:42 2011 -0400 1.2 +++ b/src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp Thu Oct 20 12:06:20 2011 -0700 1.3 @@ -147,12 +147,8 @@ 1.4 } 1.5 } 1.6 } while (cm()->restart_for_overflow()); 1.7 + 1.8 double counting_start_time = os::elapsedVTime(); 1.9 - 1.10 - // YSR: These look dubious (i.e. redundant) !!! FIX ME 1.11 - slt()->manipulatePLL(SurrogateLockerThread::acquirePLL); 1.12 - slt()->manipulatePLL(SurrogateLockerThread::releaseAndNotifyPLL); 1.13 - 1.14 if (!cm()->has_aborted()) { 1.15 double count_start_sec = os::elapsedTime(); 1.16 if (PrintGC) { 1.17 @@ -175,6 +171,7 @@ 1.18 } 1.19 } 1.20 } 1.21 + 1.22 double end_time = os::elapsedVTime(); 1.23 _vtime_count_accum += (end_time - counting_start_time); 1.24 // Update the total virtual time before doing this, since it will try 1.25 @@ -335,13 +332,15 @@ 1.26 clear_started(); 1.27 } 1.28 1.29 -// Note: this method, although exported by the ConcurrentMarkSweepThread, 1.30 -// which is a non-JavaThread, can only be called by a JavaThread. 1.31 -// Currently this is done at vm creation time (post-vm-init) by the 1.32 -// main/Primordial (Java)Thread. 1.33 -// XXX Consider changing this in the future to allow the CMS thread 1.34 +// Note: As is the case with CMS - this method, although exported 1.35 +// by the ConcurrentMarkThread, which is a non-JavaThread, can only 1.36 +// be called by a JavaThread. Currently this is done at vm creation 1.37 +// time (post-vm-init) by the main/Primordial (Java)Thread. 1.38 +// XXX Consider changing this in the future to allow the CM thread 1.39 // itself to create this thread? 1.40 void ConcurrentMarkThread::makeSurrogateLockerThread(TRAPS) { 1.41 + assert(UseG1GC, "SLT thread needed only for concurrent GC"); 1.42 + assert(THREAD->is_Java_thread(), "must be a Java thread"); 1.43 assert(_slt == NULL, "SLT already created"); 1.44 _slt = SurrogateLockerThread::make(THREAD); 1.45 }
2.1 --- a/src/share/vm/gc_implementation/g1/vm_operations_g1.cpp Fri Oct 21 12:42:42 2011 -0400 2.2 +++ b/src/share/vm/gc_implementation/g1/vm_operations_g1.cpp Thu Oct 20 12:06:20 2011 -0700 2.3 @@ -23,6 +23,7 @@ 2.4 */ 2.5 2.6 #include "precompiled.hpp" 2.7 +#include "gc_implementation/g1/concurrentMarkThread.inline.hpp" 2.8 #include "gc_implementation/g1/g1CollectedHeap.inline.hpp" 2.9 #include "gc_implementation/g1/g1CollectorPolicy.hpp" 2.10 #include "gc_implementation/g1/vm_operations_g1.hpp" 2.11 @@ -165,6 +166,20 @@ 2.12 } 2.13 } 2.14 2.15 +void VM_CGC_Operation::acquire_pending_list_lock() { 2.16 + // The caller may block while communicating 2.17 + // with the SLT thread in order to acquire/release the PLL. 2.18 + ConcurrentMarkThread::slt()-> 2.19 + manipulatePLL(SurrogateLockerThread::acquirePLL); 2.20 +} 2.21 + 2.22 +void VM_CGC_Operation::release_and_notify_pending_list_lock() { 2.23 + // The caller may block while communicating 2.24 + // with the SLT thread in order to acquire/release the PLL. 2.25 + ConcurrentMarkThread::slt()-> 2.26 + manipulatePLL(SurrogateLockerThread::releaseAndNotifyPLL); 2.27 +} 2.28 + 2.29 void VM_CGC_Operation::doit() { 2.30 gclog_or_tty->date_stamp(PrintGC && PrintGCDateStamps); 2.31 TraceCPUTime tcpu(PrintGCDetails, true, gclog_or_tty); 2.32 @@ -180,12 +195,19 @@ 2.33 } 2.34 2.35 bool VM_CGC_Operation::doit_prologue() { 2.36 + // Note the relative order of the locks must match that in 2.37 + // VM_GC_Operation::doit_prologue() or deadlocks can occur 2.38 + acquire_pending_list_lock(); 2.39 + 2.40 Heap_lock->lock(); 2.41 SharedHeap::heap()->_thread_holds_heap_lock_for_gc = true; 2.42 return true; 2.43 } 2.44 2.45 void VM_CGC_Operation::doit_epilogue() { 2.46 + // Note the relative order of the unlocks must match that in 2.47 + // VM_GC_Operation::doit_epilogue() 2.48 SharedHeap::heap()->_thread_holds_heap_lock_for_gc = false; 2.49 Heap_lock->unlock(); 2.50 + release_and_notify_pending_list_lock(); 2.51 }
3.1 --- a/src/share/vm/gc_implementation/g1/vm_operations_g1.hpp Fri Oct 21 12:42:42 2011 -0400 3.2 +++ b/src/share/vm/gc_implementation/g1/vm_operations_g1.hpp Thu Oct 20 12:06:20 2011 -0700 3.3 @@ -93,11 +93,17 @@ 3.4 } 3.5 }; 3.6 3.7 -// Concurrent GC stop-the-world operations such as initial and final mark; 3.8 +// Concurrent GC stop-the-world operations such as remark and cleanup; 3.9 // consider sharing these with CMS's counterparts. 3.10 class VM_CGC_Operation: public VM_Operation { 3.11 VoidClosure* _cl; 3.12 const char* _printGCMessage; 3.13 + 3.14 +protected: 3.15 + // java.lang.ref.Reference support 3.16 + void acquire_pending_list_lock(); 3.17 + void release_and_notify_pending_list_lock(); 3.18 + 3.19 public: 3.20 VM_CGC_Operation(VoidClosure* cl, const char *printGCMsg) 3.21 : _cl(cl), _printGCMessage(printGCMsg) { }
4.1 --- a/src/share/vm/gc_implementation/shared/concurrentGCThread.cpp Fri Oct 21 12:42:42 2011 -0400 4.2 +++ b/src/share/vm/gc_implementation/shared/concurrentGCThread.cpp Thu Oct 20 12:06:20 2011 -0700 4.3 @@ -224,6 +224,8 @@ 4.4 MutexLockerEx x(&_monitor, Mutex::_no_safepoint_check_flag); 4.5 assert(_buffer == empty, "Should be empty"); 4.6 assert(msg != empty, "empty message"); 4.7 + assert(!Heap_lock->owned_by_self(), "Heap_lock owned by requesting thread"); 4.8 + 4.9 _buffer = msg; 4.10 while (_buffer != empty) { 4.11 _monitor.notify();