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 <[email protected]>

headers/private/kernel/condition_variable.h

@@ -23,10 +23,8 @@ struct ConditionVariable;
 struct ConditionVariableEntry
 	: DoublyLinkedListLinkImpl<ConditionVariableEntry> {
 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)
 {

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

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