From 37eda488be1c9fee242e8e4bf6ca644dd13441d8 Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Sat, 3 Aug 2019 11:22:49 -0400 Subject: [PATCH] kernel/condition_variable: Granularize locking. Before this commit, *all* ConditionVariable operations (yes, all; even Wait, Notify, etc.) went through a single spinlock, that also protected the sConditionVariableHash. This obviously does not scale so well with core count, to say the least! With this commit, we add spinlocks to each Variable and Entry. This makes locking somewhat more complicated (and nuanced; see inline comment), but the trade-off seems completely worth it: (compile HaikuDepot in VMware, 2 cores) before real 1m20.219s user 1m5.619s sys 0m40.724s after real 1m12.667s user 0m57.684s sys 0m37.251s The more cores there are, the more of an optimization this will likely prove to be. But 10%-across-the-board is not bad to say the least. Change-Id: I1e40a997fff58a79e987d7cdcafa8f7358e1115a --- headers/private/kernel/condition_variable.h | 6 +- src/system/kernel/condition_variable.cpp | 99 +++++++++++++-------- 2 files changed, 67 insertions(+), 38 deletions(-) diff --git a/headers/private/kernel/condition_variable.h b/headers/private/kernel/condition_variable.h index 4e91baec22..36b9dc0561 100644 --- a/headers/private/kernel/condition_variable.h +++ b/headers/private/kernel/condition_variable.h @@ -1,5 +1,6 @@ /* * Copyright 2007-2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2019, Haiku, Inc. All rights reserved. * Distributed under the terms of the MIT License. */ #ifndef _KERNEL_CONDITION_VARIABLE_H @@ -37,9 +38,10 @@ public: inline ConditionVariable* Variable() const { return fVariable; } private: - inline void AddToVariable(ConditionVariable* variable); + inline void AddToLockedVariable(ConditionVariable* variable); private: + spinlock fLock; ConditionVariable* fVariable; Thread* fThread; status_t fWaitStatus; @@ -88,6 +90,8 @@ protected: const void* fObject; const char* fObjectType; + + spinlock fLock; EntryList fEntries; ConditionVariable* fNext; diff --git a/src/system/kernel/condition_variable.cpp b/src/system/kernel/condition_variable.cpp index b3defeecff..16ac6b809c 100644 --- a/src/system/kernel/condition_variable.cpp +++ b/src/system/kernel/condition_variable.cpp @@ -1,5 +1,6 @@ /* * Copyright 2007-2011, Ingo Weinhold, ingo_weinhold@gmx.de. + * Copyright 2019, Haiku, Inc. All rights reserved. * Distributed under the terms of the MIT License. */ @@ -42,7 +43,7 @@ struct ConditionVariableHashDefinition { typedef BOpenHashTable ConditionVariableHash; static ConditionVariableHash sConditionVariableHash; -static spinlock sConditionVariablesLock; +static rw_spinlock sConditionVariableHashLock; static int @@ -93,46 +94,64 @@ ConditionVariableEntry::Add(const void* object) { ASSERT(object != NULL); - fThread = thread_get_current_thread(); + InterruptsLocker _; + ReadSpinLocker hashLocker(sConditionVariableHashLock); - InterruptsSpinLocker _(sConditionVariablesLock); + ConditionVariable* variable = sConditionVariableHash.Lookup(object); - fVariable = sConditionVariableHash.Lookup(object); - - if (fVariable == NULL) { + if (variable == NULL) { fWaitStatus = B_ENTRY_NOT_FOUND; return false; } - fWaitStatus = STATUS_ADDED; - fVariable->fEntries.Add(this); + SpinLocker variableLocker(variable->fLock); + hashLocker.Unlock(); + + AddToLockedVariable(variable); return true; } +inline void +ConditionVariableEntry::AddToLockedVariable(ConditionVariable* variable) +{ + ASSERT(fVariable == NULL); + + B_INITIALIZE_SPINLOCK(&fLock); + fThread = thread_get_current_thread(); + fVariable = variable; + fWaitStatus = STATUS_ADDED; + fVariable->fEntries.Add(this); +} + + status_t ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout) { +#if KDEBUG if (!are_interrupts_enabled()) { panic("ConditionVariableEntry::Wait() called with interrupts " "disabled, entry: %p, variable: %p", this, fVariable); return B_ERROR; } +#endif InterruptsLocker _; - - SpinLocker conditionLocker(sConditionVariablesLock); + SpinLocker entryLocker(fLock); if (fVariable == NULL) return fWaitStatus; + SpinLocker conditionLocker(fVariable->fLock); + thread_prepare_to_block(fThread, flags, THREAD_BLOCK_TYPE_CONDITION_VARIABLE, fVariable); fWaitStatus = STATUS_WAITING; conditionLocker.Unlock(); + entryLocker.Unlock(); status_t error; if ((flags & (B_RELATIVE_TIMEOUT | B_ABSOLUTE_TIMEOUT)) != 0) @@ -140,11 +159,15 @@ ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout) else error = thread_block(); - conditionLocker.Lock(); + entryLocker.Lock(); // remove entry from variable, if not done yet if (fVariable != NULL) { - fVariable->fEntries.Remove(this); + conditionLocker.Lock(); + // we need to check we are still in the variable's entries; there may + // have been a race for our removal. + if (fVariable->fEntries.Contains(this)) + fVariable->fEntries.Remove(this); fVariable = NULL; } @@ -162,19 +185,6 @@ ConditionVariableEntry::Wait(const void* object, uint32 flags, } -inline void -ConditionVariableEntry::AddToVariable(ConditionVariable* variable) -{ - fThread = thread_get_current_thread(); - - InterruptsSpinLocker _(sConditionVariablesLock); - - fVariable = variable; - fWaitStatus = STATUS_ADDED; - fVariable->fEntries.Add(this); -} - - // #pragma mark - ConditionVariable @@ -186,6 +196,7 @@ ConditionVariable::Init(const void* object, const char* objectType) fObject = object; fObjectType = objectType; new(&fEntries) EntryList; + B_INITIALIZE_SPINLOCK(&fLock); T_SCHEDULING_ANALYSIS(InitConditionVariable(this, object, objectType)); NotifyWaitObjectListeners(&WaitObjectListener::ConditionVariableInitialized, @@ -201,13 +212,13 @@ ConditionVariable::Publish(const void* object, const char* objectType) fObject = object; fObjectType = objectType; new(&fEntries) EntryList; + B_INITIALIZE_SPINLOCK(&fLock); T_SCHEDULING_ANALYSIS(InitConditionVariable(this, object, objectType)); NotifyWaitObjectListeners(&WaitObjectListener::ConditionVariableInitialized, this); - InterruptsLocker _; - SpinLocker locker(sConditionVariablesLock); + InterruptsWriteSpinLocker _(sConditionVariableHashLock); ASSERT_PRINT(sConditionVariableHash.Lookup(object) == NULL, "condition variable: %p\n", sConditionVariableHash.Lookup(object)); @@ -221,7 +232,9 @@ ConditionVariable::Unpublish() { ASSERT(fObject != NULL); - InterruptsSpinLocker locker(sConditionVariablesLock); + InterruptsLocker _; + WriteSpinLocker hashLocker(sConditionVariableHashLock); + SpinLocker selfLocker(fLock); #if KDEBUG ConditionVariable* variable = sConditionVariableHash.Lookup(fObject); @@ -235,6 +248,8 @@ ConditionVariable::Unpublish() fObject = NULL; fObjectType = NULL; + hashLocker.Unlock(); + if (!fEntries.IsEmpty()) _NotifyLocked(true, B_ENTRY_NOT_FOUND); } @@ -243,7 +258,8 @@ ConditionVariable::Unpublish() void ConditionVariable::Add(ConditionVariableEntry* entry) { - entry->AddToVariable(this); + InterruptsSpinLocker _(fLock); + entry->AddToLockedVariable(this); } @@ -259,7 +275,7 @@ ConditionVariable::Wait(uint32 flags, bigtime_t timeout) /*static*/ void ConditionVariable::NotifyOne(const void* object, status_t result) { - InterruptsSpinLocker locker(sConditionVariablesLock); + InterruptsReadSpinLocker locker(sConditionVariableHashLock); ConditionVariable* variable = sConditionVariableHash.Lookup(object); locker.Unlock(); if (variable == NULL) @@ -272,7 +288,7 @@ ConditionVariable::NotifyOne(const void* object, status_t result) /*static*/ void ConditionVariable::NotifyAll(const void* object, status_t result) { - InterruptsSpinLocker locker(sConditionVariablesLock); + InterruptsReadSpinLocker locker(sConditionVariableHashLock); ConditionVariable* variable = sConditionVariableHash.Lookup(object); locker.Unlock(); if (variable == NULL) @@ -285,7 +301,7 @@ ConditionVariable::NotifyAll(const void* object, status_t result) void ConditionVariable::_Notify(bool all, status_t result) { - InterruptsSpinLocker locker(sConditionVariablesLock); + InterruptsSpinLocker _(fLock); if (!fEntries.IsEmpty()) { if (result > B_OK) { @@ -298,18 +314,25 @@ ConditionVariable::_Notify(bool all, status_t result) } -/*! Called with interrupts disabled and the condition variable spinlock and - scheduler lock held. -*/ +/*! Called with interrupts disabled and the condition variable's spinlock held. + */ void ConditionVariable::_NotifyLocked(bool all, status_t result) { - // dequeue and wake up the blocked threads + // Dequeue and wake up the blocked threads. + // We *cannot* hold our own lock while acquiring the Entry's lock, + // as this leads to a (non-theoretical!) race between the Entry + // entering Wait() and acquiring its own lock, and then acquiring ours. while (ConditionVariableEntry* entry = fEntries.RemoveHead()) { + release_spinlock(&fLock); + + SpinLocker _(entry->fLock); entry->fVariable = NULL; - if (entry->fWaitStatus <= 0) + if (entry->fWaitStatus <= 0) { + acquire_spinlock(&fLock); continue; + } if (entry->fWaitStatus == STATUS_WAITING) { SpinLocker _(entry->fThread->scheduler_lock); @@ -318,6 +341,8 @@ ConditionVariable::_NotifyLocked(bool all, status_t result) entry->fWaitStatus = result; + acquire_spinlock(&fLock); + if (!all) break; }