From d012d06b0673c3da9d23b1816ff5a325576eb9b3 Mon Sep 17 00:00:00 2001 From: Carl Shapiro Date: Fri, 6 Aug 2010 15:17:46 -0700 Subject: [PATCH] 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. --- vm/alloc/HeapWorker.c | 33 +++++++++++++++++++++++++++------ 1 file changed, 27 insertions(+), 6 deletions(-) diff --git a/vm/alloc/HeapWorker.c b/vm/alloc/HeapWorker.c index f1d4169ae..42b894290 100644 --- a/vm/alloc/HeapWorker.c +++ b/vm/alloc/HeapWorker.c @@ -365,14 +365,23 @@ static void* heapWorkerThreadStart(void* arg) { 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 @@ static void* heapWorkerThreadStart(void* arg) 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. -- 2.11.0