From 1700e825b1249917f478aca067bf7b84c3642fa3 Mon Sep 17 00:00:00 2001 From: Pawel Dziepak Date: Thu, 6 Mar 2014 01:13:31 +0100 Subject: [PATCH] scheduler: Always protect gCore[High]LoadHeap with spinlock Should fix #10628. If there is a race condition with a writer getting minimum or maximum from double ended heap may incorrectly result NULL. Which is not expected in the most of the thread migration logic. Apart from that, because of the race condition heap state may be observed as inconsistent thus failing assertions. ended heap --- src/system/kernel/scheduler/low_latency.cpp | 27 +++++++++----------- src/system/kernel/scheduler/power_saving.cpp | 3 +++ 2 files changed, 15 insertions(+), 15 deletions(-) diff --git a/src/system/kernel/scheduler/low_latency.cpp b/src/system/kernel/scheduler/low_latency.cpp index 911353f2a1..af286997e2 100644 --- a/src/system/kernel/scheduler/low_latency.cpp +++ b/src/system/kernel/scheduler/low_latency.cpp @@ -87,29 +87,26 @@ should_rebalance(const ThreadData* threadData) int32 coreNewLoad = coreLoad - threadLoad; - // If there is high load on this core but this thread does not contribute - // significantly consider giving it to someone less busy. - if (coreLoad > kHighLoad) { - CoreEntry* other = gCoreLoadHeap.PeekMinimum(); - if (other != NULL) { - int32 otherNewLoad = other->GetLoad() + threadLoad; - if (coreNewLoad - otherNewLoad >= kLoadDifference) - return true; - } - } - - // No cpu bound threads - the situation is quite good. Make sure it - // won't get much worse... ReadSpinLocker coreLocker(gCoreHeapsLock); CoreEntry* other = gCoreLoadHeap.PeekMinimum(); if (other == NULL) other = gCoreHighLoadHeap.PeekMinimum(); coreLocker.Unlock(); + ASSERT(other != NULL); + int32 otherNewLoad = other->GetLoad() + threadLoad; + + // If there is high load on this core but this thread does not contribute + // significantly consider giving it to someone less busy. + if (coreLoad > kHighLoad) { + if (coreNewLoad - otherNewLoad >= kLoadDifference) + return true; + } + + // No cpu bound threads - the situation is quite good. Make sure it + // won't get much worse... if (other->GetLoad() == 0 && coreNewLoad != 0) return true; - - int32 otherNewLoad = other->GetLoad() + threadLoad; return coreNewLoad - otherNewLoad >= kLoadDifference * 2; } diff --git a/src/system/kernel/scheduler/power_saving.cpp b/src/system/kernel/scheduler/power_saving.cpp index 54c2db1795..f9b4b62fca 100644 --- a/src/system/kernel/scheduler/power_saving.cpp +++ b/src/system/kernel/scheduler/power_saving.cpp @@ -52,6 +52,7 @@ choose_small_task_core() { SCHEDULER_ENTER_FUNCTION(); + ReadSpinLocker coreLocker(gCoreHeapsLock); CoreEntry* core = gCoreLoadHeap.PeekMaximum(); if (core == NULL) return sSmallTaskCore; @@ -137,9 +138,11 @@ should_rebalance(const ThreadData* threadData) if (threadLoad >= coreLoad / 2) return false; + ReadSpinLocker coreLocker(gCoreHeapsLock); CoreEntry* other = gCoreLoadHeap.PeekMaximum(); if (other == NULL) other = gCoreHighLoadHeap.PeekMinimum(); + coreLocker.Unlock(); ASSERT(other != NULL); int32 coreNewLoad = coreLoad - threadLoad;