7099824: G1: we should take the pending list lock before doing the remark pause

Thu, 20 Oct 2011 12:06:20 -0700

author
johnc
date
Thu, 20 Oct 2011 12:06:20 -0700
changeset 3218
db89aa49298f
parent 3217
8d161913dfc3
child 3219
c6a6e936dc68

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

src/share/vm/gc_implementation/g1/concurrentMarkThread.cpp file | annotate | diff | comparison | revisions
src/share/vm/gc_implementation/g1/vm_operations_g1.cpp file | annotate | diff | comparison | revisions
src/share/vm/gc_implementation/g1/vm_operations_g1.hpp file | annotate | diff | comparison | revisions
src/share/vm/gc_implementation/shared/concurrentGCThread.cpp file | annotate | diff | comparison | revisions
     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();

mercurial