From 27ce97fd42d758350c5100c4bbcb78de0a6d48b5 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/hle_ipc.cpp | 11 ++++++++-- src/core/hle/kernel/hle_ipc.h | 22 +++++++++++++++++-- src/core/hle/kernel/k_client_port.cpp | 5 +++-- src/core/hle/kernel/k_client_port.h | 4 +++- src/core/hle/kernel/k_server_session.cpp | 21 +++++++++++------- src/core/hle/kernel/k_server_session.h | 17 +++------------ src/core/hle/kernel/k_session.cpp | 5 +++-- src/core/hle/kernel/k_session.h | 5 ++++- src/core/hle/service/ns/pl_u.cpp | 2 -- src/core/hle/service/service.cpp | 6 +++--- src/core/hle/service/service.h | 5 +++-- src/core/hle/service/sm/controller.cpp | 37 +++++++++----------------------- src/core/hle/service/sm/sm.cpp | 2 +- 13 files changed, 75 insertions(+), 67 deletions(-) (limited to 'src/core/hle') diff --git a/src/core/hle/kernel/hle_ipc.cpp b/src/core/hle/kernel/hle_ipc.cpp index 2b5c30f7a..eee695dd9 100644 --- a/src/core/hle/kernel/hle_ipc.cpp +++ b/src/core/hle/kernel/hle_ipc.cpp @@ -30,9 +30,16 @@ namespace Kernel { -SessionRequestHandler::SessionRequestHandler() = default; +SessionRequestHandler::SessionRequestHandler(KernelCore& kernel_, const char* service_name_) + : kernel{kernel_}, service_thread{kernel.CreateServiceThread(service_name_)} {} -SessionRequestHandler::~SessionRequestHandler() = default; +SessionRequestHandler::~SessionRequestHandler() { + kernel.ReleaseServiceThread(service_thread); +} + +SessionRequestManager::SessionRequestManager(KernelCore& kernel_) : kernel{kernel_} {} + +SessionRequestManager::~SessionRequestManager() {} void SessionRequestHandler::ClientConnected(KServerSession* session) { session->SetSessionHandler(shared_from_this()); diff --git a/src/core/hle/kernel/hle_ipc.h b/src/core/hle/kernel/hle_ipc.h index b47e363cc..159565203 100644 --- a/src/core/hle/kernel/hle_ipc.h +++ b/src/core/hle/kernel/hle_ipc.h @@ -46,6 +46,7 @@ class KThread; class KReadableEvent; class KSession; class KWritableEvent; +class ServiceThread; enum class ThreadWakeupReason; @@ -56,7 +57,7 @@ enum class ThreadWakeupReason; */ class SessionRequestHandler : public std::enable_shared_from_this { public: - SessionRequestHandler(); + SessionRequestHandler(KernelCore& kernel, const char* service_name_); virtual ~SessionRequestHandler(); /** @@ -83,6 +84,14 @@ public: * @param server_session ServerSession associated with the connection. */ void ClientDisconnected(KServerSession* session); + + std::weak_ptr GetServiceThread() const { + return service_thread; + } + +protected: + KernelCore& kernel; + std::weak_ptr service_thread; }; using SessionRequestHandlerPtr = std::shared_ptr; @@ -94,7 +103,8 @@ using SessionRequestHandlerPtr = std::shared_ptr; */ class SessionRequestManager final { public: - SessionRequestManager() = default; + explicit SessionRequestManager(KernelCore& kernel); + ~SessionRequestManager(); bool IsDomain() const { return is_domain; @@ -142,10 +152,18 @@ public: session_handler = std::move(handler); } + std::weak_ptr GetServiceThread() const { + return session_handler->GetServiceThread(); + } + private: bool is_domain{}; SessionRequestHandlerPtr session_handler; std::vector domain_handlers; + +private: + KernelCore& kernel; + std::weak_ptr service_thread; }; /** diff --git a/src/core/hle/kernel/k_client_port.cpp b/src/core/hle/kernel/k_client_port.cpp index 23d830d1f..d4a38fb00 100644 --- a/src/core/hle/kernel/k_client_port.cpp +++ b/src/core/hle/kernel/k_client_port.cpp @@ -56,7 +56,8 @@ bool KClientPort::IsSignaled() const { return num_sessions < max_sessions; } -ResultCode KClientPort::CreateSession(KClientSession** out) { +ResultCode KClientPort::CreateSession(KClientSession** out, + std::shared_ptr session_manager) { // Reserve a new session from the resource limit. KScopedResourceReservation session_reservation(kernel.CurrentProcess()->GetResourceLimit(), LimitableResource::Sessions); @@ -101,7 +102,7 @@ ResultCode KClientPort::CreateSession(KClientSession** out) { } // Initialize the session. - session->Initialize(this, parent->GetName()); + session->Initialize(this, parent->GetName(), session_manager); // Commit the session reservation. session_reservation.Commit(); diff --git a/src/core/hle/kernel/k_client_port.h b/src/core/hle/kernel/k_client_port.h index f2fff3b01..54bb05e20 100644 --- a/src/core/hle/kernel/k_client_port.h +++ b/src/core/hle/kernel/k_client_port.h @@ -16,6 +16,7 @@ namespace Kernel { class KClientSession; class KernelCore; class KPort; +class SessionRequestManager; class KClientPort final : public KSynchronizationObject { KERNEL_AUTOOBJECT_TRAITS(KClientPort, KSynchronizationObject); @@ -52,7 +53,8 @@ public: void Destroy() override; bool IsSignaled() const override; - ResultCode CreateSession(KClientSession** out); + ResultCode CreateSession(KClientSession** out, + std::shared_ptr session_manager = nullptr); private: std::atomic num_sessions{}; diff --git a/src/core/hle/kernel/k_server_session.cpp b/src/core/hle/kernel/k_server_session.cpp index dbf03b462..e66a9198a 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; } diff --git a/src/core/hle/kernel/k_server_session.h b/src/core/hle/kernel/k_server_session.h index 27b757ad2..0f4b51e37 100644 --- a/src/core/hle/kernel/k_server_session.h +++ b/src/core/hle/kernel/k_server_session.h @@ -32,6 +32,7 @@ class HLERequestContext; class KernelCore; class KSession; class SessionRequestHandler; +class SessionRequestManager; class KThread; class KServerSession final : public KSynchronizationObject, @@ -46,7 +47,8 @@ public: void Destroy() override; - void Initialize(KSession* parent_, std::string&& name_); + void Initialize(KSession* parent_, std::string&& name_, + std::shared_ptr manager_); KSession* GetParent() { return parent; @@ -104,16 +106,6 @@ public: return manager; } - /// Gets the session request manager, which forwards requests to the underlying service - const std::shared_ptr& GetSessionRequestManager() const { - return manager; - } - - /// Sets the session request manager, which forwards requests to the underlying service - void SetSessionRequestManager(std::shared_ptr manager_) { - manager = std::move(manager_); - } - private: /// Queues a sync request from the emulated application. ResultCode QueueSyncRequest(KThread* thread, Core::Memory::Memory& memory); @@ -131,9 +123,6 @@ private: /// When set to True, converts the session to a domain at the end of the command bool convert_to_domain{}; - /// Thread to dispatch service requests - std::weak_ptr service_thread; - /// KSession that owns this KServerSession KSession* parent{}; }; diff --git a/src/core/hle/kernel/k_session.cpp b/src/core/hle/kernel/k_session.cpp index 025b8b555..940878e03 100644 --- a/src/core/hle/kernel/k_session.cpp +++ b/src/core/hle/kernel/k_session.cpp @@ -15,7 +15,8 @@ KSession::KSession(KernelCore& kernel_) : KAutoObjectWithSlabHeapAndContainer{kernel_}, server{kernel_}, client{kernel_} {} KSession::~KSession() = default; -void KSession::Initialize(KClientPort* port_, const std::string& name_) { +void KSession::Initialize(KClientPort* port_, const std::string& name_, + std::shared_ptr manager_) { // Increment reference count. // Because reference count is one on creation, this will result // in a reference count of two. Thus, when both server and client are closed @@ -27,7 +28,7 @@ void KSession::Initialize(KClientPort* port_, const std::string& name_) { KAutoObject::Create(std::addressof(client)); // Initialize our sub sessions. - server.Initialize(this, name_ + ":Server"); + server.Initialize(this, name_ + ":Server", manager_); client.Initialize(this, name_ + ":Client"); // Set state and name. diff --git a/src/core/hle/kernel/k_session.h b/src/core/hle/kernel/k_session.h index 4ddd080d2..62c328a68 100644 --- a/src/core/hle/kernel/k_session.h +++ b/src/core/hle/kernel/k_session.h @@ -13,6 +13,8 @@ namespace Kernel { +class SessionRequestManager; + class KSession final : public KAutoObjectWithSlabHeapAndContainer { KERNEL_AUTOOBJECT_TRAITS(KSession, KAutoObject); @@ -20,7 +22,8 @@ public: explicit KSession(KernelCore& kernel_); ~KSession() override; - void Initialize(KClientPort* port_, const std::string& name_); + void Initialize(KClientPort* port_, const std::string& name_, + std::shared_ptr manager_ = nullptr); void Finalize() override; diff --git a/src/core/hle/service/ns/pl_u.cpp b/src/core/hle/service/ns/pl_u.cpp index 6e5ba26a3..74cc45f1e 100644 --- a/src/core/hle/service/ns/pl_u.cpp +++ b/src/core/hle/service/ns/pl_u.cpp @@ -254,8 +254,6 @@ void PL_U::GetSharedMemoryNativeHandle(Kernel::HLERequestContext& ctx) { LOG_DEBUG(Service_NS, "called"); // Create shared font memory object - auto& kernel = system.Kernel(); - std::memcpy(kernel.GetFontSharedMem().GetPointer(), impl->shared_font->data(), impl->shared_font->size()); diff --git a/src/core/hle/service/service.cpp b/src/core/hle/service/service.cpp index 7a15eeba0..4e1541630 100644 --- a/src/core/hle/service/service.cpp +++ b/src/core/hle/service/service.cpp @@ -93,8 +93,8 @@ namespace Service { ServiceFrameworkBase::ServiceFrameworkBase(Core::System& system_, const char* service_name_, u32 max_sessions_, InvokerFn* handler_invoker_) - : system{system_}, service_name{service_name_}, max_sessions{max_sessions_}, - handler_invoker{handler_invoker_} {} + : SessionRequestHandler(system_.Kernel(), service_name_), system{system_}, + service_name{service_name_}, max_sessions{max_sessions_}, handler_invoker{handler_invoker_} {} ServiceFrameworkBase::~ServiceFrameworkBase() { // Wait for other threads to release access before destroying @@ -111,7 +111,7 @@ void ServiceFrameworkBase::InstallAsService(SM::ServiceManager& service_manager) port_installed = true; } -Kernel::KClientPort& ServiceFrameworkBase::CreatePort(Kernel::KernelCore& kernel) { +Kernel::KClientPort& ServiceFrameworkBase::CreatePort() { const auto guard = LockService(); ASSERT(!port_installed); diff --git a/src/core/hle/service/service.h b/src/core/hle/service/service.h index 4c048173b..ec757753c 100644 --- a/src/core/hle/service/service.h +++ b/src/core/hle/service/service.h @@ -23,6 +23,7 @@ namespace Kernel { class HLERequestContext; class KClientPort; class KServerSession; +class ServiceThread; } // namespace Kernel namespace Service { @@ -41,7 +42,7 @@ class ServiceManager; static const int kMaxPortSize = 8; ///< Maximum size of a port name (8 characters) /// Arbitrary default number of maximum connections to an HLE service. -static const u32 DefaultMaxSessions = 10; +static const u32 DefaultMaxSessions = 64; /** * This is an non-templated base of ServiceFramework to reduce code bloat and compilation times, it @@ -74,7 +75,7 @@ public: void InvokeRequestTipc(Kernel::HLERequestContext& ctx); /// Creates a port pair and registers it on the kernel's global port registry. - Kernel::KClientPort& CreatePort(Kernel::KernelCore& kernel); + Kernel::KClientPort& CreatePort(); /// Handles a synchronization request for the service. ResultCode HandleSyncRequest(Kernel::KServerSession& session, diff --git a/src/core/hle/service/sm/controller.cpp b/src/core/hle/service/sm/controller.cpp index 5fa5e0512..8b9418e0f 100644 --- a/src/core/hle/service/sm/controller.cpp +++ b/src/core/hle/service/sm/controller.cpp @@ -28,42 +28,25 @@ void Controller::ConvertCurrentObjectToDomain(Kernel::HLERequestContext& ctx) { } void Controller::CloneCurrentObject(Kernel::HLERequestContext& ctx) { - // TODO(bunnei): This is just creating a new handle to the same Session. I assume this is wrong - // and that we probably want to actually make an entirely new Session, but we still need to - // verify this on hardware. - LOG_DEBUG(Service, "called"); - auto& kernel = system.Kernel(); - auto* session = ctx.Session()->GetParent(); - auto* port = session->GetParent()->GetParent(); + auto& parent_session = *ctx.Session()->GetParent(); + auto& parent_port = parent_session.GetParent()->GetParent()->GetClientPort(); + auto& session_manager = parent_session.GetServerSession().GetSessionRequestManager(); - // Reserve a new session from the process resource limit. - Kernel::KScopedResourceReservation session_reservation( - kernel.CurrentProcess()->GetResourceLimit(), Kernel::LimitableResource::Sessions); - if (!session_reservation.Succeeded()) { + // Create a session. + Kernel::KClientSession* session{}; + const ResultCode result = parent_port.CreateSession(std::addressof(session), session_manager); + if (result.IsError()) { + LOG_CRITICAL(Service, "CreateSession failed with error 0x{:08X}", result.raw); IPC::ResponseBuilder rb{ctx, 2}; - rb.Push(Kernel::ResultLimitReached); + rb.Push(result); } - // Create a new session. - auto* clone = Kernel::KSession::Create(kernel); - clone->Initialize(&port->GetClientPort(), session->GetName()); - - // Commit the session reservation. - session_reservation.Commit(); - - // Enqueue the session with the named port. - port->EnqueueSession(&clone->GetServerSession()); - - // Set the session request manager. - clone->GetServerSession().SetSessionRequestManager( - session->GetServerSession().GetSessionRequestManager()); - // We succeeded. IPC::ResponseBuilder rb{ctx, 2, 0, 1, IPC::ResponseBuilder::Flags::AlwaysMoveHandles}; rb.Push(ResultSuccess); - rb.PushMoveObjects(clone->GetClientSession()); + rb.PushMoveObjects(session); } void Controller::CloneCurrentObjectEx(Kernel::HLERequestContext& ctx) { diff --git a/src/core/hle/service/sm/sm.cpp b/src/core/hle/service/sm/sm.cpp index d8b20a3f2..bffa9ffcb 100644 --- a/src/core/hle/service/sm/sm.cpp +++ b/src/core/hle/service/sm/sm.cpp @@ -46,7 +46,7 @@ Kernel::KClientPort& ServiceManager::InterfaceFactory(ServiceManager& self, Core self.sm_interface = sm; self.controller_interface = std::make_unique(system); - return sm->CreatePort(system.Kernel()); + return sm->CreatePort(); } ResultVal ServiceManager::RegisterService(std::string name, -- cgit v1.2.3