kernel/condition_variable: Atomicize ConditionVariableEntry and drop the lock.

Before 2019, the entire ConditionVariable system was "giant"-locked:
that is, there was a single global lock that all ConditionVariable
and ConditionVariableEntry operations had to pass through. This of
course was not very performant on multicore systems and when
ConditionVariables see significant use, so I reworked it then to have
more granular locking.

Those patches took a number of attempts to get right, as having two
objects in separate threads that can each access the other not turn
into a deadlock or use-after-free is not easy to say the least,
and the ultimate solution I came up with erased most of the performance
gains I initially saw on the first (partially broken) patchsets.

So I have wanted to revisit this and see if there was a better way
even since then. Recently there have been a few reports of
ConditionVariable-related panics (apparently double unlocks),
notably #16894, and so that was reason enough to actually revisit
this code and see if a better solution could be found.

Well, I think I have come up with one: after this commit, Entries
no longer have their own lock, and instead accesses to Entry members
are almost always atomic; and there is now a case where we spin inside
Variable::_NotifyLocked as well as one in Entry::_RemoveFromVariable.

This leads to somewhat simpler code (no more lock/unlock dance in Notify),
though it is significantly more difficult to understand the nuances of it,
so I have left a sizable number of comments explaining the intricacies
of the new logic.

Note: I initially tried 1000 for "tries", but on a few instances I did see
the panic hit, strangely. I don't think the code that is waited on can
be reasonably reduced any further, so I have just increased the limit to
10000 (which is still well below what spinlocks use.) Hopefully this suffices.

Quick benchmark, x86, compiling HaikuDepot and the mime_db in VMware, 2 cores:
before:
real    0m23.627s
user    0m25.152s
sys     0m7.319s

after:
real    0m23.962s
user    0m25.229s
sys     0m7.330s

Though I occasionally I saw sys times as low as 7.171s, so this seems
to be at least not a regression if not a definitive improvement.

Change-Id: Id042947976885cd5c1433cc4290bdf41b01ed10e
Reviewed-on: https://review.haiku-os.org/c/haiku/+/4727
Tested-by: Commit checker robot <[email protected]>
Reviewed-by: Alex von Gluck IV <[email protected]>

headers/private/kernel/condition_variable.h

@@ -40,7 +40,6 @@ private:
 			void				_RemoveFromVariable();
 
 private:
-			spinlock			fLock;
 			ConditionVariable*	fVariable;
 			Thread*				fThread;
 			status_t			fWaitStatus;
@@ -92,6 +91,8 @@ protected:
 
 			spinlock			fLock;
 			EntryList			fEntries;
+			int32				fEntriesCount;
+
 			ConditionVariable*	fNext;
 
 			friend struct ConditionVariableEntry;

src/system/kernel/condition_variable.cpp

@@ -98,6 +98,8 @@ ConditionVariableEntry::ConditionVariableEntry()
 
 ConditionVariableEntry::~ConditionVariableEntry()
 {
+	// We can use an "unsafe" non-atomic access of fVariable here, since we only
+	// care whether it is non-NULL, not what its specific value is.
 	if (fVariable != NULL)
 		_RemoveFromVariable();
 }
