Change suspend order in GC
The GC currently does this:
1. acquire heapLock
2. suspend all threads
3. acquire heapWorkerLock
When the HeapWorker thread is suspended in #2, it might be holding
the lock we want in step #3, leading to VM deadlock. This change
reverses the order of #2 and #3, which should allow the HeapWorker
thread to progress to a point where it releases the lock before
the GC requests the suspension.
If futexes are being unfair, the GC might have to wait a bit
longer while the HeapWorker does stuff (it might unlock, do work,
and re-lock without the scheduler giving time to the GC thread;
with the old scheme the HeapWorker would see the pending thread
suspension immediately and stop). A better fix is planned.
Bug 3340837
Change-Id: Ib9b37c130903a2800f8f28ae61540a428dbfc5be
diff --git a/vm/alloc/Heap.c b/vm/alloc/Heap.c
index 39f92b5..8e8dc7f 100644
--- a/vm/alloc/Heap.c
+++ b/vm/alloc/Heap.c
@@ -549,6 +549,13 @@
gcMode = (reason == GC_FOR_MALLOC) ? GC_PARTIAL : GC_FULL;
gcHeap->gcRunning = true;
+ /*
+ * Grab the heapWorkerLock to prevent the HeapWorker thread from
+ * doing work. If it's executing a finalizer or an enqueue operation
+ * it won't be holding the lock, so this should return quickly.
+ */
+ dvmLockMutex(&gDvm.heapWorkerLock);
+
rootSuspend = dvmGetRelativeTimeMsec();
dvmSuspendAllThreads(SUSPEND_FOR_GC);
rootStart = dvmGetRelativeTimeMsec();
@@ -588,12 +595,6 @@
}
}
- /* Wait for the HeapWorker thread to block.
- * (It may also already be suspended in interp code,
- * in which case it's not holding heapWorkerLock.)
- */
- dvmLockMutex(&gDvm.heapWorkerLock);
-
/* Make sure that the HeapWorker thread hasn't become
* wedged inside interp code. If it has, this call will
* print a message and abort the VM.
@@ -828,6 +829,26 @@
}
}
+/*
+ * If the concurrent GC is running, wait for it to finish. The caller
+ * must hold the heap lock.
+ *
+ * Note: the second dvmChangeStatus() could stall if we were in RUNNING
+ * on entry, and some other thread has asked us to suspend. In that
+ * case we will be suspended with the heap lock held, which can lead to
+ * deadlock if the other thread tries to do something with the managed heap.
+ * For example, the debugger might suspend us and then execute a method that
+ * allocates memory. We can avoid this situation by releasing the lock
+ * before self-suspending. (The developer can work around this specific
+ * situation by single-stepping the VM. Alternatively, we could disable
+ * concurrent GC when the debugger is attached, but that might change
+ * behavior more than is desirable.)
+ *
+ * This should not be a problem in production, because any GC-related
+ * activity will grab the lock before issuing a suspend-all. (We may briefly
+ * suspend when the GC thread calls dvmUnlockHeap before dvmResumeAllThreads,
+ * but there's no risk of deadlock.)
+ */
void dvmWaitForConcurrentGcToComplete(void)
{
Thread *self = dvmThreadSelf();