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; }