6940310: G1: MT-unsafe calls to CM::region_stack_push() / CM::region_stack_pop()

Mon, 05 Apr 2010 12:19:22 -0400

author
tonyp
date
Mon, 05 Apr 2010 12:19:22 -0400
changeset 1793
72f725c5a7be
parent 1792
781e29eb8e08
child 1794
23b1b27ac76c

6940310: G1: MT-unsafe calls to CM::region_stack_push() / CM::region_stack_pop()
Summary: Calling the methods region_stack_push() and region_stack_pop() concurrent is not MT-safe. The assumption is that we will only call region_stack_push() during a GC pause and region_stack_pop() during marking. Unfortunately, we also call region_stack_push() during marking which seems to be introducing subtle marking failures. This change introduces lock-based methods for pushing / popping to be called during marking.
Reviewed-by: iveresov, johnc

src/share/vm/gc_implementation/g1/concurrentMark.cpp file | annotate | diff | comparison | revisions
src/share/vm/gc_implementation/g1/concurrentMark.hpp file | annotate | diff | comparison | revisions
src/share/vm/runtime/mutexLocker.cpp file | annotate | diff | comparison | revisions
src/share/vm/runtime/mutexLocker.hpp file | annotate | diff | comparison | revisions
     1.1 --- a/src/share/vm/gc_implementation/g1/concurrentMark.cpp	Fri Apr 02 12:10:08 2010 -0400
     1.2 +++ b/src/share/vm/gc_implementation/g1/concurrentMark.cpp	Mon Apr 05 12:19:22 2010 -0400
     1.3 @@ -297,6 +297,11 @@
     1.4    }
     1.5  }
     1.6  
     1.7 +// Currently we do not call this at all. Normally we would call it
     1.8 +// during the concurrent marking / remark phases but we now call
     1.9 +// the lock-based version instead. But we might want to resurrect this
    1.10 +// code in the future. So, we'll leave it here commented out.
    1.11 +#if 0
    1.12  MemRegion CMRegionStack::pop() {
    1.13    while (true) {
    1.14      // Otherwise...
    1.15 @@ -321,6 +326,41 @@
    1.16      // Otherwise, we need to try again.
    1.17    }
    1.18  }
    1.19 +#endif // 0
    1.20 +
    1.21 +void CMRegionStack::push_with_lock(MemRegion mr) {
    1.22 +  assert(mr.word_size() > 0, "Precondition");
    1.23 +  MutexLockerEx x(CMRegionStack_lock, Mutex::_no_safepoint_check_flag);
    1.24 +
    1.25 +  if (isFull()) {
    1.26 +    _overflow = true;
    1.27 +    return;
    1.28 +  }
    1.29 +
    1.30 +  _base[_index] = mr;
    1.31 +  _index += 1;
    1.32 +}
    1.33 +
    1.34 +MemRegion CMRegionStack::pop_with_lock() {
    1.35 +  MutexLockerEx x(CMRegionStack_lock, Mutex::_no_safepoint_check_flag);
    1.36 +
    1.37 +  while (true) {
    1.38 +    if (_index == 0) {
    1.39 +      return MemRegion();
    1.40 +    }
    1.41 +    _index -= 1;
    1.42 +
    1.43 +    MemRegion mr = _base[_index];
    1.44 +    if (mr.start() != NULL) {
    1.45 +      assert(mr.end() != NULL, "invariant");
    1.46 +      assert(mr.word_size() > 0, "invariant");
    1.47 +      return mr;
    1.48 +    } else {
    1.49 +      // that entry was invalidated... let's skip it
    1.50 +      assert(mr.end() == NULL, "invariant");
    1.51 +    }
    1.52 +  }
    1.53 +}
    1.54  
    1.55  bool CMRegionStack::invalidate_entries_into_cset() {
    1.56    bool result = false;
    1.57 @@ -3363,7 +3403,7 @@
    1.58        gclog_or_tty->print_cr("[%d] draining region stack, size = %d",
    1.59                               _task_id, _cm->region_stack_size());
    1.60  
    1.61 -    MemRegion mr = _cm->region_stack_pop();
    1.62 +    MemRegion mr = _cm->region_stack_pop_with_lock();
    1.63      // it returns MemRegion() if the pop fails
    1.64      statsOnly(if (mr.start() != NULL) ++_region_stack_pops );
    1.65  
    1.66 @@ -3384,7 +3424,7 @@
    1.67          if (has_aborted())
    1.68            mr = MemRegion();
    1.69          else {
    1.70 -          mr = _cm->region_stack_pop();
    1.71 +          mr = _cm->region_stack_pop_with_lock();
    1.72            // it returns MemRegion() if the pop fails
    1.73            statsOnly(if (mr.start() != NULL) ++_region_stack_pops );
    1.74          }
    1.75 @@ -3417,7 +3457,7 @@
    1.76            }
    1.77            // Now push the part of the region we didn't scan on the
    1.78            // region stack to make sure a task scans it later.
    1.79 -          _cm->region_stack_push(newRegion);
    1.80 +          _cm->region_stack_push_with_lock(newRegion);
    1.81          }
    1.82          // break from while
    1.83          mr = MemRegion();
     2.1 --- a/src/share/vm/gc_implementation/g1/concurrentMark.hpp	Fri Apr 02 12:10:08 2010 -0400
     2.2 +++ b/src/share/vm/gc_implementation/g1/concurrentMark.hpp	Mon Apr 05 12:19:22 2010 -0400
     2.3 @@ -252,9 +252,19 @@
     2.4    // with other "push" operations (no pops).
     2.5    void push(MemRegion mr);
     2.6  
     2.7 +#if 0
     2.8 +  // This is currently not used. See the comment in the .cpp file.
     2.9 +
    2.10    // Lock-free; assumes that it will only be called in parallel
    2.11    // with other "pop" operations (no pushes).
    2.12    MemRegion pop();
    2.13 +#endif // 0
    2.14 +
    2.15 +  // These two are the implementations that use a lock. They can be
    2.16 +  // called concurrently with each other but they should not be called
    2.17 +  // concurrently with the lock-free versions (push() / pop()).
    2.18 +  void push_with_lock(MemRegion mr);
    2.19 +  MemRegion pop_with_lock();
    2.20  
    2.21    bool isEmpty()    { return _index == 0; }
    2.22    bool isFull()     { return _index == _capacity; }
    2.23 @@ -540,6 +550,10 @@
    2.24  
    2.25    // Manipulation of the region stack
    2.26    bool region_stack_push(MemRegion mr) {
    2.27 +    // Currently we only call the lock-free version during evacuation
    2.28 +    // pauses.
    2.29 +    assert(SafepointSynchronize::is_at_safepoint(), "world should be stopped");
    2.30 +
    2.31      _regionStack.push(mr);
    2.32      if (_regionStack.overflow()) {
    2.33        set_has_overflown();
    2.34 @@ -547,7 +561,33 @@
    2.35      }
    2.36      return true;
    2.37    }
    2.38 -  MemRegion region_stack_pop()          { return _regionStack.pop(); }
    2.39 +#if 0
    2.40 +  // Currently this is not used. See the comment in the .cpp file.
    2.41 +  MemRegion region_stack_pop() { return _regionStack.pop(); }
    2.42 +#endif // 0
    2.43 +
    2.44 +  bool region_stack_push_with_lock(MemRegion mr) {
    2.45 +    // Currently we only call the lock-based version during either
    2.46 +    // concurrent marking or remark.
    2.47 +    assert(!SafepointSynchronize::is_at_safepoint() || !concurrent(),
    2.48 +           "if we are at a safepoint it should be the remark safepoint");
    2.49 +
    2.50 +    _regionStack.push_with_lock(mr);
    2.51 +    if (_regionStack.overflow()) {
    2.52 +      set_has_overflown();
    2.53 +      return false;
    2.54 +    }
    2.55 +    return true;
    2.56 +  }
    2.57 +  MemRegion region_stack_pop_with_lock() {
    2.58 +    // Currently we only call the lock-based version during either
    2.59 +    // concurrent marking or remark.
    2.60 +    assert(!SafepointSynchronize::is_at_safepoint() || !concurrent(),
    2.61 +           "if we are at a safepoint it should be the remark safepoint");
    2.62 +
    2.63 +    return _regionStack.pop_with_lock();
    2.64 +  }
    2.65 +
    2.66    int region_stack_size()               { return _regionStack.size(); }
    2.67    bool region_stack_overflow()          { return _regionStack.overflow(); }
    2.68    bool region_stack_empty()             { return _regionStack.isEmpty(); }
     3.1 --- a/src/share/vm/runtime/mutexLocker.cpp	Fri Apr 02 12:10:08 2010 -0400
     3.2 +++ b/src/share/vm/runtime/mutexLocker.cpp	Mon Apr 05 12:19:22 2010 -0400
     3.3 @@ -70,6 +70,7 @@
     3.4  Monitor* CMark_lock                   = NULL;
     3.5  Monitor* ZF_mon                       = NULL;
     3.6  Monitor* Cleanup_mon                  = NULL;
     3.7 +Mutex*   CMRegionStack_lock           = NULL;
     3.8  Mutex*   SATB_Q_FL_lock               = NULL;
     3.9  Monitor* SATB_Q_CBL_mon               = NULL;
    3.10  Mutex*   Shared_SATB_Q_lock           = NULL;
    3.11 @@ -167,6 +168,7 @@
    3.12      def(CMark_lock                 , Monitor, nonleaf,     true ); // coordinate concurrent mark thread
    3.13      def(ZF_mon                     , Monitor, leaf,        true );
    3.14      def(Cleanup_mon                , Monitor, nonleaf,     true );
    3.15 +    def(CMRegionStack_lock         , Mutex,   leaf,        true );
    3.16      def(SATB_Q_FL_lock             , Mutex  , special,     true );
    3.17      def(SATB_Q_CBL_mon             , Monitor, nonleaf,     true );
    3.18      def(Shared_SATB_Q_lock         , Mutex,   nonleaf,     true );
     4.1 --- a/src/share/vm/runtime/mutexLocker.hpp	Fri Apr 02 12:10:08 2010 -0400
     4.2 +++ b/src/share/vm/runtime/mutexLocker.hpp	Mon Apr 05 12:19:22 2010 -0400
     4.3 @@ -63,6 +63,7 @@
     4.4  extern Monitor* CMark_lock;                      // used for concurrent mark thread coordination
     4.5  extern Monitor* ZF_mon;                          // used for G1 conc zero-fill.
     4.6  extern Monitor* Cleanup_mon;                     // used for G1 conc cleanup.
     4.7 +extern Mutex*   CMRegionStack_lock;              // used for protecting accesses to the CM region stack
     4.8  extern Mutex*   SATB_Q_FL_lock;                  // Protects SATB Q
     4.9                                                   // buffer free list.
    4.10  extern Monitor* SATB_Q_CBL_mon;                  // Protects SATB Q

mercurial