From 0ab9f280ec94e78ae21971b15bdfc1e15030689e Mon Sep 17 00:00:00 2001 From: Augustin Cavalier Date: Wed, 14 Jun 2023 13:20:27 -0400 Subject: [PATCH] user_mutex: Granularize locking. Now the table is locked with a rw_lock, and individual entries have each their own rw_locks also. This improves the "contention" benchmark I have here (2 locks, 6 threads, acquire & release one lock 50,000 times per thread) mentioned in the last commit down to under 4s consistently, around 3.45s with the system idle and around 3.9s when moving windows around. Before this commit, it was around 6.7s in the best case and 7.0s in the worst, and before that, it couldn't break 3.8s in the best case. This does make GLTeapot just slightly worse: it is now down to 310-330 (with occasional dips to 300-310) from 320-340. But the following commits will improve that substantially. Change-Id: Ie029a2510746f876f4d4c74d7e878fdadf3cf590 Reviewed-on: https://review.haiku-os.org/c/haiku/+/6601 Reviewed-by: waddlesplash Tested-by: Commit checker robot --- src/system/kernel/locks/user_mutex.cpp | 179 +++++++++++++++++-------- 1 file changed, 122 insertions(+), 57 deletions(-) diff --git a/src/system/kernel/locks/user_mutex.cpp b/src/system/kernel/locks/user_mutex.cpp index 5900a25289..215ddd50d6 100644 --- a/src/system/kernel/locks/user_mutex.cpp +++ b/src/system/kernel/locks/user_mutex.cpp @@ -1,4 +1,5 @@ /* + * Copyright 2023, Haiku, Inc. All rights reserved. * Copyright 2018, Jérôme Duval, jerome.duval@gmail.com. * Copyright 2015, Hamish Morrison, hamishm53@gmail.com. * Copyright 2010, Ingo Weinhold, ingo_weinhold@gmx.de. @@ -20,11 +21,19 @@ #include +/*! One UserMutexEntry corresponds to one mutex address. + * + * The mutex's "waiting" state is controlled by the rw_lock: a waiter acquires + * a "read" lock before initiating a wait, and an unblocker acquires a "write" + * lock. That way, unblockers can be sure that no waiters will start waiting + * during unblock, and they can thus safely (without races) unset WAITING. + */ struct UserMutexEntry { phys_addr_t address; UserMutexEntry* hash_next; int32 ref_count; + rw_lock lock; ConditionVariable condition; }; @@ -57,7 +66,7 @@ typedef BOpenHashTable UserMutexTable; static UserMutexTable sUserMutexTable; -static mutex sUserMutexTableLock = MUTEX_INITIALIZER("user mutex table"); +static rw_lock sUserMutexTableLock = RW_LOCK_INITIALIZER("user mutex table"); // #pragma mark - user atomics @@ -107,9 +116,23 @@ user_atomic_test_and_set(int32* value, int32 newValue, int32 testAgainst) static UserMutexEntry* -get_user_mutex_entry(phys_addr_t address) +get_user_mutex_entry(phys_addr_t address, bool noInsert = false, bool alreadyLocked = false) { + ReadLocker tableReadLocker; + if (!alreadyLocked) + tableReadLocker.SetTo(sUserMutexTableLock, false); + UserMutexEntry* entry = sUserMutexTable.Lookup(address); + if (entry != NULL) { + atomic_add(&entry->ref_count, 1); + return entry; + } else if (noInsert) + return entry; + + tableReadLocker.Unlock(); + WriteLocker tableWriteLocker(sUserMutexTableLock); + + entry = sUserMutexTable.Lookup(address); if (entry != NULL) { atomic_add(&entry->ref_count, 1); return entry; @@ -121,6 +144,7 @@ get_user_mutex_entry(phys_addr_t address) entry->address = address; entry->ref_count = 1; + rw_lock_init(&entry->lock, "UserMutexEntry lock"); entry->condition.Init(entry, "UserMutexEntry"); sUserMutexTable.Insert(entry); @@ -129,13 +153,16 @@ get_user_mutex_entry(phys_addr_t address) static void -put_user_mutex_entry(UserMutexEntry* entry, MutexLocker& locker) +put_user_mutex_entry(UserMutexEntry* entry) { + if (entry == NULL) + return; + const phys_addr_t address = entry->address; if (atomic_add(&entry->ref_count, -1) != 1) return; - locker.Lock(); + WriteLocker tableWriteLocker(sUserMutexTableLock); // Was it removed & deleted while we were waiting for the lock? if (sUserMutexTable.Lookup(address) != entry) @@ -146,43 +173,43 @@ put_user_mutex_entry(UserMutexEntry* entry, MutexLocker& locker) return; sUserMutexTable.Remove(entry); + tableWriteLocker.Unlock(); + + rw_lock_destroy(&entry->lock); delete entry; } static status_t -user_mutex_wait_locked(int32* mutex, phys_addr_t physicalAddress, const char* name, - uint32 flags, bigtime_t timeout, MutexLocker& locker) +user_mutex_wait_locked(UserMutexEntry* entry, + uint32 flags, bigtime_t timeout, ReadLocker& locker) { - // add or get the entry from the table - UserMutexEntry* entry = get_user_mutex_entry(physicalAddress); - if (entry == NULL) - return B_NO_MEMORY; - - // wait ConditionVariableEntry waiter; entry->condition.Add(&waiter); locker.Unlock(); - status_t error = waiter.Wait(flags, timeout); - - // this will re-lock only if necessary - put_user_mutex_entry(entry, locker); - - return error; + return waiter.Wait(flags, timeout); } static bool -user_mutex_prepare_to_lock(int32* mutex) +user_mutex_prepare_to_lock(UserMutexEntry* entry, int32* mutex) { + ASSERT_READ_LOCKED_RW_LOCK(&entry->lock); + int32 oldValue = user_atomic_or(mutex, B_USER_MUTEX_LOCKED | B_USER_MUTEX_WAITING); if ((oldValue & B_USER_MUTEX_LOCKED) == 0 || (oldValue & B_USER_MUTEX_DISABLED) != 0) { - // clear the waiting flag and be done - if ((oldValue & B_USER_MUTEX_WAITING) == 0) - user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING); + // possibly unset waiting flag + if ((oldValue & B_USER_MUTEX_WAITING) == 0) { + rw_lock_read_unlock(&entry->lock); + rw_lock_write_lock(&entry->lock); + if (entry->condition.EntriesCount() == 0) + user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING); + rw_lock_write_unlock(&entry->lock); + rw_lock_read_lock(&entry->lock); + } return true; } @@ -191,23 +218,31 @@ user_mutex_prepare_to_lock(int32* mutex) static status_t -user_mutex_lock_locked(int32* mutex, phys_addr_t physicalAddress, - const char* name, uint32 flags, bigtime_t timeout, MutexLocker& locker) +user_mutex_lock_locked(UserMutexEntry* entry, int32* mutex, + uint32 flags, bigtime_t timeout, ReadLocker& locker) { - if (user_mutex_prepare_to_lock(mutex)) + if (user_mutex_prepare_to_lock(entry, mutex)) return B_OK; - return user_mutex_wait_locked(mutex, physicalAddress, name, - flags, timeout, locker); + status_t error = user_mutex_wait_locked(entry, flags, timeout, locker); + + // possibly unset waiting flag + if (error != B_OK && entry->condition.EntriesCount() == 0) { + WriteLocker writeLocker(entry->lock); + if (entry->condition.EntriesCount() == 0) + user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING); + } + + return error; } static void -user_mutex_unblock_locked(int32* mutex, phys_addr_t physicalAddress, uint32 flags) +user_mutex_unblock(UserMutexEntry* entry, int32* mutex, uint32 flags) { - UserMutexEntry* entry = sUserMutexTable.Lookup(physicalAddress); - if (entry == NULL) { - // no one is waiting + WriteLocker entryLocker(entry->lock); + if (entry->condition.EntriesCount() == 0) { + // Nobody is actually waiting at present. user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING); return; } @@ -234,8 +269,8 @@ user_mutex_unblock_locked(int32* mutex, phys_addr_t physicalAddress, uint32 flag static status_t -user_mutex_sem_acquire_locked(int32* sem, phys_addr_t physicalAddress, - const char* name, uint32 flags, bigtime_t timeout, MutexLocker& locker) +user_mutex_sem_acquire_locked(UserMutexEntry* entry, int32* sem, + uint32 flags, bigtime_t timeout, ReadLocker& locker) { // The semaphore may have been released in the meantime, and we also // need to mark it as contended if it isn't already. @@ -247,16 +282,15 @@ user_mutex_sem_acquire_locked(int32* sem, phys_addr_t physicalAddress, oldValue = value; } - return user_mutex_wait_locked(sem, physicalAddress, name, flags, + return user_mutex_wait_locked(entry, flags, timeout, locker); } static void -user_mutex_sem_release_locked(int32* sem, phys_addr_t physicalAddress) +user_mutex_sem_release(UserMutexEntry* entry, int32* sem) { - UserMutexEntry* entry = sUserMutexTable.Lookup(physicalAddress); - if (!entry) { + if (entry == NULL) { // no waiters - mark as uncontended and release int32 oldValue = user_atomic_get(sem); while (true) { @@ -268,6 +302,7 @@ user_mutex_sem_release_locked(int32* sem, phys_addr_t physicalAddress) } } + WriteLocker entryLocker(entry->lock); entry->condition.NotifyOne(B_OK); if (entry->condition.EntriesCount() == 0) { // mark the semaphore uncontended @@ -287,11 +322,15 @@ user_mutex_lock(int32* mutex, const char* name, uint32 flags, bigtime_t timeout) return error; // get the lock + UserMutexEntry* entry = get_user_mutex_entry(wiringInfo.physicalAddress); + if (entry == NULL) + return B_NO_MEMORY; { - MutexLocker locker(sUserMutexTableLock); - error = user_mutex_lock_locked(mutex, wiringInfo.physicalAddress, name, - flags, timeout, locker); + ReadLocker entryLocker(entry->lock); + error = user_mutex_lock_locked(entry, mutex, + flags, timeout, entryLocker); } + put_user_mutex_entry(entry); // unwire the page vm_unwire_page(&wiringInfo); @@ -319,19 +358,32 @@ user_mutex_switch_lock(int32* fromMutex, int32* toMutex, const char* name, } // unlock the first mutex and lock the second one + UserMutexEntry* fromEntry = NULL, + *toEntry = get_user_mutex_entry(toWiringInfo.physicalAddress); + if (toEntry == NULL) + return B_NO_MEMORY; { - MutexLocker locker(sUserMutexTableLock); + ConditionVariableEntry waiter; - const bool alreadyLocked = user_mutex_prepare_to_lock(toMutex); - user_atomic_and(fromMutex, ~(int32)B_USER_MUTEX_LOCKED); - user_mutex_unblock_locked(fromMutex, fromWiringInfo.physicalAddress, - flags); - - if (!alreadyLocked) { - error = user_mutex_lock_locked(toMutex, toWiringInfo.physicalAddress, - name, flags, timeout, locker); + bool alreadyLocked = false; + { + ReadLocker entryLocker(toEntry->lock); + alreadyLocked = user_mutex_prepare_to_lock(toEntry, toMutex); + if (!alreadyLocked) + toEntry->condition.Add(&waiter); } + + const int32 oldValue = user_atomic_and(fromMutex, ~(int32)B_USER_MUTEX_LOCKED); + if ((oldValue & B_USER_MUTEX_WAITING) != 0) + fromEntry = get_user_mutex_entry(fromWiringInfo.physicalAddress, true); + if (fromEntry != NULL) + user_mutex_unblock(fromEntry, fromMutex, flags); + + if (!alreadyLocked) + error = waiter.Wait(flags, timeout); } + put_user_mutex_entry(fromEntry); + put_user_mutex_entry(toEntry); // unwire the pages vm_unwire_page(&toWiringInfo); @@ -384,10 +436,18 @@ _user_mutex_unblock(int32* mutex, uint32 flags) if (error != B_OK) return error; - { - MutexLocker locker(sUserMutexTableLock); - user_mutex_unblock_locked(mutex, wiringInfo.physicalAddress, flags); + // In the case where there is no entry, we must hold the read lock until we + // unset WAITING, because otherwise some other thread could initiate a wait. + ReadLocker tableReadLocker(sUserMutexTableLock); + UserMutexEntry* entry = get_user_mutex_entry(wiringInfo.physicalAddress, true, true); + if (entry == NULL) { + user_atomic_and(mutex, ~(int32)B_USER_MUTEX_WAITING); + tableReadLocker.Unlock(); + } else { + tableReadLocker.Unlock(); + user_mutex_unblock(entry, mutex, flags); } + put_user_mutex_entry(entry); vm_unwire_page(&wiringInfo); @@ -426,11 +486,15 @@ _user_mutex_sem_acquire(int32* sem, const char* name, uint32 flags, if (error != B_OK) return error; + UserMutexEntry* entry = get_user_mutex_entry(wiringInfo.physicalAddress); + if (entry == NULL) + return B_NO_MEMORY; { - MutexLocker locker(sUserMutexTableLock); - error = user_mutex_sem_acquire_locked(sem, wiringInfo.physicalAddress, - name, flags | B_CAN_INTERRUPT, timeout, locker); + ReadLocker entryLocker(entry->lock); + error = user_mutex_sem_acquire_locked(entry, sem, + flags | B_CAN_INTERRUPT, timeout, entryLocker); } + put_user_mutex_entry(entry); vm_unwire_page(&wiringInfo); return syscall_restart_handle_timeout_post(error, timeout); @@ -450,10 +514,11 @@ _user_mutex_sem_release(int32* sem) if (error != B_OK) return error; + UserMutexEntry* entry = get_user_mutex_entry(wiringInfo.physicalAddress, true); { - MutexLocker locker(sUserMutexTableLock); - user_mutex_sem_release_locked(sem, wiringInfo.physicalAddress); + user_mutex_sem_release(entry, sem); } + put_user_mutex_entry(entry); vm_unwire_page(&wiringInfo); return B_OK;