Mon, 05 Apr 2010 12:19:22 -0400
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
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