kernel/user_mutex: Refactor around ConditionVariable features.

* Get rid of the multiple entries/condition-variables system.
   Instead, allocate one structure per variable and do not add/remove
   it from the hash until all waiters are gone.

 * Get rid of "locked". All wait wakeups of B_OK mean "locked". All
   nonzero values mean "not locked." This mirrors what the kernel mutex
   implementation does (however, that also tracks the owning thread,
   for assertion's sake.)

 * Remove "lastWaiter" logic (for now.) As we no longer hold a lock
   after wakeup, we cannot reliably check and act on it outside the
   "wait" function. This means that interrupted or timed-out waits
   will cause a potentially unnecessary syscall on next unblock,
   but that will be resolved in the next commit.

Due to the single global lock, user mutex acquisition is an extremely
"noisy" process that can take shorter or longer depending on what is
going on elsewhere on the system, so performance is hard to measure.

With one benchmark that acquires mutexes as fast as possible with
lots of contention, most runs came in as being around the same amount
of time both before and after this change (around 4.25s real). Moving
Terminal's window around while running the test caused runtime to go
up to around 6.7s before this change, and about 7.0s after.

GLTeapot seems to go from 350-380 FPS before this change and 320-340
after. It still spends the vast majority of its time waiting for address
space and cache locks, however.

It is expected that the next commits will build on this change to
improve performance beyond even the "before" numbers above.

Change-Id: I6581a6f7cb0ca0513ea639f8499a1c0c8596c026
Reviewed-on: https://review.haiku-os.org/c/haiku/+/6490
Reviewed-by: waddlesplash <[email protected]>
Reviewed-by: Adrien Destugues <[email protected]>
Tested-by: Commit checker robot <[email protected]>

src/system/kernel/locks/user_mutex.cpp

@@ -20,15 +20,12 @@
 #include <vm/VMArea.h>
 
 
-struct UserMutexEntry;
+struct UserMutexEntry {
-typedef DoublyLinkedList<UserMutexEntry> UserMutexEntryList;
-
-struct UserMutexEntry : public DoublyLinkedListLinkImpl<UserMutexEntry> {
 	phys_addr_t			address;
+	UserMutexEntry*		hash_next;
+	int32				ref_count;
+
 	ConditionVariable	condition;
-	bool				locked;
-	UserMutexEntryList	otherEntries;
-	UserMutexEntry*		hashNext;
 };
 
 struct UserMutexHashDefinition {
@@ -52,7 +49,7 @@ struct UserMutexHashDefinition {
 
 	UserMutexEntry*& GetLink(UserMutexEntry* value) const
 	{
-		return value->hashNext;
+		return value->hash_next;
 	}
 };
 
@@ -109,68 +106,68 @@ user_atomic_test_and_set(int32* value, int32 newValue, int32 testAgainst)
 // #pragma mark - user mutex entries
 
 
-static void
+static UserMutexEntry*
-add_user_mutex_entry(UserMutexEntry* entry)
+get_user_mutex_entry(phys_addr_t address)
 {
-	UserMutexEntry* firstEntry = sUserMutexTable.Lookup(entry->address);
+	UserMutexEntry* entry = sUserMutexTable.Lookup(address);
-	if (firstEntry != NULL)
+	if (entry != NULL) {
-		firstEntry->otherEntries.Add(entry);
+		atomic_add(&entry->ref_count, 1);
-	else
+		return entry;
+	}
+
+	entry = new(std::nothrow) UserMutexEntry;
+	if (entry == NULL)
+		return entry;
+
+	entry->address = address;
+	entry->ref_count = 1;
+	entry->condition.Init(entry, "UserMutexEntry");
+
 	sUserMutexTable.Insert(entry);
+	return entry;
 }
 
 
-static bool
+static void
-remove_user_mutex_entry(UserMutexEntry* entry)
+put_user_mutex_entry(UserMutexEntry* entry, MutexLocker& locker)
 {
-	UserMutexEntry* firstEntry = sUserMutexTable.Lookup(entry->address);
+	const phys_addr_t address = entry->address;
-	if (firstEntry != entry) {
+	if (atomic_add(&entry->ref_count, -1) != 1)
-		// The entry is not the first entry in the table. Just remove it from
+		return;
-		// the first entry's list.
+
-		firstEntry->otherEntries.Remove(entry);
+	locker.Lock();
-		return true;
+
-	}
+	// Was it removed & deleted while we were waiting for the lock?
+	if (sUserMutexTable.Lookup(address) != entry)
+		return;
+
+	// Or did someone else acquire a reference to it?
+	if (atomic_get(&entry->ref_count) > 0)
+		return;
 
-	// The entry is the first entry in the table. Remove it from the table and,
-	// if any, add the next entry to the table.
 	sUserMutexTable.Remove(entry);
-
+	delete entry;
-	firstEntry = entry->otherEntries.RemoveHead();
-	if (firstEntry != NULL) {
-		firstEntry->otherEntries.MoveFrom(&entry->otherEntries);
-		sUserMutexTable.Insert(firstEntry);
-		return true;
-	}
-
-	return false;
 }
 
 
 static status_t
 user_mutex_wait_locked(int32* mutex, phys_addr_t physicalAddress, const char* name,
-	uint32 flags, bigtime_t timeout, MutexLocker& locker, bool& lastWaiter)
+	uint32 flags, bigtime_t timeout, MutexLocker& locker)
 {
-	// add the entry to the table
+	// add or get the entry from the table
-	UserMutexEntry entry;
+	UserMutexEntry* entry = get_user_mutex_entry(physicalAddress);
-	entry.address = physicalAddress;
+	if (entry == NULL)
-	entry.locked = false;
+		return B_NO_MEMORY;
-	add_user_mutex_entry(&entry);
 
 	// wait
-	entry.condition.Init((void*)physicalAddress, "user mutex");
+	ConditionVariableEntry waiter;
-	status_t error = entry.condition.Wait(locker.Get(), flags, timeout);
+	entry->condition.Add(&waiter);
+	locker.Unlock();
 
-	if (error != B_OK && entry.locked)
+	status_t error = waiter.Wait(flags, timeout);
-		error = B_OK;
 
-	if (!entry.locked) {
+	// this will re-lock only if necessary
-		// if nobody woke us up, we have to dequeue ourselves
+	put_user_mutex_entry(entry, locker);
-		lastWaiter = !remove_user_mutex_entry(&entry);
-	} else {
-		// otherwise the waker has done the work of marking the
-		// mutex or semaphore uncontended
-		lastWaiter = false;
-	}
 
 	return error;
 }
@@ -200,14 +197,8 @@ user_mutex_lock_locked(int32* mutex, phys_addr_t physicalAddress,
 	if (user_mutex_prepare_to_lock(mutex))
 		return B_OK;
 
-	bool lastWaiter;
+	return user_mutex_wait_locked(mutex, physicalAddress, name,
-	status_t error = user_mutex_wait_locked(mutex, physicalAddress, name,
+		flags, timeout, locker);
-		flags, timeout, locker, lastWaiter);
-
-	if (lastWaiter)
-		user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING);
-
-	return error;
 }
 
 
@@ -221,37 +212,24 @@ user_mutex_unblock_locked(int32* mutex, phys_addr_t physicalAddress, uint32 flag
 		return;
 	}
 
-	// Someone is waiting: try to hand off the lock to them, if possible.
 	int32 oldValue = 0;
 	if ((flags & B_USER_MUTEX_UNBLOCK_ALL) == 0) {
+		// This is not merely an unblock, but a hand-off.
 		oldValue = user_atomic_or(mutex, B_USER_MUTEX_LOCKED);
 		if ((oldValue & B_USER_MUTEX_LOCKED) != 0)
 			return;
-	} else {
-		oldValue = user_atomic_get(mutex);
 	}
 
-	// unblock the first thread
-	entry->locked = true;
-	entry->condition.NotifyOne();
-
 	if ((flags & B_USER_MUTEX_UNBLOCK_ALL) != 0
 			|| (oldValue & B_USER_MUTEX_DISABLED) != 0) {
-		// unblock and dequeue all the other waiting threads as well
+		// unblock and dequeue all the waiting threads
-		while (UserMutexEntry* otherEntry = entry->otherEntries.RemoveHead()) {
+		entry->condition.NotifyAll(B_OK);
-			otherEntry->locked = true;
+	} else {
-			otherEntry->condition.NotifyOne();
+		entry->condition.NotifyOne(B_OK);
 	}
 
-		// dequeue the first thread and mark the mutex uncontended
+	if (entry->condition.EntriesCount() == 0)
-		sUserMutexTable.Remove(entry);
 		user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING);
-	} else {
-		bool otherWaiters = remove_user_mutex_entry(entry);
-		if (!otherWaiters) {
-			user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING);
-		}
-	}
 }
 
 
@@ -269,14 +247,8 @@ user_mutex_sem_acquire_locked(int32* sem, phys_addr_t physicalAddress,
 		oldValue = value;
 	}
 
-	bool lastWaiter;
+	return user_mutex_wait_locked(sem, physicalAddress, name, flags,
-	status_t error = user_mutex_wait_locked(sem, physicalAddress, name, flags,
+		timeout, locker);
-		timeout, locker, lastWaiter);
-
-	if (lastWaiter)
-		user_atomic_test_and_set(sem, 0, -1);
-
-	return error;
 }
 
 
@@ -296,12 +268,8 @@ user_mutex_sem_release_locked(int32* sem, phys_addr_t physicalAddress)
 		}
 	}
 
-	bool otherWaiters = remove_user_mutex_entry(entry);
+	entry->condition.NotifyOne(B_OK);
-
+	if (entry->condition.EntriesCount() == 0) {
-	entry->locked = true;
-	entry->condition.NotifyOne();
-
-	if (!otherWaiters) {
 		// mark the semaphore uncontended
 		user_atomic_test_and_set(sem, 0, -1);
 	}