@@ -132,37 +134,60 @@ 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);
+	atomic_add(&fVariable->fEntriesCount, 1);
 }
 
 
 void
 ConditionVariableEntry::_RemoveFromVariable()
 {
+	// This section is critical because it can race with _NotifyLocked on the
+	// variable's thread, so we must not be interrupted during it.
 	InterruptsLocker _;
-	SpinLocker entryLocker(fLock);
 
-	if (fVariable != NULL) {
+	ConditionVariable* variable = atomic_pointer_get(&fVariable);
-		SpinLocker conditionLocker(fVariable->fLock);
+	if (atomic_pointer_get_and_set(&fThread, (Thread*)NULL) == NULL) {
-		if (fVariable->fEntries.Contains(this)) {
+		// If fThread was already NULL, that means the variable is already
-			fVariable->fEntries.Remove(this);
+		// in the process of clearing us out (or already has finished doing so.)
-		} else {
+		// We thus cannot access fVariable, and must spin until it is cleared.
-			entryLocker.Unlock();
+		int32 tries = 0;
-			// The variable's fEntries did not contain us, but we currently
+		while (atomic_pointer_get(&fVariable) != NULL) {
-			// have the variable's lock acquired. This must mean we are in
+			tries++;
-			// a race with the variable's Notify. It is possible we will be
+			if ((tries % 10000) == 0)
-			// destroyed immediately upon returning here, so we need to
+				panic("variable pointer was not unset for a long time!");
-			// 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;
+
+		return;
 	}
+
+	while (true) {
+		if (atomic_pointer_get(&fVariable) == NULL) {
+			// The variable must have cleared us out. Acknowledge this and return.
+			atomic_add(&variable->fEntriesCount, -1);
+			return;
+		}
+
+		// There is of course a small race between checking the pointer and then
+		// the try_acquire in which the variable might clear out our fVariable.
+		// However, in the case where we were the ones to clear fThread, the
+		// variable will notice that and then wait for us to acknowledge the
+		// removal by decrementing fEntriesCount, as we do above; and until
+		// we do that, we may validly use our cached pointer to the variable.
+		if (try_acquire_spinlock(&variable->fLock))
+			break;
+	}
+
+	// We now hold the variable's lock. Remove ourselves.
+	if (fVariable->fEntries.Contains(this))
+		fVariable->fEntries.Remove(this);
+
+	atomic_pointer_set(&fVariable, (ConditionVariable*)NULL);
+	atomic_add(&variable->fEntriesCount, -1);
+	release_spinlock(&variable->fLock);
 }
 
 
@@ -177,18 +202,25 @@ ConditionVariableEntry::Wait(uint32 flags, bigtime_t timeout)
 	}
 #endif
 
+	// The race in-between get_and_set and (re)set is irrelevant, because
+	// if the status really is <= 0, we have already been or are about to
+	// be removed from the variable, and nothing else is going to set the status.
+	status_t waitStatus = atomic_get_and_set(&fWaitStatus, STATUS_WAITING);
+	if (waitStatus <= 0) {
+		fWaitStatus = waitStatus;
+		return waitStatus;
+	}
+
 	InterruptsLocker _;
-	SpinLocker entryLocker(fLock);
 
-	if (fVariable == NULL)
+	thread_prepare_to_block(thread_get_current_thread(), flags,
-		return fWaitStatus;
+		THREAD_BLOCK_TYPE_CONDITION_VARIABLE, atomic_pointer_get(&fVariable));
 
-	thread_prepare_to_block(fThread, flags,
+	waitStatus = atomic_get(&fWaitStatus);
-		THREAD_BLOCK_TYPE_CONDITION_VARIABLE, fVariable);
+	if (waitStatus <= 0) {
-
+		// We were just woken up! Unblock ourselves immediately.
-	fWaitStatus = STATUS_WAITING;
+		thread_unblock(thread_get_current_thread(), waitStatus);
-
+	}
-	entryLocker.Unlock();
 
 	status_t error;
 	if ((flags & (B_RELATIVE_TIMEOUT | B_ABSOLUTE_TIMEOUT)) != 0)
@@ -222,6 +254,7 @@ ConditionVariable::Init(const void* object, const char* objectType)
 	fObject = object;
 	fObjectType = objectType;
 	new(&fEntries) EntryList;
+	fEntriesCount = 0;
 	B_INITIALIZE_SPINLOCK(&fLock);
 
 	T_SCHEDULING_ANALYSIS(InitConditionVariable(this, object, objectType));
@@ -238,6 +271,7 @@ ConditionVariable::Publish(const void* object, const char* objectType)
 	fObject = object;
 	fObjectType = objectType;
 	new(&fEntries) EntryList;
+	fEntriesCount = 0;
 	B_INITIALIZE_SPINLOCK(&fLock);
 
 	T_SCHEDULING_ANALYSIS(InitConditionVariable(this, object, objectType));
@@ -346,31 +380,38 @@ void
 ConditionVariable::_NotifyLocked(bool all, status_t result)
 {
 	// 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);
+		Thread* thread = atomic_pointer_get_and_set(&entry->fThread, (Thread*)NULL);
-		acquire_spinlock(&entry->fLock);
+		if (thread == NULL) {
+			// The entry must be in the process of trying to remove itself from us.
+			// Clear its variable and wait for it to acknowledge this in fEntriesCount,
+			// as it is the one responsible for decrementing that.
+			const int32 oldCount = atomic_get(&fEntriesCount);
+			atomic_pointer_set(&entry->fVariable, (ConditionVariable*)NULL);
 
-		entry->fVariable = NULL;
+			// As fEntriesCount is only modified while our lock is held, nothing else
+			// will modify it while we are spinning, since we hold it at present.
+			int32 tries = 0;
+			while (atomic_get(&fEntriesCount) == oldCount) {
+				tries++;
+				if ((tries % 10000) == 0)
+					panic("entries count was not decremented for a long time!");
+			}
+		} else {
+			const status_t waitStatus = atomic_get_and_set(&entry->fWaitStatus, result);
 
-		if (entry->fWaitStatus <= 0) {
+			// No matter what the thread is doing, as we were the ones to clear its
-			release_spinlock(&entry->fLock);
+			// fThread, so we are the ones responsible for decrementing fEntriesCount.
-			acquire_spinlock(&fLock);
+			// (We may not validly access the entry once we unset its fVariable.)
-			continue;
+			atomic_pointer_set(&entry->fVariable, (ConditionVariable*)NULL);
+			atomic_add(&fEntriesCount, -1);
+
+			// Do this after unsetting fVariable, as in case the entry wakes up
+			// and tries to remove itself, it need not not have to wait for us.
+			if (waitStatus == STATUS_WAITING)
+				thread_unblock(thread, result);
 		}
 
-		if (entry->fWaitStatus == STATUS_WAITING) {
-			SpinLocker _(entry->fThread->scheduler_lock);
-			thread_unblock_locked(entry->fThread, result);
-		}
-
-		entry->fWaitStatus = result;
-
-		release_spinlock(&entry->fLock);
-		acquire_spinlock(&fLock);
-
 		if (!all)
 			break;
 	}