diff --git a/Kernel/FutexQueue.cpp b/Kernel/FutexQueue.cpp index 16c7cf45b9a..f15828ea626 100644 --- a/Kernel/FutexQueue.cpp +++ b/Kernel/FutexQueue.cpp @@ -16,6 +16,13 @@ bool FutexQueue::should_add_blocker(Thread::Blocker& b, void* data) VERIFY(m_lock.is_locked()); VERIFY(b.blocker_type() == Thread::Blocker::Type::Futex); + VERIFY(m_imminent_waits > 0); + m_imminent_waits--; + + if (m_was_removed) { + dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: should not block thread {}: was removed", this, *static_cast(data)); + return false; + } dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: should block thread {}", this, *static_cast(data)); return true; @@ -43,7 +50,7 @@ u32 FutexQueue::wake_n_requeue(u32 wake_count, const Function& ge } return false; }); - is_empty = is_empty_locked(); + is_empty = is_empty_and_no_imminent_waits_locked(); if (requeue_count > 0) { auto blockers_to_requeue = do_take_blockers(requeue_count); if (!blockers_to_requeue.is_empty()) { @@ -69,7 +76,7 @@ u32 FutexQueue::wake_n_requeue(u32 wake_count, const Function& ge blocker.finish_requeue(*target_futex_queue); } target_futex_queue->do_append_blockers(move(blockers_to_requeue)); - is_empty_target = target_futex_queue->is_empty_locked(); + is_empty_target = target_futex_queue->is_empty_and_no_imminent_waits_locked(); } else { dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: wake_n_requeue could not get target queue to requeue {} blockers", this, blockers_to_requeue.size()); do_append_blockers(move(blockers_to_requeue)); @@ -81,8 +88,10 @@ u32 FutexQueue::wake_n_requeue(u32 wake_count, const Function& ge u32 FutexQueue::wake_n(u32 wake_count, const Optional& bitset, bool& is_empty) { - if (wake_count == 0) + if (wake_count == 0) { + is_empty = false; return 0; // should we assert instead? + } ScopedSpinLock lock(m_lock); dbgln_if(FUTEXQUEUE_DEBUG, "FutexQueue @ {}: wake_n({})", this, wake_count); u32 did_wake = 0; @@ -100,7 +109,7 @@ u32 FutexQueue::wake_n(u32 wake_count, const Optional& bitset, bool& is_emp } return false; }); - is_empty = is_empty_locked(); + is_empty = is_empty_and_no_imminent_waits_locked(); return did_wake; } @@ -120,8 +129,40 @@ u32 FutexQueue::wake_all(bool& is_empty) } return false; }); - is_empty = is_empty_locked(); + is_empty = is_empty_and_no_imminent_waits_locked(); return did_wake; } +bool FutexQueue::is_empty_and_no_imminent_waits_locked() +{ + return m_imminent_waits == 0 && is_empty_locked(); +} + +bool FutexQueue::queue_imminent_wait() +{ + ScopedSpinLock lock(m_lock); + if (m_was_removed) + return false; + m_imminent_waits++; + return true; +} + +bool FutexQueue::try_remove() +{ + ScopedSpinLock lock(m_lock); + if (m_was_removed) + return false; + if (!is_empty_and_no_imminent_waits_locked()) + return false; + m_was_removed = true; + return true; +} + +void FutexQueue::did_remove() +{ + ScopedSpinLock lock(m_lock); + VERIFY(m_was_removed); + VERIFY(is_empty_and_no_imminent_waits_locked()); +} + } diff --git a/Kernel/FutexQueue.h b/Kernel/FutexQueue.h index b63af4e135f..bc61ed66cd2 100644 --- a/Kernel/FutexQueue.h +++ b/Kernel/FutexQueue.h @@ -33,6 +33,17 @@ public: virtual void vmobject_deleted(VMObject&) override; + bool queue_imminent_wait(); + void did_remove(); + bool try_remove(); + + bool is_empty_and_no_imminent_waits() + { + ScopedSpinLock lock(m_lock); + return is_empty_and_no_imminent_waits_locked(); + } + bool is_empty_and_no_imminent_waits_locked(); + protected: virtual bool should_add_blocker(Thread::Blocker& b, void* data) override; @@ -42,6 +53,8 @@ private: 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/Syscalls/futex.cpp b/Kernel/Syscalls/futex.cpp index cd669e1dc76..208a49bf6c2 100644 --- a/Kernel/Syscalls/futex.cpp +++ b/Kernel/Syscalls/futex.cpp @@ -164,8 +164,9 @@ KResultOr Process::sys$futex(Userspace return nullptr; }; - auto find_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset, bool create_if_not_found) -> RefPtr { + auto find_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset, bool create_if_not_found, bool* did_create = nullptr) -> RefPtr { VERIFY(is_private || vmobject); + 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 {}; @@ -173,6 +174,7 @@ KResultOr Process::sys$futex(Userspace if (it != 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); VERIFY(result == AK::HashSetResult::InsertedNewEntry); @@ -184,7 +186,12 @@ KResultOr Process::sys$futex(Userspace auto remove_futex_queue = [&](VMObject* vmobject, FlatPtr user_address_or_offset) { auto* queues = is_private ? &m_futex_queues : find_global_futex_queues(*vmobject, false); if (queues) { - queues->remove(user_address_or_offset); + if (auto it = queues->find(user_address_or_offset); it != queues->end()) { + if (it->value->try_remove()) { + it->value->did_remove(); + queues->remove(it); + } + } if (!is_private && queues->is_empty()) g_global_futex_queues->remove(vmobject); } @@ -208,17 +215,24 @@ KResultOr Process::sys$futex(Userspace ScopedSpinLock lock(queue_lock); auto do_wait = [&](u32 bitset) -> int { - auto user_value = user_atomic_load_relaxed(params.userspace_address); - if (!user_value.has_value()) - return EFAULT; - if (user_value.value() != params.val) { - dbgln("futex wait: EAGAIN. user value: {:p} @ {:p} != val: {}", user_value.value(), params.userspace_address, params.val); - return EAGAIN; - } - atomic_thread_fence(AK::MemoryOrder::memory_order_acquire); + bool did_create; + RefPtr futex_queue; + do { + auto user_value = user_atomic_load_relaxed(params.userspace_address); + if (!user_value.has_value()) + return EFAULT; + if (user_value.value() != params.val) { + dbgln_if(FUTEX_DEBUG, "futex wait: EAGAIN. user value: {:p} @ {:p} != val: {}", user_value.value(), params.userspace_address, params.val); + return EAGAIN; + } + atomic_thread_fence(AK::MemoryOrder::memory_order_acquire); - auto futex_queue = find_futex_queue(vmobject.ptr(), user_address_or_offset, true); - VERIFY(futex_queue); + did_create = false; + futex_queue = find_futex_queue(vmobject.ptr(), user_address_or_offset, 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. + } while (!did_create && !futex_queue->queue_imminent_wait()); // We need to release the lock before blocking. But we have a reference // to the FutexQueue so that we can keep it alive. @@ -227,7 +241,7 @@ KResultOr Process::sys$futex(Userspace Thread::BlockResult block_result = futex_queue->wait_on(timeout, bitset); lock.lock(); - if (futex_queue->is_empty()) { + 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); }