From 1728b8c7778d24e0daed7468f33dadf2f48c15ad Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Sun, 1 Mar 2020 21:39:19 -0500 Subject: [PATCH] kernel: Rework ConditionVariableEntry destruction. It is no longer an error to destroy a ConditionVariableEntry that is still attached to a ConditionVariable; it will now be implicitly detached in that case. This makes ConditionVariableEntrys much eaiser to use from an API standpoint. Change-Id: I03c676d3a198aa885de733d3e1729b15f80de031 Reviewed-on: https://review.haiku-os.org/c/haiku/+/2301 Reviewed-by: waddlesplash --- headers/private/kernel/condition_variable.h | 29 ++------ src/system/kernel/condition_variable.cpp | 67 ++++++++++++------- .../kernelland_emu/condition_variable.cpp | 37 +++++++--- 3 files changed, 74 insertions(+), 59 deletions(-) diff --git a/headers/private/kernel/condition_variable.h b/headers/private/kernel/condition_variable.h index 36b9dc0561..da327f76ef 100644 --- a/headers/private/kernel/condition_variable.h +++ b/headers/private/kernel/condition_variable.h @@ -23,10 +23,8 @@ struct ConditionVariable; struct ConditionVariableEntry : DoublyLinkedListLinkImpl { public: -#if KDEBUG - inline ConditionVariableEntry(); - inline ~ConditionVariableEntry(); -#endif + ConditionVariableEntry(); + ~ConditionVariableEntry(); bool Add(const void* object); status_t Wait(uint32 flags = 0, bigtime_t timeout = 0); @@ -38,7 +36,8 @@ public: inline ConditionVariable* Variable() const { return fVariable; } private: - inline void AddToLockedVariable(ConditionVariable* variable); + inline void _AddToLockedVariable(ConditionVariable* variable); + void _RemoveFromVariable(); private: spinlock fLock; @@ -100,26 +99,6 @@ protected: }; -#if KDEBUG - -inline -ConditionVariableEntry::ConditionVariableEntry() - : fVariable(NULL) -{ -} - -inline -ConditionVariableEntry::~ConditionVariableEntry() -{ - if (fVariable != NULL) { - panic("Destroying condition variable entry %p, but it's still " - "attached to variable %p\n", this, fVariable); - } -} - -#endif - - inline void ConditionVariable::NotifyOne(status_t result) { diff --git a/src/system/kernel/condition_variable.cpp b/src/system/kernel/condition_variable.cpp index 1f0c19a3be..c26d1526e6 100644 --- a/src/system/kernel/condition_variable.cpp +++ b/src/system/kernel/condition_variable.cpp @@ -90,6 +90,19 @@ dump_condition_variable(int argc, char** argv) // #pragma mark - ConditionVariableEntry +ConditionVariableEntry::ConditionVariableEntry() + : fVariable(NULL) +{ +} + + +ConditionVariableEntry::~ConditionVariableEntry() +{ + if (fVariable != NULL) + _RemoveFromVariable(); +} + + bool ConditionVariableEntry::Add(const void* object) { @@ -108,14 +121,14 @@ ConditionVariableEntry::Add(const void* object) SpinLocker variableLocker(variable->fLock); hashLocker.Unlock(); - AddToLockedVariable(variable); + _AddToLockedVariable(variable); return true; } inline void -ConditionVariableEntry::AddToLockedVariable(ConditionVariable* variable) +ConditionVariableEntry::_AddToLockedVariable(ConditionVariable* variable) { ASSERT(fVariable == NULL); @@ -127,6 +140,32 @@ ConditionVariableEntry::AddToLockedVariable(ConditionVariable* variable) } +void +ConditionVariableEntry::_RemoveFromVariable() +{ + InterruptsLocker _; + SpinLocker entryLocker(fLock); + + if (fVariable != NULL) { + SpinLocker conditionLocker(fVariable->fLock); + if (fVariable->fEntries.Contains(this)) { + fVariable->fEntries.Remove(this); + } else { + entryLocker.Unlock(); + // The variable's fEntries did not contain us, but we currently + // have the variable's lock acquired. This must mean we are in + // a race with the variable's Notify. It is possible we will be + // destroyed immediately upon returning here, so we need to + // spin until our fVariable member is unset by the Notify thread + // and then re-acquire our own lock to avoid a use-after-free. + while (atomic_pointer_get(&fVariable) != NULL) {} + entryLocker.Lock(); + } + fVariable = NULL; + } +} + + status_t ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout) { @@ -157,27 +196,7 @@ ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout) else error = thread_block(); - entryLocker.Lock(); - - // Remove entry from variable, if not done yet. - if (fVariable != NULL) { - SpinLocker conditionLocker(fVariable->fLock); - if (fVariable->fEntries.Contains(this)) { - fVariable->fEntries.Remove(this); - } else { - entryLocker.Unlock(); - // The variable's fEntries did not contain us, but we currently - // have the variable's lock acquired. This must mean we are in - // a race with the variable's Notify. It is possible we will be - // destroyed immediately upon returning here, so we need to - // spin until our fVariable member is unset by the Notify thread - // and then re-acquire our own lock to avoid a use-after-free. - while (atomic_pointer_get(&fVariable) != NULL) {} - entryLocker.Lock(); - } - fVariable = NULL; - } - + _RemoveFromVariable(); return error; } @@ -266,7 +285,7 @@ void ConditionVariable::Add(ConditionVariableEntry* entry) { InterruptsSpinLocker _(fLock); - entry->AddToLockedVariable(this); + entry->_AddToLockedVariable(this); } diff --git a/src/tests/add-ons/kernel/kernelland_emu/condition_variable.cpp b/src/tests/add-ons/kernel/kernelland_emu/condition_variable.cpp index fa1b3041bd..2d1cb7a303 100644 --- a/src/tests/add-ons/kernel/kernelland_emu/condition_variable.cpp +++ b/src/tests/add-ons/kernel/kernelland_emu/condition_variable.cpp @@ -53,6 +53,19 @@ static mutex sConditionVariablesLock = MUTEX_INITIALIZER("condition variables"); // #pragma mark - ConditionVariableEntry +ConditionVariableEntry::ConditionVariableEntry() + : fVariable(NULL) +{ +} + + +ConditionVariableEntry::~ConditionVariableEntry() +{ + if (fVariable != NULL) + _RemoveFromVariable(); +} + + bool ConditionVariableEntry::Add(const void* object) { @@ -95,14 +108,7 @@ ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout) while ((error = _kern_block_thread(flags, timeout)) == B_INTERRUPTED) { } - conditionLocker.Lock(); - - // remove entry from variable, if not done yet - if (fVariable != NULL) { - fVariable->fEntries.Remove(this); - fVariable = NULL; - } - + _RemoveFromVariable(); return error; } @@ -118,7 +124,7 @@ ConditionVariableEntry::Wait(const void* object, uint32 flags, inline void -ConditionVariableEntry::AddToLockedVariable(ConditionVariable* variable) +ConditionVariableEntry::_AddToLockedVariable(ConditionVariable* variable) { fThread = get_current_thread(); fVariable = variable; @@ -127,6 +133,17 @@ ConditionVariableEntry::AddToLockedVariable(ConditionVariable* variable) } +void +ConditionVariableEntry::_RemoveFromVariable() +{ + MutexLocker _(sConditionVariablesLock); + if (fVariable != NULL) { + fVariable->fEntries.Remove(this); + fVariable = NULL; + } +} + + // #pragma mark - ConditionVariable @@ -178,7 +195,7 @@ void ConditionVariable::Add(ConditionVariableEntry* entry) { MutexLocker _(sConditionVariablesLock); - entry->AddToLockedVariable(this); + entry->_AddToLockedVariable(this); }