From 8bf7b19d1d35b1869a412fb23a3158dcc974cca1 Mon Sep 17 00:00:00 2001 From: bunnei Date: Fri, 4 Jun 2021 19:26:48 -0700 Subject: hle: kernel: Refactor to allocate a ServiceThread per service handler. - Previously, we would allocate a thread per session, which adds new threads on CloneCurrentObject. - This results in race conditions with N sessions queuing requests to the same service interface. - Fixes Pokken Tournament DX crashes/softlocks, which were regressed by #6347. --- src/core/hle/kernel/k_server_session.cpp | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) (limited to 'src/core/hle/kernel/k_server_session.cpp') diff --git a/src/core/hle/kernel/k_server_session.cpp b/src/core/hle/kernel/k_server_session.cpp index dbf03b4623..e66a9198ad 100644 --- a/src/core/hle/kernel/k_server_session.cpp +++ b/src/core/hle/kernel/k_server_session.cpp @@ -13,8 +13,10 @@ #include "core/hle/kernel/hle_ipc.h" #include "core/hle/kernel/k_client_port.h" #include "core/hle/kernel/k_handle_table.h" +#include "core/hle/kernel/k_port.h" #include "core/hle/kernel/k_process.h" #include "core/hle/kernel/k_scheduler.h" +#include "core/hle/kernel/k_server_port.h" #include "core/hle/kernel/k_server_session.h" #include "core/hle/kernel/k_session.h" #include "core/hle/kernel/k_thread.h" @@ -23,18 +25,21 @@ namespace Kernel { -KServerSession::KServerSession(KernelCore& kernel_) - : KSynchronizationObject{kernel_}, manager{std::make_shared()} {} +KServerSession::KServerSession(KernelCore& kernel_) : KSynchronizationObject{kernel_} {} -KServerSession::~KServerSession() { - kernel.ReleaseServiceThread(service_thread); -} +KServerSession::~KServerSession() {} -void KServerSession::Initialize(KSession* parent_, std::string&& name_) { +void KServerSession::Initialize(KSession* parent_, std::string&& name_, + std::shared_ptr manager_) { // Set member variables. parent = parent_; name = std::move(name_); - service_thread = kernel.CreateServiceThread(name); + + if (manager_) { + manager = manager_; + } else { + manager = std::make_shared(kernel); + } } void KServerSession::Destroy() { @@ -114,7 +119,7 @@ ResultCode KServerSession::QueueSyncRequest(KThread* thread, Core::Memory::Memor context->PopulateFromIncomingCommandBuffer(kernel.CurrentProcess()->GetHandleTable(), cmd_buf); - if (auto strong_ptr = service_thread.lock()) { + if (auto strong_ptr = manager->GetServiceThread().lock()) { strong_ptr->QueueSyncRequest(*parent, std::move(context)); return ResultSuccess; } -- cgit v1.2.3 From 38d8bcc574ae50cf13d3da159346c112aafd532b Mon Sep 17 00:00:00 2001 From: bunnei Date: Sun, 6 Jun 2021 15:39:11 -0700 Subject: hle: kernel: KAutoObjectWithListContainer: Use boost::instrusive::rbtree. - Fixes some crashes introduced by our common intrusive red/black tree impl. --- src/core/hle/kernel/k_auto_object.h | 11 +++++++---- src/core/hle/kernel/k_auto_object_container.cpp | 4 ++-- src/core/hle/kernel/k_auto_object_container.h | 5 +++-- src/core/hle/kernel/k_client_port.cpp | 4 ++-- src/core/hle/kernel/k_client_session.h | 4 ++-- src/core/hle/kernel/k_readable_event.h | 4 ++-- src/core/hle/kernel/k_server_port.cpp | 4 ++-- src/core/hle/kernel/k_server_port.h | 2 +- src/core/hle/kernel/k_server_session.cpp | 4 ++-- src/core/hle/kernel/k_server_session.h | 2 +- src/core/hle/kernel/k_writable_event.cpp | 4 ++-- 11 files changed, 26 insertions(+), 22 deletions(-) (limited to 'src/core/hle/kernel/k_server_session.cpp') diff --git a/src/core/hle/kernel/k_auto_object.h b/src/core/hle/kernel/k_auto_object.h index bc18582be0..88a052f658 100644 --- a/src/core/hle/kernel/k_auto_object.h +++ b/src/core/hle/kernel/k_auto_object.h @@ -7,10 +7,11 @@ #include #include +#include + #include "common/assert.h" #include "common/common_funcs.h" #include "common/common_types.h" -#include "common/intrusive_red_black_tree.h" #include "core/hle/kernel/k_class_token.h" namespace Kernel { @@ -175,7 +176,7 @@ private: class KAutoObjectWithListContainer; -class KAutoObjectWithList : public KAutoObject { +class KAutoObjectWithList : public KAutoObject, public boost::intrusive::set_base_hook<> { public: explicit KAutoObjectWithList(KernelCore& kernel_) : KAutoObject(kernel_) {} @@ -192,6 +193,10 @@ public: } } + friend bool operator<(const KAutoObjectWithList& left, const KAutoObjectWithList& right) { + return &left < &right; + } + public: virtual u64 GetId() const { return reinterpret_cast(this); @@ -203,8 +208,6 @@ public: private: friend class KAutoObjectWithListContainer; - - Common::IntrusiveRedBlackTreeNode list_node; }; template diff --git a/src/core/hle/kernel/k_auto_object_container.cpp b/src/core/hle/kernel/k_auto_object_container.cpp index fc0c288748..010006bb70 100644 --- a/src/core/hle/kernel/k_auto_object_container.cpp +++ b/src/core/hle/kernel/k_auto_object_container.cpp @@ -9,13 +9,13 @@ namespace Kernel { void KAutoObjectWithListContainer::Register(KAutoObjectWithList* obj) { KScopedLightLock lk(m_lock); - m_object_list.insert(*obj); + m_object_list.insert_unique(*obj); } void KAutoObjectWithListContainer::Unregister(KAutoObjectWithList* obj) { KScopedLightLock lk(m_lock); - m_object_list.erase(m_object_list.iterator_to(*obj)); + m_object_list.erase(*obj); } size_t KAutoObjectWithListContainer::GetOwnedCount(KProcess* owner) { diff --git a/src/core/hle/kernel/k_auto_object_container.h b/src/core/hle/kernel/k_auto_object_container.h index ff40cf5a71..459953450a 100644 --- a/src/core/hle/kernel/k_auto_object_container.h +++ b/src/core/hle/kernel/k_auto_object_container.h @@ -6,6 +6,8 @@ #include +#include + #include "common/assert.h" #include "common/common_funcs.h" #include "common/common_types.h" @@ -23,8 +25,7 @@ class KAutoObjectWithListContainer { YUZU_NON_MOVEABLE(KAutoObjectWithListContainer); public: - using ListType = Common::IntrusiveRedBlackTreeMemberTraits< - &KAutoObjectWithList::list_node>::TreeType; + using ListType = boost::intrusive::rbtree; public: class ListAccessor : public KScopedLightLock { diff --git a/src/core/hle/kernel/k_client_port.cpp b/src/core/hle/kernel/k_client_port.cpp index d4a38fb005..50606bd917 100644 --- a/src/core/hle/kernel/k_client_port.cpp +++ b/src/core/hle/kernel/k_client_port.cpp @@ -16,11 +16,11 @@ namespace Kernel { KClientPort::KClientPort(KernelCore& kernel_) : KSynchronizationObject{kernel_} {} KClientPort::~KClientPort() = default; -void KClientPort::Initialize(KPort* parent_, s32 max_sessions_, std::string&& name_) { +void KClientPort::Initialize(KPort* parent_port_, s32 max_sessions_, std::string&& name_) { // Set member variables. num_sessions = 0; peak_sessions = 0; - parent = parent_; + parent = parent_port_; max_sessions = max_sessions_; name = std::move(name_); } diff --git a/src/core/hle/kernel/k_client_session.h b/src/core/hle/kernel/k_client_session.h index b11d5b4e37..230e3b6b81 100644 --- a/src/core/hle/kernel/k_client_session.h +++ b/src/core/hle/kernel/k_client_session.h @@ -36,9 +36,9 @@ public: explicit KClientSession(KernelCore& kernel_); ~KClientSession() override; - void Initialize(KSession* parent_, std::string&& name_) { + void Initialize(KSession* parent_session_, std::string&& name_) { // Set member variables. - parent = parent_; + parent = parent_session_; name = std::move(name_); } diff --git a/src/core/hle/kernel/k_readable_event.h b/src/core/hle/kernel/k_readable_event.h index b2850ac7bf..149fa78dda 100644 --- a/src/core/hle/kernel/k_readable_event.h +++ b/src/core/hle/kernel/k_readable_event.h @@ -21,9 +21,9 @@ public: explicit KReadableEvent(KernelCore& kernel_); ~KReadableEvent() override; - void Initialize(KEvent* parent_, std::string&& name_) { + void Initialize(KEvent* parent_event_, std::string&& name_) { is_signaled = false; - parent = parent_; + parent = parent_event_; name = std::move(name_); } diff --git a/src/core/hle/kernel/k_server_port.cpp b/src/core/hle/kernel/k_server_port.cpp index 8cbde177aa..c5dc58387f 100644 --- a/src/core/hle/kernel/k_server_port.cpp +++ b/src/core/hle/kernel/k_server_port.cpp @@ -17,9 +17,9 @@ namespace Kernel { KServerPort::KServerPort(KernelCore& kernel_) : KSynchronizationObject{kernel_} {} KServerPort::~KServerPort() = default; -void KServerPort::Initialize(KPort* parent_, std::string&& name_) { +void KServerPort::Initialize(KPort* parent_port_, std::string&& name_) { // Set member variables. - parent = parent_; + parent = parent_port_; name = std::move(name_); } diff --git a/src/core/hle/kernel/k_server_port.h b/src/core/hle/kernel/k_server_port.h index 55481d63f4..67a36da404 100644 --- a/src/core/hle/kernel/k_server_port.h +++ b/src/core/hle/kernel/k_server_port.h @@ -29,7 +29,7 @@ public: explicit KServerPort(KernelCore& kernel_); ~KServerPort() override; - void Initialize(KPort* parent_, std::string&& name_); + void Initialize(KPort* parent_port_, std::string&& name_); /// Whether or not this server port has an HLE handler available. bool HasSessionRequestHandler() const { diff --git a/src/core/hle/kernel/k_server_session.cpp b/src/core/hle/kernel/k_server_session.cpp index e66a9198ad..3024395dde 100644 --- a/src/core/hle/kernel/k_server_session.cpp +++ b/src/core/hle/kernel/k_server_session.cpp @@ -29,10 +29,10 @@ KServerSession::KServerSession(KernelCore& kernel_) : KSynchronizationObject{ker KServerSession::~KServerSession() {} -void KServerSession::Initialize(KSession* parent_, std::string&& name_, +void KServerSession::Initialize(KSession* parent_session_, std::string&& name_, std::shared_ptr manager_) { // Set member variables. - parent = parent_; + parent = parent_session_; name = std::move(name_); if (manager_) { diff --git a/src/core/hle/kernel/k_server_session.h b/src/core/hle/kernel/k_server_session.h index 0f4b51e37a..9efd400bc7 100644 --- a/src/core/hle/kernel/k_server_session.h +++ b/src/core/hle/kernel/k_server_session.h @@ -47,7 +47,7 @@ public: void Destroy() override; - void Initialize(KSession* parent_, std::string&& name_, + void Initialize(KSession* parent_session_, std::string&& name_, std::shared_ptr manager_); KSession* GetParent() { diff --git a/src/core/hle/kernel/k_writable_event.cpp b/src/core/hle/kernel/k_writable_event.cpp index b7b83c1516..bdb1db6d5e 100644 --- a/src/core/hle/kernel/k_writable_event.cpp +++ b/src/core/hle/kernel/k_writable_event.cpp @@ -13,8 +13,8 @@ KWritableEvent::KWritableEvent(KernelCore& kernel_) KWritableEvent::~KWritableEvent() = default; -void KWritableEvent::Initialize(KEvent* parent_, std::string&& name_) { - parent = parent_; +void KWritableEvent::Initialize(KEvent* parent_event_, std::string&& name_) { + parent = parent_event_; name = std::move(name_); parent->GetReadableEvent().Open(); } -- cgit v1.2.3 From c833ca5fa076667eb3c548725dc19d4f3265f61f Mon Sep 17 00:00:00 2001 From: bunnei Date: Sun, 6 Jun 2021 17:03:36 -0700 Subject: hle: kernel: k_server_session: Ensure service thread is valid before dereference. --- src/core/hle/kernel/k_server_session.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'src/core/hle/kernel/k_server_session.cpp') diff --git a/src/core/hle/kernel/k_server_session.cpp b/src/core/hle/kernel/k_server_session.cpp index 3024395dde..96c8d7b0ee 100644 --- a/src/core/hle/kernel/k_server_session.cpp +++ b/src/core/hle/kernel/k_server_session.cpp @@ -119,9 +119,11 @@ ResultCode KServerSession::QueueSyncRequest(KThread* thread, Core::Memory::Memor context->PopulateFromIncomingCommandBuffer(kernel.CurrentProcess()->GetHandleTable(), cmd_buf); - if (auto strong_ptr = manager->GetServiceThread().lock()) { + if (auto strong_ptr = manager->GetServiceThread().lock(); strong_ptr) { strong_ptr->QueueSyncRequest(*parent, std::move(context)); return ResultSuccess; + } else { + ASSERT(false, "strong_ptr was nullptr!"); } return ResultSuccess; -- cgit v1.2.3 From ff57fd7281c8196a078d6b09d30e51cd165ab83c Mon Sep 17 00:00:00 2001 From: bunnei Date: Sun, 6 Jun 2021 17:54:06 -0700 Subject: hle: kernel: k_server_session: Return service thread by strong pointer. --- src/core/hle/kernel/hle_ipc.h | 6 +++--- src/core/hle/kernel/k_server_session.cpp | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) (limited to 'src/core/hle/kernel/k_server_session.cpp') diff --git a/src/core/hle/kernel/hle_ipc.h b/src/core/hle/kernel/hle_ipc.h index 159565203c..2aaf93fca4 100644 --- a/src/core/hle/kernel/hle_ipc.h +++ b/src/core/hle/kernel/hle_ipc.h @@ -85,8 +85,8 @@ public: */ void ClientDisconnected(KServerSession* session); - std::weak_ptr GetServiceThread() const { - return service_thread; + std::shared_ptr GetServiceThread() const { + return service_thread.lock(); } protected: @@ -152,7 +152,7 @@ public: session_handler = std::move(handler); } - std::weak_ptr GetServiceThread() const { + std::shared_ptr GetServiceThread() const { return session_handler->GetServiceThread(); } diff --git a/src/core/hle/kernel/k_server_session.cpp b/src/core/hle/kernel/k_server_session.cpp index 96c8d7b0ee..b231f8183c 100644 --- a/src/core/hle/kernel/k_server_session.cpp +++ b/src/core/hle/kernel/k_server_session.cpp @@ -119,7 +119,7 @@ ResultCode KServerSession::QueueSyncRequest(KThread* thread, Core::Memory::Memor context->PopulateFromIncomingCommandBuffer(kernel.CurrentProcess()->GetHandleTable(), cmd_buf); - if (auto strong_ptr = manager->GetServiceThread().lock(); strong_ptr) { + if (auto strong_ptr = manager->GetServiceThread(); strong_ptr) { strong_ptr->QueueSyncRequest(*parent, std::move(context)); return ResultSuccess; } else { -- cgit v1.2.3 From eeb8d0def8a36bed86f65278e7b2ddbbd8578428 Mon Sep 17 00:00:00 2001 From: bunnei Date: Sun, 6 Jun 2021 22:09:25 -0700 Subject: hle: kernel: KServerSession: Use ASSERT_MSG where appropriate. --- src/core/hle/kernel/k_server_session.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/core/hle/kernel/k_server_session.cpp') diff --git a/src/core/hle/kernel/k_server_session.cpp b/src/core/hle/kernel/k_server_session.cpp index b231f8183c..528ca86146 100644 --- a/src/core/hle/kernel/k_server_session.cpp +++ b/src/core/hle/kernel/k_server_session.cpp @@ -123,7 +123,7 @@ ResultCode KServerSession::QueueSyncRequest(KThread* thread, Core::Memory::Memor strong_ptr->QueueSyncRequest(*parent, std::move(context)); return ResultSuccess; } else { - ASSERT(false, "strong_ptr was nullptr!"); + ASSERT_MSG(false, "strong_ptr was nullptr!"); } return ResultSuccess; -- cgit v1.2.3