summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
Diffstat
-rw-r--r--Kernel/Debug.h.in4
-rw-r--r--Kernel/Locking/Mutex.cpp216
-rw-r--r--Kernel/Locking/Mutex.h23
-rw-r--r--Kernel/Net/Socket.h2
-rw-r--r--Kernel/Process.cpp2
-rw-r--r--Kernel/Process.h4
-rw-r--r--Kernel/Syscalls/execve.cpp2
-rw-r--r--Kernel/Thread.cpp8
-rw-r--r--Meta/CMake/all_the_debug_macros.cmake1
9 files changed, 95 insertions, 167 deletions
diff --git a/Kernel/Debug.h.in b/Kernel/Debug.h.in
index bbd09d2f87..f28ac2309d 100644
--- a/Kernel/Debug.h.in
+++ b/Kernel/Debug.h.in
@@ -182,6 +182,10 @@
#cmakedefine01 LOCK_RESTORE_DEBUG
#endif
+#ifndef LOCK_SHARED_UPGRADE_DEBUG
+#cmakedefine01 LOCK_SHARED_UPGRADE_DEBUG
+#endif
+
#ifndef LOCK_TRACE_DEBUG
#cmakedefine01 LOCK_TRACE_DEBUG
#endif
diff --git a/Kernel/Locking/Mutex.cpp b/Kernel/Locking/Mutex.cpp
index 7592e71ea0..27c3b13e1c 100644
--- a/Kernel/Locking/Mutex.cpp
+++ b/Kernel/Locking/Mutex.cpp
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2022, Idan Horowitz <idan.horowitz@serenityos.org>
*
* SPDX-License-Identifier: BSD-2-Clause
*/
@@ -31,12 +32,15 @@ void Mutex::lock(Mode mode, [[maybe_unused]] LockLocation const& location)
dbgln_if(LOCK_TRACE_DEBUG, "Mutex::lock @ ({}) {}: acquire {}, currently unlocked", this, m_name, mode_to_string(mode));
m_mode = mode;
VERIFY(!m_holder);
- VERIFY(m_shared_holders.is_empty());
+ VERIFY(m_shared_holders == 0);
if (mode == Mode::Exclusive) {
m_holder = current_thread;
} else {
VERIFY(mode == Mode::Shared);
- m_shared_holders.set(current_thread, 1);
+ ++m_shared_holders;
+#if LOCK_SHARED_UPGRADE_DEBUG
+ m_shared_holders_map.set(current_thread, 1);
+#endif
}
VERIFY(m_times_locked == 0);
m_times_locked++;
@@ -59,12 +63,11 @@ void Mutex::lock(Mode mode, [[maybe_unused]] LockLocation const& location)
if (m_mode == Mode::Exclusive) {
VERIFY(m_holder == current_thread);
- VERIFY(m_shared_holders.is_empty());
+ VERIFY(m_shared_holders == 0);
} else if (did_block && mode == Mode::Shared) {
// Only if we blocked trying to acquire a shared lock the lock would have been converted
VERIFY(!m_holder);
- VERIFY(!m_shared_holders.is_empty());
- VERIFY(m_shared_holders.find(current_thread) != m_shared_holders.end());
+ VERIFY(m_shared_holders > 0);
}
if constexpr (LOCK_TRACE_DEBUG) {
@@ -89,19 +92,14 @@ void Mutex::lock(Mode mode, [[maybe_unused]] LockLocation const& location)
case Mode::Shared: {
VERIFY(!m_holder);
if (mode == Mode::Exclusive) {
- if (m_shared_holders.size() == 1) {
- auto it = m_shared_holders.begin();
- if (it->key == current_thread) {
- it->value++;
- m_times_locked++;
- m_mode = Mode::Exclusive;
- m_holder = current_thread;
- m_shared_holders.clear();
- dbgln_if(LOCK_TRACE_DEBUG, "Mutex::lock @ {} ({}): acquire {}, converted shared to exclusive lock, locks held {}", this, m_name, mode_to_string(mode), m_times_locked);
- return;
- }
- }
-
+ dbgln_if(LOCK_TRACE_DEBUG, "Mutex::lock @ {} ({}): blocking for exclusive access, currently shared, locks held {}", this, m_name, m_times_locked);
+#if LOCK_SHARED_UPGRADE_DEBUG
+ VERIFY(m_shared_holders_map.size() != 1 || m_shared_holders_map.begin()->key != current_thread);
+#endif
+ // WARNING: The following block will deadlock if the current thread is the only shared locker of this Mutex
+ // and is asking to upgrade the lock to be exclusive without first releasing the shared lock. We have no
+ // allocation-free way to detect such a scenario, so if you suspect that this is the cause of your deadlock,
+ // try turning on LOCK_SHARED_UPGRADE_DEBUG.
block(*current_thread, mode, lock, 1);
did_block = true;
VERIFY(m_mode == mode);
@@ -112,23 +110,26 @@ void Mutex::lock(Mode mode, [[maybe_unused]] LockLocation const& location)
VERIFY(m_times_locked > 0);
if (m_mode == Mode::Shared) {
VERIFY(!m_holder);
- VERIFY(!did_block || m_shared_holders.contains(current_thread));
+ VERIFY(!did_block);
} else if (did_block) {
VERIFY(mode == Mode::Exclusive);
VERIFY(m_holder == current_thread);
- VERIFY(m_shared_holders.is_empty());
+ VERIFY(m_shared_holders == 0);
}
if (!did_block) {
// if we didn't block we must still be a shared lock
VERIFY(m_mode == Mode::Shared);
m_times_locked++;
- VERIFY(!m_shared_holders.is_empty());
- auto it = m_shared_holders.find(current_thread);
- if (it != m_shared_holders.end())
+ VERIFY(m_shared_holders > 0);
+ ++m_shared_holders;
+#if LOCK_SHARED_UPGRADE_DEBUG
+ auto it = m_shared_holders_map.find(current_thread);
+ if (it != m_shared_holders_map.end())
it->value++;
else
- m_shared_holders.set(current_thread, 1);
+ m_shared_holders_map.set(current_thread, 1);
+#endif
}
#if LOCK_DEBUG
@@ -166,20 +167,21 @@ void Mutex::unlock()
switch (current_mode) {
case Mode::Exclusive:
VERIFY(m_holder == current_thread);
- VERIFY(m_shared_holders.is_empty());
+ VERIFY(m_shared_holders == 0);
if (m_times_locked == 0)
m_holder = nullptr;
break;
case Mode::Shared: {
VERIFY(!m_holder);
- auto it = m_shared_holders.find(current_thread);
- VERIFY(it != m_shared_holders.end());
- if (it->value > 1) {
+ VERIFY(m_shared_holders > 0);
+ --m_shared_holders;
+#if LOCK_SHARED_UPGRADE_DEBUG
+ auto it = m_shared_holders_map.find(current_thread);
+ if (it->value > 1)
it->value--;
- } else {
- VERIFY(it->value > 0);
- m_shared_holders.remove(it);
- }
+ else
+ m_shared_holders_map.remove(it);
+#endif
break;
}
default:
@@ -193,7 +195,7 @@ void Mutex::unlock()
#endif
if (m_times_locked == 0) {
- VERIFY(current_mode == Mode::Exclusive ? !m_holder : m_shared_holders.is_empty());
+ VERIFY(current_mode == Mode::Exclusive ? !m_holder : m_shared_holders == 0);
m_mode = Mode::Unlocked;
unblock_waiters(current_mode);
@@ -230,8 +232,11 @@ void Mutex::unblock_waiters(Mode previous_mode)
m_mode = Mode::Shared;
for (auto& thread : m_blocked_threads_list_shared) {
auto requested_locks = thread.unblock_from_lock(*this);
- auto set_result = m_shared_holders.set(&thread, requested_locks);
+ m_shared_holders += requested_locks;
+#if LOCK_SHARED_UPGRADE_DEBUG
+ auto set_result = m_shared_holders_map.set(&thread, requested_locks);
VERIFY(set_result == AK::HashSetResult::InsertedNewEntry);
+#endif
m_times_locked += requested_locks;
}
return true;
@@ -255,7 +260,7 @@ void Mutex::unblock_waiters(Mode previous_mode)
}
}
-auto Mutex::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode
+auto Mutex::force_unlock_exclusive_if_locked(u32& lock_count_to_restore) -> Mode
{
// NOTE: This may be called from an interrupt handler (not an IRQ handler)
// and also from within critical sections!
@@ -270,7 +275,7 @@ auto Mutex::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode
return Mode::Unlocked;
}
- dbgln_if(LOCK_RESTORE_DEBUG, "Mutex::force_unlock_if_locked @ {}: unlocking exclusive with lock count: {}", this, m_times_locked);
+ dbgln_if(LOCK_RESTORE_DEBUG, "Mutex::force_unlock_exclusive_if_locked @ {}: unlocking exclusive with lock count: {}", this, m_times_locked);
#if LOCK_DEBUG
m_holder->holding_lock(*this, -(int)m_times_locked, {});
#endif
@@ -282,32 +287,6 @@ auto Mutex::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode
unblock_waiters(Mode::Exclusive);
break;
}
- case Mode::Shared: {
- VERIFY(!m_holder);
- auto it = m_shared_holders.find(current_thread);
- if (it == m_shared_holders.end()) {
- lock_count_to_restore = 0;
- return Mode::Unlocked;
- }
-
- dbgln_if(LOCK_RESTORE_DEBUG, "Mutex::force_unlock_if_locked @ {}: unlocking exclusive with lock count: {}, total locks: {}",
- this, it->value, m_times_locked);
-
- VERIFY(it->value > 0);
- lock_count_to_restore = it->value;
- VERIFY(lock_count_to_restore > 0);
-#if LOCK_DEBUG
- m_holder->holding_lock(*this, -(int)lock_count_to_restore, {});
-#endif
- m_shared_holders.remove(it);
- VERIFY(m_times_locked >= lock_count_to_restore);
- m_times_locked -= lock_count_to_restore;
- if (m_times_locked == 0) {
- m_mode = Mode::Unlocked;
- unblock_waiters(Mode::Shared);
- }
- break;
- }
case Mode::Unlocked: {
lock_count_to_restore = 0;
break;
@@ -318,107 +297,46 @@ auto Mutex::force_unlock_if_locked(u32& lock_count_to_restore) -> Mode
return current_mode;
}
-void Mutex::restore_lock(Mode mode, u32 lock_count, [[maybe_unused]] LockLocation const& location)
+void Mutex::restore_exclusive_lock(u32 lock_count, [[maybe_unused]] LockLocation const& location)
{
- VERIFY(mode != Mode::Unlocked);
VERIFY(lock_count > 0);
VERIFY(!Processor::current_in_irq());
auto* current_thread = Thread::current();
bool did_block = false;
SpinlockLocker lock(m_lock);
- switch (mode) {
- case Mode::Exclusive: {
- auto previous_mode = m_mode;
- if ((m_mode == Mode::Exclusive && m_holder != current_thread)
- || (m_mode == Mode::Shared && (m_shared_holders.size() != 1 || !m_shared_holders.contains(current_thread)))) {
- block(*current_thread, Mode::Exclusive, lock, lock_count);
- did_block = true;
- // If we blocked then m_mode should have been updated to what we requested
- VERIFY(m_mode == Mode::Exclusive);
- }
-
- dbgln_if(LOCK_RESTORE_DEBUG, "Mutex::restore_lock @ {}: restoring {} with lock count {}, was {}", this, mode_to_string(mode), lock_count, mode_to_string(previous_mode));
-
- VERIFY(m_mode != Mode::Shared);
- VERIFY(m_shared_holders.is_empty());
- if (did_block) {
- VERIFY(m_times_locked > 0);
- VERIFY(m_holder == current_thread);
- } else {
- if (m_mode == Mode::Unlocked) {
- m_mode = Mode::Exclusive;
- VERIFY(m_times_locked == 0);
- m_times_locked = lock_count;
- VERIFY(!m_holder);
- m_holder = current_thread;
- } else if (m_mode == Mode::Shared) {
- // Upgrade the shared lock to an exclusive lock
- VERIFY(!m_holder);
- VERIFY(m_shared_holders.size() == 1);
- VERIFY(m_shared_holders.contains(current_thread));
- m_mode = Mode::Exclusive;
- m_holder = current_thread;
- m_shared_holders.clear();
- } else {
- VERIFY(m_mode == Mode::Exclusive);
- VERIFY(m_holder == current_thread);
- VERIFY(m_times_locked > 0);
- m_times_locked += lock_count;
- }
- }
-
-#if LOCK_DEBUG
- m_holder->holding_lock(*this, (int)lock_count, location);
-#endif
- return;
+ [[maybe_unused]] auto previous_mode = m_mode;
+ if (m_mode == Mode::Exclusive && m_holder != current_thread) {
+ block(*current_thread, Mode::Exclusive, lock, lock_count);
+ did_block = true;
+ // If we blocked then m_mode should have been updated to what we requested
+ VERIFY(m_mode == Mode::Exclusive);
}
- case Mode::Shared: {
- auto previous_mode = m_mode;
- if (m_mode == Mode::Exclusive && m_holder != current_thread) {
- block(*current_thread, Mode::Shared, lock, lock_count);
- did_block = true;
- // If we blocked then m_mode should have been updated to what we requested
- VERIFY(m_mode == Mode::Shared);
- }
- dbgln_if(LOCK_RESTORE_DEBUG, "Mutex::restore_lock @ {}: restoring {} with lock count {}, was {}",
- this, mode_to_string(mode), lock_count, mode_to_string(previous_mode));
+ dbgln_if(LOCK_RESTORE_DEBUG, "Mutex::restore_exclusive_lock @ {}: restoring exclusive with lock count {}, was {}", this, lock_count, mode_to_string(previous_mode));
- VERIFY(!m_holder);
- if (did_block) {
- VERIFY(m_times_locked > 0);
- VERIFY(m_shared_holders.contains(current_thread));
+ VERIFY(m_mode != Mode::Shared);
+ VERIFY(m_shared_holders == 0);
+ if (did_block) {
+ VERIFY(m_times_locked > 0);
+ VERIFY(m_holder == current_thread);
+ } else {
+ if (m_mode == Mode::Unlocked) {
+ m_mode = Mode::Exclusive;
+ VERIFY(m_times_locked == 0);
+ m_times_locked = lock_count;
+ VERIFY(!m_holder);
+ m_holder = current_thread;
} else {
- if (m_mode == Mode::Unlocked) {
- m_mode = Mode::Shared;
- m_times_locked += lock_count;
- auto set_result = m_shared_holders.set(current_thread, lock_count);
- // There may be other shared lock holders already, but we should not have an entry yet
- VERIFY(set_result == AK::HashSetResult::InsertedNewEntry);
- } else if (m_mode == Mode::Shared) {
- m_times_locked += lock_count;
- if (auto it = m_shared_holders.find(current_thread); it != m_shared_holders.end()) {
- it->value += lock_count;
- } else {
- auto set_result = m_shared_holders.set(current_thread, lock_count);
- // There may be other shared lock holders already, but we should not have an entry yet
- VERIFY(set_result == AK::HashSetResult::InsertedNewEntry);
- }
- } else {
- VERIFY(m_mode == Mode::Exclusive);
- VERIFY(m_holder == current_thread);
- m_times_locked += lock_count;
- }
+ VERIFY(m_mode == Mode::Exclusive);
+ VERIFY(m_holder == current_thread);
+ VERIFY(m_times_locked > 0);
+ m_times_locked += lock_count;
}
+ }
#if LOCK_DEBUG
- m_holder->holding_lock(*this, (int)lock_count, location);
+ m_holder->holding_lock(*this, (int)lock_count, location);
#endif
- return;
- }
- default:
- VERIFY_NOT_REACHED();
- }
}
}
diff --git a/Kernel/Locking/Mutex.h b/Kernel/Locking/Mutex.h
index 54748e3ce1..ec6d138410 100644
--- a/Kernel/Locking/Mutex.h
+++ b/Kernel/Locking/Mutex.h
@@ -1,5 +1,6 @@
/*
* Copyright (c) 2018-2020, Andreas Kling <kling@serenityos.org>
+ * Copyright (c) 2022, Idan Horowitz <idan.horowitz@serenityos.org>
*
* SPDX-License-Identifier: BSD-2-Clause
*/
@@ -33,23 +34,23 @@ public:
~Mutex() = default;
void lock(Mode mode = Mode::Exclusive, LockLocation const& location = LockLocation::current());
- void restore_lock(Mode, u32, LockLocation const& location = LockLocation::current());
+ void restore_exclusive_lock(u32, LockLocation const& location = LockLocation::current());
void unlock();
- [[nodiscard]] Mode force_unlock_if_locked(u32&);
+ [[nodiscard]] Mode force_unlock_exclusive_if_locked(u32&);
[[nodiscard]] bool is_locked() const
{
SpinlockLocker lock(m_lock);
return m_mode != Mode::Unlocked;
}
- [[nodiscard]] bool is_locked_by_current_thread() const
+
+ [[nodiscard]] bool is_exclusively_locked_by_current_thread() const
{
SpinlockLocker lock(m_lock);
- if (m_mode == Mode::Exclusive)
- return m_holder == Thread::current();
- if (m_mode == Mode::Shared)
- return m_shared_holders.contains(Thread::current());
- return false;
+ VERIFY(m_mode != Mode::Shared); // This method should only be used on exclusively-held locks
+ if (m_mode == Mode::Unlocked)
+ return false;
+ return m_holder == Thread::current();
}
[[nodiscard]] StringView name() const { return m_name; }
@@ -93,12 +94,16 @@ private:
// When locked exclusively, this is always the one thread that holds the
// lock.
RefPtr<Thread> m_holder;
- HashMap<Thread*, u32> m_shared_holders;
+ size_t m_shared_holders { 0 };
BlockedThreadList m_blocked_threads_list_exclusive;
BlockedThreadList m_blocked_threads_list_shared;
mutable Spinlock m_lock;
+
+#if LOCK_SHARED_UPGRADE_DEBUG
+ HashMap<Thread*, u32> m_shared_holders_map;
+#endif
};
class MutexLocker {
diff --git a/Kernel/Net/Socket.h b/Kernel/Net/Socket.h
index 013172c11c..979df8b27c 100644
--- a/Kernel/Net/Socket.h
+++ b/Kernel/Net/Socket.h
@@ -132,7 +132,7 @@ protected:
ErrorOr<void> so_error() const
{
- VERIFY(m_mutex.is_locked_by_current_thread());
+ VERIFY(m_mutex.is_exclusively_locked_by_current_thread());
return m_so_error;
}
diff --git a/Kernel/Process.cpp b/Kernel/Process.cpp
index ccebd03bf9..7888644471 100644
--- a/Kernel/Process.cpp
+++ b/Kernel/Process.cpp
@@ -105,7 +105,7 @@ void Process::kill_threads_except_self()
});
u32 dropped_lock_count = 0;
- if (big_lock().force_unlock_if_locked(dropped_lock_count) != LockMode::Unlocked)
+ if (big_lock().force_unlock_exclusive_if_locked(dropped_lock_count) != LockMode::Unlocked)
dbgln("Process {} big lock had {} locks", *this, dropped_lock_count);
}
diff --git a/Kernel/Process.h b/Kernel/Process.h
index f774857404..ccf8478a45 100644
--- a/Kernel/Process.h
+++ b/Kernel/Process.h
@@ -978,10 +978,10 @@ inline ProcessID Thread::pid() const
}
#define VERIFY_PROCESS_BIG_LOCK_ACQUIRED(process) \
- VERIFY(process->big_lock().is_locked_by_current_thread());
+ VERIFY(process->big_lock().is_exclusively_locked_by_current_thread());
#define VERIFY_NO_PROCESS_BIG_LOCK(process) \
- VERIFY(!process->big_lock().is_locked_by_current_thread());
+ VERIFY(!process->big_lock().is_exclusively_locked_by_current_thread());
inline static ErrorOr<NonnullOwnPtr<KString>> try_copy_kstring_from_user(const Kernel::Syscall::StringArgument& string)
{
diff --git a/Kernel/Syscalls/execve.cpp b/Kernel/Syscalls/execve.cpp
index b47a9fb65b..127387ce39 100644
--- a/Kernel/Syscalls/execve.cpp
+++ b/Kernel/Syscalls/execve.cpp
@@ -630,7 +630,7 @@ ErrorOr<void> Process::do_exec(NonnullRefPtr<OpenFileDescription> main_program_d
}
u32 lock_count_to_restore;
- [[maybe_unused]] auto rc = big_lock().force_unlock_if_locked(lock_count_to_restore);
+ [[maybe_unused]] auto rc = big_lock().force_unlock_exclusive_if_locked(lock_count_to_restore);
VERIFY_INTERRUPTS_DISABLED();
VERIFY(Processor::in_critical());
return {};
diff --git a/Kernel/Thread.cpp b/Kernel/Thread.cpp
index 3196e0a1b1..c980f6fd2c 100644
--- a/Kernel/Thread.cpp
+++ b/Kernel/Thread.cpp
@@ -193,7 +193,7 @@ void Thread::block(Kernel::Mutex& lock, SpinlockLocker<Spinlock>& lock_lock, u32
// Yield to the scheduler, and wait for us to resume unblocked.
VERIFY(!g_scheduler_lock.is_locked_by_current_processor());
VERIFY(Processor::in_critical());
- if (&lock != &big_lock && big_lock.is_locked_by_current_thread()) {
+ if (&lock != &big_lock && big_lock.is_exclusively_locked_by_current_thread()) {
// We're locking another lock and already hold the big lock...
// We need to release the big lock
yield_and_release_relock_big_lock();
@@ -390,7 +390,7 @@ void Thread::exit(void* exit_value)
void Thread::yield_without_releasing_big_lock(VerifyLockNotHeld verify_lock_not_held)
{
VERIFY(!g_scheduler_lock.is_locked_by_current_processor());
- VERIFY(verify_lock_not_held == VerifyLockNotHeld::No || !process().big_lock().is_locked_by_current_thread());
+ VERIFY(verify_lock_not_held == VerifyLockNotHeld::No || !process().big_lock().is_exclusively_locked_by_current_thread());
// Disable interrupts here. This ensures we don't accidentally switch contexts twice
InterruptDisabler disable;
Scheduler::yield(); // flag a switch
@@ -415,7 +415,7 @@ void Thread::yield_and_release_relock_big_lock()
LockMode Thread::unlock_process_if_locked(u32& lock_count_to_restore)
{
- return process().big_lock().force_unlock_if_locked(lock_count_to_restore);
+ return process().big_lock().force_unlock_exclusive_if_locked(lock_count_to_restore);
}
void Thread::relock_process(LockMode previous_locked, u32 lock_count_to_restore)
@@ -433,7 +433,7 @@ void Thread::relock_process(LockMode previous_locked, u32 lock_count_to_restore)
if (previous_locked != LockMode::Unlocked) {
// We've unblocked, relock the process if needed and carry on.
- process().big_lock().restore_lock(previous_locked, lock_count_to_restore);
+ process().big_lock().restore_exclusive_lock(lock_count_to_restore);
}
}
diff --git a/Meta/CMake/all_the_debug_macros.cmake b/Meta/CMake/all_the_debug_macros.cmake
index abb8b61613..4ff054c51c 100644
--- a/Meta/CMake/all_the_debug_macros.cmake
+++ b/Meta/CMake/all_the_debug_macros.cmake
@@ -106,6 +106,7 @@ set(LOCK_DEBUG ON)
set(LOCK_IN_CRITICAL_DEBUG ON)
set(LOCK_RANK_ENFORCEMENT ON)
set(LOCK_RESTORE_DEBUG ON)
+set(LOCK_SHARED_UPGRADE_DEBUG ON)
set(LOCK_TRACE_DEBUG ON)
set(LOOKUPSERVER_DEBUG ON)
set(MALLOC_DEBUG ON)
generated by cgit v1.2.3 (git 2.25.1) at 2024-11-15 00:27:58 +0000