Fix the ordering of lock acquisition in the heap worker.
A feature of the concurrent GC is that gcHeapLock is released while
roots are traced through. This complicates the heap worker thread as
it assumes that when the gcHeapLock can be acquired no threads are
contending for the heapWorkerLock. However, the concurrent GC holds
heapWorkerLock for the duration of a GC. If the heap worker thread
becomes active while the GC temporarily release gcHeapLock it may
deadlock the GC by acquring the gcHeapLock, needed by the GC during
its final pause, and wait for the heapWorkerLock held by the GC.
This change attempts to resolve this issue by checking to see if after
a transition into running whether the GC has become active. If so, it
releases gcHeapLock and reacquires it after the GC has signaled
completion. This must be done in a loop as there is no guarantee that
the GC has not become active when heap worker is rescheduled.
diff --git a/vm/alloc/HeapWorker.c b/vm/alloc/HeapWorker.c
index f1d4169..42b8942 100644
--- a/vm/alloc/HeapWorker.c
+++ b/vm/alloc/HeapWorker.c
@@ -365,14 +365,23 @@
{
size_t madvisedSizes[HEAP_SOURCE_MAX_HEAP_COUNT];
- /* The heap must be locked before the HeapWorker;
- * unroll and re-order the locks. dvmLockHeap()
- * will put us in VMWAIT if necessary. Once it
- * returns, there shouldn't be any contention on
- * heapWorkerLock.
+ /*
+ * Acquire the gcHeapLock. The requires releasing the
+ * heapWorkerLock before the gcHeapLock is acquired.
+ * It is possible that the gcHeapLock may be acquired
+ * during a concurrent GC in which case heapWorkerLock
+ * is held by the GC and we are unable to make forward
+ * progress. We avoid deadlock by releasing the
+ * gcHeapLock and then waiting to be signaled when the
+ * GC completes. There is no guarantee that the next
+ * time we are run will coincide with GC inactivity so
+ * the check and wait must be performed within a loop.
*/
dvmUnlockMutex(&gDvm.heapWorkerLock);
dvmLockHeap();
+ while (gDvm.gcHeap->gcRunning) {
+ dvmWaitForConcurrentGcToComplete();
+ }
dvmLockMutex(&gDvm.heapWorkerLock);
memset(madvisedSizes, 0, sizeof(madvisedSizes));
@@ -404,11 +413,23 @@
dvmWaitCond(&gDvm.heapWorkerCond, &gDvm.heapWorkerLock);
}
- /* dvmChangeStatus() may block; don't hold heapWorkerLock.
+ /*
+ * Return to the running state before doing heap work. This
+ * will block if the GC has initiated a suspend. We release
+ * the heapWorkerLock beforehand for the GC to make progress
+ * and wait to be signaled after the GC completes. There is
+ * no guarantee that the next time we are run will coincide
+ * with GC inactivity so the check and wait must be performed
+ * within a loop.
*/
dvmUnlockMutex(&gDvm.heapWorkerLock);
dvmChangeStatus(NULL, THREAD_RUNNING);
+ dvmLockHeap();
+ while (gDvm.gcHeap->gcRunning) {
+ dvmWaitForConcurrentGcToComplete();
+ }
dvmLockMutex(&gDvm.heapWorkerLock);
+ dvmUnlockHeap();
LOGV("HeapWorker is awake\n");
/* Process any events in the queue.
