From 4226b662cddd5ce0b0d8a3abe45161a74b756a88 Mon Sep 17 00:00:00 2001 From: Andreas Kling Date: Mon, 16 Aug 2021 23:29:25 +0200 Subject: [PATCH] Kernel+Userland: Remove global futexes We only ever use private futexes, so it doesn't make sense to carry around all the complexity required for global (cross-process) futexes. --- Kernel/API/POSIX/futex.h | 3 +- Kernel/FutexQueue.cpp | 8 + Kernel/FutexQueue.h | 15 +- Kernel/Memory/VMObject.cpp | 7 - Kernel/Memory/VMObject.h | 23 --- Kernel/Syscalls/futex.cpp | 172 ++++-------------- Userland/Libraries/LibC/serenity.h | 6 +- .../Libraries/LibPthread/pthread_cond.cpp | 2 +- 8 files changed, 48 insertions(+), 188 deletions(-) diff --git a/Kernel/API/POSIX/futex.h b/Kernel/API/POSIX/futex.h index 74f030c866..d814e9910e 100644 --- a/Kernel/API/POSIX/futex.h +++ b/Kernel/API/POSIX/futex.h @@ -52,9 +52,8 @@ extern "C" { #define FUTEX_WAIT_BITSET 9 #define FUTEX_WAKE_BITSET 10 -#define FUTEX_PRIVATE_FLAG (1 << 7) #define FUTEX_CLOCK_REALTIME (1 << 8) -#define FUTEX_CMD_MASK ~(FUTEX_PRIVATE_FLAG | FUTEX_CLOCK_REALTIME) +#define FUTEX_CMD_MASK ~(FUTEX_CLOCK_REALTIME) #define FUTEX_BITSET_MATCH_ANY 0xffffffff diff --git a/Kernel/FutexQueue.cpp b/Kernel/FutexQueue.cpp index f15828ea62..974b6292ce 100644 --- a/Kernel/FutexQueue.cpp +++ b/Kernel/FutexQueue.cpp @@ -10,6 +10,14 @@ namespace Kernel { +FutexQueue::FutexQueue() +{ +} + +FutexQueue::~FutexQueue() +{ +} + bool FutexQueue::should_add_blocker(Thread::Blocker& b, void* data) { VERIFY(data != nullptr); // Thread that is requesting to be blocked diff --git a/Kernel/FutexQueue.h b/Kernel/FutexQueue.h index 809478eff0..3f092180f3 100644 --- a/Kernel/FutexQueue.h +++ b/Kernel/FutexQueue.h @@ -14,11 +14,11 @@ namespace Kernel { -class FutexQueue : public Thread::BlockCondition - , public RefCounted - , public Memory::VMObjectDeletedHandler { +class FutexQueue + : public RefCounted + , public Thread::BlockCondition { public: - FutexQueue(FlatPtr user_address_or_offset, Memory::VMObject* vmobject = nullptr); + FutexQueue(); virtual ~FutexQueue(); u32 wake_n_requeue(u32, const Function&, u32, bool&, bool&); @@ -31,8 +31,6 @@ public: return Thread::current()->block(timeout, *this, forward(args)...); } - virtual void vmobject_deleted(Memory::VMObject&) override; - bool queue_imminent_wait(); void did_remove(); bool try_remove(); @@ -48,11 +46,6 @@ protected: virtual bool should_add_blocker(Thread::Blocker& b, void* data) override; private: - // For private futexes we just use the user space address. - // But for global futexes we use the offset into the VMObject - const FlatPtr m_user_address_or_offset; - WeakPtr m_vmobject; - const bool m_is_global; size_t m_imminent_waits { 1 }; // We only create this object if we're going to be waiting, so start out with 1 bool m_was_removed { false }; }; diff --git a/Kernel/Memory/VMObject.cpp b/Kernel/Memory/VMObject.cpp index c018231882..e151f9bde2 100644 --- a/Kernel/Memory/VMObject.cpp +++ b/Kernel/Memory/VMObject.cpp @@ -31,13 +31,6 @@ VMObject::VMObject(size_t size) VMObject::~VMObject() { - { - ScopedSpinLock lock(m_on_deleted_lock); - for (auto& it : m_on_deleted) - it->vmobject_deleted(*this); - m_on_deleted.clear(); - } - VERIFY(m_regions.is_empty()); } diff --git a/Kernel/Memory/VMObject.h b/Kernel/Memory/VMObject.h index 15c6e110aa..83f82b7ab0 100644 --- a/Kernel/Memory/VMObject.h +++ b/Kernel/Memory/VMObject.h @@ -7,11 +7,8 @@ #pragma once #include -#include #include -#include #include -#include #include #include #include @@ -20,12 +17,6 @@ namespace Kernel::Memory { -class VMObjectDeletedHandler { -public: - virtual ~VMObjectDeletedHandler() = default; - virtual void vmobject_deleted(VMObject&) = 0; -}; - class VMObject : public ListedRefCounted , public Weakable { @@ -63,17 +54,6 @@ public: m_regions.remove(region); } - void register_on_deleted_handler(VMObjectDeletedHandler& handler) - { - ScopedSpinLock locker(m_on_deleted_lock); - m_on_deleted.set(&handler); - } - void unregister_on_deleted_handler(VMObjectDeletedHandler& handler) - { - ScopedSpinLock locker(m_on_deleted_lock); - m_on_deleted.remove(&handler); - } - protected: explicit VMObject(size_t); explicit VMObject(VMObject const&); @@ -91,9 +71,6 @@ private: VMObject& operator=(VMObject&&) = delete; VMObject(VMObject&&) = delete; - HashTable m_on_deleted; - SpinLock m_on_deleted_lock; - Region::ListInVMObject m_regions; public: diff --git a/Kernel/Syscalls/futex.cpp b/Kernel/Syscalls/futex.cpp index 808599477d..3b5395c049 100644 --- a/Kernel/Syscalls/futex.cpp +++ b/Kernel/Syscalls/futex.cpp @@ -11,66 +11,6 @@ namespace Kernel { -static SpinLock g_global_futex_lock; -static Singleton> g_global_futex_queues; - -FutexQueue::FutexQueue(FlatPtr user_address_or_offset, Memory::VMObject* vmobject) - : m_user_address_or_offset(user_address_or_offset) - , m_is_global(vmobject != nullptr) -{ - dbgln_if(FUTEX_DEBUG, "Futex @ {}{}", - this, - m_is_global ? " (global)" : " (local)"); - - if (m_is_global) { - // Only register for global futexes - m_vmobject = vmobject->make_weak_ptr(); - vmobject->register_on_deleted_handler(*this); - } -} - -FutexQueue::~FutexQueue() -{ - if (m_is_global) { - if (auto vmobject = m_vmobject.strong_ref()) - vmobject->unregister_on_deleted_handler(*this); - } - dbgln_if(FUTEX_DEBUG, "~Futex @ {}{}", - this, - m_is_global ? " (global)" : " (local)"); -} - -void FutexQueue::vmobject_deleted(Memory::VMObject& vmobject) -{ - VERIFY(m_is_global); // If we got called we must be a global futex - // Because we're taking ourselves out of the global queue, we need - // to make sure we have at last a reference until we're done - NonnullRefPtr own_ref(*this); - - dbgln_if(FUTEX_DEBUG, "Futex::vmobject_deleted @ {}{}", - this, - m_is_global ? " (global)" : " (local)"); - - // Because this is called from the VMObject's destructor, getting a - // strong_ref in this function is unsafe! - m_vmobject = nullptr; // Just to be safe... - - { - ScopedSpinLock lock(g_global_futex_lock); - g_global_futex_queues->remove(&vmobject); - } - - bool did_wake_all; - auto wake_count = wake_all(did_wake_all); - - if constexpr (FUTEX_DEBUG) { - if (wake_count > 0) - dbgln("Futex @ {} unblocked {} waiters due to vmobject free", this, wake_count); - } - - VERIFY(did_wake_all); // No one should be left behind... -} - void Process::clear_futex_queues_on_exec() { ScopedSpinLock lock(m_futex_lock); @@ -120,100 +60,50 @@ KResultOr Process::sys$futex(Userspace } } - bool is_private = (params.futex_op & FUTEX_PRIVATE_FLAG) != 0; - auto& queue_lock = is_private ? m_futex_lock : g_global_futex_lock; - auto user_address_or_offset = FlatPtr(params.userspace_address); - auto user_address_or_offset2 = FlatPtr(params.userspace_address2); - - // If this is a global lock, look up the underlying VMObject *before* - // acquiring the queue lock - RefPtr vmobject, vmobject2; - if (!is_private) { - auto region = address_space().find_region_containing(Memory::VirtualRange { VirtualAddress { user_address_or_offset }, sizeof(u32) }); - if (!region) - return EFAULT; - vmobject = region->vmobject(); - user_address_or_offset = region->offset_in_vmobject_from_vaddr(VirtualAddress(user_address_or_offset)); - - switch (cmd) { - case FUTEX_REQUEUE: - case FUTEX_CMP_REQUEUE: - case FUTEX_WAKE_OP: { - auto region2 = address_space().find_region_containing(Memory::VirtualRange { VirtualAddress { user_address_or_offset2 }, sizeof(u32) }); - if (!region2) - return EFAULT; - vmobject2 = region2->vmobject(); - user_address_or_offset2 = region->offset_in_vmobject_from_vaddr(VirtualAddress(user_address_or_offset2)); - break; - } - } - } - - auto find_global_futex_queues = [&](Memory::VMObject& vmobject, bool create_if_not_found) -> FutexQueues* { - auto& global_queues = *g_global_futex_queues; - auto it = global_queues.find(&vmobject); - if (it != global_queues.end()) - return &it->value; - if (create_if_not_found) { - // TODO: is there a better way than setting and finding it again? - auto result = global_queues.set(&vmobject, {}); - VERIFY(result == AK::HashSetResult::InsertedNewEntry); - it = global_queues.find(&vmobject); - VERIFY(it != global_queues.end()); - return &it->value; - } - return nullptr; - }; - - auto find_futex_queue = [&](Memory::VMObject* vmobject, FlatPtr user_address_or_offset, bool create_if_not_found, bool* did_create = nullptr) -> RefPtr { - VERIFY(is_private || vmobject); + auto find_futex_queue = [&](FlatPtr user_address, bool create_if_not_found, bool* did_create = nullptr) -> RefPtr { VERIFY(!create_if_not_found || did_create != nullptr); - auto* queues = is_private ? &m_futex_queues : find_global_futex_queues(*vmobject, create_if_not_found); - if (!queues) - return {}; - auto it = queues->find(user_address_or_offset); - if (it != queues->end()) + auto* queues = &m_futex_queues; + auto it = m_futex_queues.find(user_address); + if (it != m_futex_queues.end()) return it->value; if (create_if_not_found) { *did_create = true; - auto futex_queue = adopt_ref(*new FutexQueue(user_address_or_offset, vmobject)); - auto result = queues->set(user_address_or_offset, futex_queue); + auto futex_queue = adopt_ref(*new FutexQueue); + auto result = queues->set(user_address, futex_queue); VERIFY(result == AK::HashSetResult::InsertedNewEntry); return futex_queue; } return {}; }; - auto remove_futex_queue = [&](Memory::VMObject* vmobject, FlatPtr user_address_or_offset) { - auto* queues = is_private ? &m_futex_queues : find_global_futex_queues(*vmobject, false); - if (queues) { - if (auto it = queues->find(user_address_or_offset); it != queues->end()) { - if (it->value->try_remove()) { - it->value->did_remove(); - queues->remove(it); - } + auto remove_futex_queue = [&](FlatPtr user_address) { + if (auto it = m_futex_queues.find(user_address); it != m_futex_queues.end()) { + if (it->value->try_remove()) { + it->value->did_remove(); + m_futex_queues.remove(it); } - if (!is_private && queues->is_empty()) - g_global_futex_queues->remove(vmobject); } }; - auto do_wake = [&](Memory::VMObject* vmobject, FlatPtr user_address_or_offset, u32 count, Optional bitmask) -> int { + auto do_wake = [&](FlatPtr user_address, u32 count, Optional bitmask) -> int { if (count == 0) return 0; - ScopedSpinLock lock(queue_lock); - auto futex_queue = find_futex_queue(vmobject, user_address_or_offset, false); + ScopedSpinLock locker(m_futex_lock); + auto futex_queue = find_futex_queue(user_address, false); if (!futex_queue) return 0; bool is_empty; u32 woke_count = futex_queue->wake_n(count, bitmask, is_empty); if (is_empty) { // If there are no more waiters, we want to get rid of the futex! - remove_futex_queue(vmobject, user_address_or_offset); + remove_futex_queue(user_address); } return (int)woke_count; }; + auto user_address = FlatPtr(params.userspace_address); + auto user_address2 = FlatPtr(params.userspace_address2); + auto do_wait = [&](u32 bitset) -> int { bool did_create; RefPtr futex_queue; @@ -227,9 +117,9 @@ KResultOr Process::sys$futex(Userspace } atomic_thread_fence(AK::MemoryOrder::memory_order_acquire); - ScopedSpinLock lock(queue_lock); + ScopedSpinLock locker(m_futex_lock); did_create = false; - futex_queue = find_futex_queue(vmobject.ptr(), user_address_or_offset, true, &did_create); + futex_queue = find_futex_queue(user_address, true, &did_create); VERIFY(futex_queue); // We need to try again if we didn't create this queue and the existing queue // was removed before we were able to queue an imminent wait. @@ -240,10 +130,10 @@ KResultOr Process::sys$futex(Userspace Thread::BlockResult block_result = futex_queue->wait_on(timeout, bitset); - ScopedSpinLock lock(queue_lock); + ScopedSpinLock locker(m_futex_lock); if (futex_queue->is_empty_and_no_imminent_waits()) { // If there are no more waiters, we want to get rid of the futex! - remove_futex_queue(vmobject, user_address_or_offset); + remove_futex_queue(user_address); } if (block_result == Thread::BlockResult::InterruptedByTimeout) { return ETIMEDOUT; @@ -260,8 +150,8 @@ KResultOr Process::sys$futex(Userspace atomic_thread_fence(AK::MemoryOrder::memory_order_acquire); int woken_or_requeued = 0; - ScopedSpinLock lock(queue_lock); - if (auto futex_queue = find_futex_queue(vmobject.ptr(), user_address_or_offset, false)) { + ScopedSpinLock locker(m_futex_lock); + if (auto futex_queue = find_futex_queue(user_address, false)) { RefPtr target_futex_queue; bool is_empty, is_target_empty; woken_or_requeued = futex_queue->wake_n_requeue( @@ -269,14 +159,14 @@ KResultOr Process::sys$futex(Userspace // NOTE: futex_queue's lock is being held while this callback is called // The reason we're doing this in a callback is that we don't want to always // create a target queue, only if we actually have anything to move to it! - target_futex_queue = find_futex_queue(vmobject2.ptr(), user_address_or_offset2, true); + target_futex_queue = find_futex_queue(user_address2, true); return target_futex_queue.ptr(); }, params.val2, is_empty, is_target_empty); if (is_empty) - remove_futex_queue(vmobject, user_address_or_offset); + remove_futex_queue(user_address); if (is_target_empty && target_futex_queue) - remove_futex_queue(vmobject2, user_address_or_offset2); + remove_futex_queue(user_address2); } return woken_or_requeued; }; @@ -286,7 +176,7 @@ KResultOr Process::sys$futex(Userspace return do_wait(0); case FUTEX_WAKE: - return do_wake(vmobject.ptr(), user_address_or_offset, params.val, {}); + return do_wake(user_address, params.val, {}); case FUTEX_WAKE_OP: { Optional oldval; @@ -319,7 +209,7 @@ KResultOr Process::sys$futex(Userspace if (!oldval.has_value()) return EFAULT; atomic_thread_fence(AK::MemoryOrder::memory_order_acquire); - int result = do_wake(vmobject.ptr(), user_address_or_offset, params.val, {}); + int result = do_wake(user_address, params.val, {}); if (params.val2 > 0) { bool compare_result; switch (_FUTEX_CMP(params.val3)) { @@ -345,7 +235,7 @@ KResultOr Process::sys$futex(Userspace return EINVAL; } if (compare_result) - result += do_wake(vmobject2.ptr(), user_address_or_offset2, params.val2, {}); + result += do_wake(user_address2, params.val2, {}); } return result; } @@ -366,7 +256,7 @@ KResultOr Process::sys$futex(Userspace VERIFY(params.val3 != FUTEX_BITSET_MATCH_ANY); // we should have turned it into FUTEX_WAKE if (params.val3 == 0) return EINVAL; - return do_wake(vmobject.ptr(), user_address_or_offset, params.val, params.val3); + return do_wake(user_address, params.val, params.val3); } return ENOSYS; } diff --git a/Userland/Libraries/LibC/serenity.h b/Userland/Libraries/LibC/serenity.h index fd1ef5275e..fc4f105dd7 100644 --- a/Userland/Libraries/LibC/serenity.h +++ b/Userland/Libraries/LibC/serenity.h @@ -31,18 +31,18 @@ static ALWAYS_INLINE int futex_wait(uint32_t* userspace_address, uint32_t value, if (abstime) { // NOTE: FUTEX_WAIT takes a relative timeout, so use FUTEX_WAIT_BITSET instead! - op = FUTEX_WAIT_BITSET | FUTEX_PRIVATE_FLAG; + op = FUTEX_WAIT_BITSET; if (clockid == CLOCK_REALTIME || clockid == CLOCK_REALTIME_COARSE) op |= FUTEX_CLOCK_REALTIME; } else { - op = FUTEX_WAIT | FUTEX_PRIVATE_FLAG; + op = FUTEX_WAIT; } return futex(userspace_address, op, value, abstime, nullptr, FUTEX_BITSET_MATCH_ANY); } static ALWAYS_INLINE int futex_wake(uint32_t* userspace_address, uint32_t count) { - return futex(userspace_address, FUTEX_WAKE | FUTEX_PRIVATE_FLAG, count, NULL, NULL, 0); + return futex(userspace_address, FUTEX_WAKE, count, NULL, NULL, 0); } int purge(int mode); diff --git a/Userland/Libraries/LibPthread/pthread_cond.cpp b/Userland/Libraries/LibPthread/pthread_cond.cpp index 5960a6abaa..7913daec61 100644 --- a/Userland/Libraries/LibPthread/pthread_cond.cpp +++ b/Userland/Libraries/LibPthread/pthread_cond.cpp @@ -127,7 +127,7 @@ int pthread_cond_broadcast(pthread_cond_t* cond) pthread_mutex_t* mutex = AK::atomic_load(&cond->mutex, AK::memory_order_relaxed); VERIFY(mutex); - int rc = futex(&cond->value, FUTEX_REQUEUE | FUTEX_PRIVATE_FLAG, 1, nullptr, &mutex->lock, INT_MAX); + int rc = futex(&cond->value, FUTEX_REQUEUE, 1, nullptr, &mutex->lock, INT_MAX); VERIFY(rc >= 0); return 0; }