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
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp Fri Apr 02 12:10:08 2010 -0400
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.cpp Mon Apr 05 12:19:22 2010 -0400
@@ -297,6 +297,11 @@
}
}
+// Currently we do not call this at all. Normally we would call it
+// during the concurrent marking / remark phases but we now call
+// the lock-based version instead. But we might want to resurrect this
+// code in the future. So, we'll leave it here commented out.
+#if 0
MemRegion CMRegionStack::pop() {
while (true) {
// Otherwise...
@@ -321,6 +326,41 @@
// Otherwise, we need to try again.
}
}
+#endif // 0
+
+void CMRegionStack::push_with_lock(MemRegion mr) {
+ assert(mr.word_size() > 0, "Precondition");
+ MutexLockerEx x(CMRegionStack_lock, Mutex::_no_safepoint_check_flag);
+
+ if (isFull()) {
+ _overflow = true;
+ return;
+ }
+
+ _base[_index] = mr;
+ _index += 1;
+}
+
+MemRegion CMRegionStack::pop_with_lock() {
+ MutexLockerEx x(CMRegionStack_lock, Mutex::_no_safepoint_check_flag);
+
+ while (true) {
+ if (_index == 0) {
+ return MemRegion();
+ }
+ _index -= 1;
+
+ MemRegion mr = _base[_index];
+ if (mr.start() != NULL) {
+ assert(mr.end() != NULL, "invariant");
+ assert(mr.word_size() > 0, "invariant");
+ return mr;
+ } else {
+ // that entry was invalidated... let's skip it
+ assert(mr.end() == NULL, "invariant");
+ }
+ }
+}
bool CMRegionStack::invalidate_entries_into_cset() {
bool result = false;
@@ -3363,7 +3403,7 @@
gclog_or_tty->print_cr("[%d] draining region stack, size = %d",
_task_id, _cm->region_stack_size());
- MemRegion mr = _cm->region_stack_pop();
+ MemRegion mr = _cm->region_stack_pop_with_lock();
// it returns MemRegion() if the pop fails
statsOnly(if (mr.start() != NULL) ++_region_stack_pops );
@@ -3384,7 +3424,7 @@
if (has_aborted())
mr = MemRegion();
else {
- mr = _cm->region_stack_pop();
+ mr = _cm->region_stack_pop_with_lock();
// it returns MemRegion() if the pop fails
statsOnly(if (mr.start() != NULL) ++_region_stack_pops );
}
@@ -3417,7 +3457,7 @@
}
// Now push the part of the region we didn't scan on the
// region stack to make sure a task scans it later.
- _cm->region_stack_push(newRegion);
+ _cm->region_stack_push_with_lock(newRegion);
}
// break from while
mr = MemRegion();
--- a/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp Fri Apr 02 12:10:08 2010 -0400
+++ b/hotspot/src/share/vm/gc_implementation/g1/concurrentMark.hpp Mon Apr 05 12:19:22 2010 -0400
@@ -252,9 +252,19 @@
// with other "push" operations (no pops).
void push(MemRegion mr);
+#if 0
+ // This is currently not used. See the comment in the .cpp file.
+
// Lock-free; assumes that it will only be called in parallel
// with other "pop" operations (no pushes).
MemRegion pop();
+#endif // 0
+
+ // These two are the implementations that use a lock. They can be
+ // called concurrently with each other but they should not be called
+ // concurrently with the lock-free versions (push() / pop()).
+ void push_with_lock(MemRegion mr);
+ MemRegion pop_with_lock();
bool isEmpty() { return _index == 0; }
bool isFull() { return _index == _capacity; }
@@ -540,6 +550,10 @@
// Manipulation of the region stack
bool region_stack_push(MemRegion mr) {
+ // Currently we only call the lock-free version during evacuation
+ // pauses.
+ assert(SafepointSynchronize::is_at_safepoint(), "world should be stopped");
+
_regionStack.push(mr);
if (_regionStack.overflow()) {
set_has_overflown();
@@ -547,7 +561,33 @@
}
return true;
}
- MemRegion region_stack_pop() { return _regionStack.pop(); }
+#if 0
+ // Currently this is not used. See the comment in the .cpp file.
+ MemRegion region_stack_pop() { return _regionStack.pop(); }
+#endif // 0
+
+ bool region_stack_push_with_lock(MemRegion mr) {
+ // Currently we only call the lock-based version during either
+ // concurrent marking or remark.
+ assert(!SafepointSynchronize::is_at_safepoint() || !concurrent(),
+ "if we are at a safepoint it should be the remark safepoint");
+
+ _regionStack.push_with_lock(mr);
+ if (_regionStack.overflow()) {
+ set_has_overflown();
+ return false;
+ }
+ return true;
+ }
+ MemRegion region_stack_pop_with_lock() {
+ // Currently we only call the lock-based version during either
+ // concurrent marking or remark.
+ assert(!SafepointSynchronize::is_at_safepoint() || !concurrent(),
+ "if we are at a safepoint it should be the remark safepoint");
+
+ return _regionStack.pop_with_lock();
+ }
+
int region_stack_size() { return _regionStack.size(); }
bool region_stack_overflow() { return _regionStack.overflow(); }
bool region_stack_empty() { return _regionStack.isEmpty(); }
--- a/hotspot/src/share/vm/runtime/mutexLocker.cpp Fri Apr 02 12:10:08 2010 -0400
+++ b/hotspot/src/share/vm/runtime/mutexLocker.cpp Mon Apr 05 12:19:22 2010 -0400
@@ -70,6 +70,7 @@
Monitor* CMark_lock = NULL;
Monitor* ZF_mon = NULL;
Monitor* Cleanup_mon = NULL;
+Mutex* CMRegionStack_lock = NULL;
Mutex* SATB_Q_FL_lock = NULL;
Monitor* SATB_Q_CBL_mon = NULL;
Mutex* Shared_SATB_Q_lock = NULL;
@@ -167,6 +168,7 @@
def(CMark_lock , Monitor, nonleaf, true ); // coordinate concurrent mark thread
def(ZF_mon , Monitor, leaf, true );
def(Cleanup_mon , Monitor, nonleaf, true );
+ def(CMRegionStack_lock , Mutex, leaf, true );
def(SATB_Q_FL_lock , Mutex , special, true );
def(SATB_Q_CBL_mon , Monitor, nonleaf, true );
def(Shared_SATB_Q_lock , Mutex, nonleaf, true );
--- a/hotspot/src/share/vm/runtime/mutexLocker.hpp Fri Apr 02 12:10:08 2010 -0400
+++ b/hotspot/src/share/vm/runtime/mutexLocker.hpp Mon Apr 05 12:19:22 2010 -0400
@@ -63,6 +63,7 @@
extern Monitor* CMark_lock; // used for concurrent mark thread coordination
extern Monitor* ZF_mon; // used for G1 conc zero-fill.
extern Monitor* Cleanup_mon; // used for G1 conc cleanup.
+extern Mutex* CMRegionStack_lock; // used for protecting accesses to the CM region stack
extern Mutex* SATB_Q_FL_lock; // Protects SATB Q
// buffer free list.
extern Monitor* SATB_Q_CBL_mon; // Protects SATB Q