From c8c27f26f700e74be0b7fb982e7ca74373399974 Mon Sep 17 00:00:00 2001 From: Ryan Houdek Date: Wed, 22 Jan 2025 11:48:53 -0800 Subject: [PATCH] Review --- .../FEXCore/Debug/InternalThreadState.h | 1 + FEXCore/include/FEXCore/Utils/Profiler.h | 5 +-- Source/Common/Profiler.cpp | 12 +++---- Source/Common/Profiler.h | 28 +++++++++-------- .../LinuxSyscalls/ThreadManager.cpp | 31 +++++++++---------- .../LinuxSyscalls/ThreadManager.h | 2 +- Source/Windows/ARM64EC/Module.cpp | 4 +-- Source/Windows/Common/Profiler.cpp | 2 +- Source/Windows/Common/Profiler.h | 6 ++-- Source/Windows/WOW64/Module.cpp | 10 +++--- 10 files changed, 49 insertions(+), 52 deletions(-) diff --git a/FEXCore/include/FEXCore/Debug/InternalThreadState.h b/FEXCore/include/FEXCore/Debug/InternalThreadState.h index 5eb185e54..5c8bc424f 100644 --- a/FEXCore/include/FEXCore/Debug/InternalThreadState.h +++ b/FEXCore/include/FEXCore/Debug/InternalThreadState.h @@ -99,6 +99,7 @@ struct InternalThreadState : public FEXCore::Allocator::FEXAllocOperators { std::shared_mutex ObjectCacheRefCounter {}; + // This pointer is owned by the frontend. FEXCore::Profiler::ThreadStats* ThreadStats {}; ///< Data pointer for exclusive use by the frontend diff --git a/FEXCore/include/FEXCore/Utils/Profiler.h b/FEXCore/include/FEXCore/Utils/Profiler.h index 3653dafc4..123d672a7 100644 --- a/FEXCore/include/FEXCore/Utils/Profiler.h +++ b/FEXCore/include/FEXCore/Utils/Profiler.h @@ -26,7 +26,8 @@ struct ThreadStatsHeader { uint8_t _pad[2]; char fex_version[48]; std::atomic Head; - std::atomic Size; + std::atomic Size; + uint32_t Pad; }; struct ThreadStats { @@ -46,7 +47,7 @@ struct ThreadStats { #ifdef _M_ARM_64 /** - * @brief Get the raw cycle counter which is synchronizing. + * @brief Get the raw cycle counter with synchronizing isb. * * `CNTVCTSS_EL0` also does the same thing, but requires the FEAT_ECV feature. */ diff --git a/Source/Common/Profiler.cpp b/Source/Common/Profiler.cpp index 70001a762..1e2f662f0 100644 --- a/Source/Common/Profiler.cpp +++ b/Source/Common/Profiler.cpp @@ -14,7 +14,7 @@ void StatAllocBase::SaveHeader(FEXCore::Profiler::AppType AppType) { Head->Size.store(CurrentSize, std::memory_order_relaxed); Head->Version = FEXCore::Profiler::STATS_VERSION; - constexpr std::array::length(GIT_DESCRIBE_STRING) + 1> GitString = {GIT_DESCRIBE_STRING}; + std::string_view GitString = GIT_DESCRIBE_STRING; strncpy(Head->fex_version, GitString.data(), std::min(GitString.size(), sizeof(Head->fex_version))); Head->app_type = AppType; @@ -26,7 +26,7 @@ void StatAllocBase::SaveHeader(FEXCore::Profiler::AppType AppType) { bool StatAllocBase::AllocateMoreSlots() { const auto OriginalSlotCount = TotalSlotsFromSize(); - uint64_t NewSize = AllocateMoreSlots(CurrentSize * 2); + uint32_t NewSize = FrontendAllocateSlots(CurrentSize * 2); if (NewSize == CurrentSize) { return false; @@ -39,7 +39,7 @@ bool StatAllocBase::AllocateMoreSlots() { return true; } -FEXCore::Profiler::ThreadStats* StatAllocBase::AllocateBaseSlot(uint32_t TID) { +FEXCore::Profiler::ThreadStats* StatAllocBase::AllocateSlot(uint32_t TID) { if (!RemainingSlots) { if (!AllocateMoreSlots()) { return nullptr; @@ -47,7 +47,7 @@ FEXCore::Profiler::ThreadStats* StatAllocBase::AllocateBaseSlot(uint32_t TID) { } // Find a free slot - memory_barrier(); + store_memory_barrier(); FEXCore::Profiler::ThreadStats* AllocatedSlot {}; for (size_t i = 0; i < TotalSlotsFromSize(); ++i) { AllocatedSlot = &Stats[i]; @@ -76,7 +76,7 @@ FEXCore::Profiler::ThreadStats* StatAllocBase::AllocateBaseSlot(uint32_t TID) { return AllocatedSlot; } -void StatAllocBase::DeallocateBaseSlot(FEXCore::Profiler::ThreadStats* AllocatedSlot) { +void StatAllocBase::DeallocateSlot(FEXCore::Profiler::ThreadStats* AllocatedSlot) { if (!AllocatedSlot) { return; } @@ -84,7 +84,7 @@ void StatAllocBase::DeallocateBaseSlot(FEXCore::Profiler::ThreadStats* Allocated // TID == 0 will signal the reader to ignore this slot & deallocate it! AllocatedSlot->TID.store(0, std::memory_order_relaxed); - memory_barrier(); + store_memory_barrier(); const auto SlotOffset = OffsetFromStat(AllocatedSlot); const auto AllocatedSlotNext = AllocatedSlot->Next.load(std::memory_order_relaxed); diff --git a/Source/Common/Profiler.h b/Source/Common/Profiler.h index 023678ce8..f39c91861 100644 --- a/Source/Common/Profiler.h +++ b/Source/Common/Profiler.h @@ -13,43 +13,45 @@ struct InternalThreadState; } #ifdef _M_ARM_64 -static inline void memory_barrier() { +static inline void store_memory_barrier() { asm volatile("dmb ishst;" ::: "memory"); } #else -static inline void memory_barrier() { +static inline void store_memory_barrier() { // Intentionally empty. + // x86 is strongly memory ordered with regular loadstores. No need for barrier. } #endif namespace FEX::Profiler { class StatAllocBase { -public: - virtual ~StatAllocBase() = default; - protected: - FEXCore::Profiler::ThreadStats* AllocateBaseSlot(uint32_t TID); - void DeallocateBaseSlot(FEXCore::Profiler::ThreadStats* AllocatedSlot); + FEXCore::Profiler::ThreadStats* AllocateSlot(uint32_t TID); + void DeallocateSlot(FEXCore::Profiler::ThreadStats* AllocatedSlot); uint32_t OffsetFromStat(FEXCore::Profiler::ThreadStats* Stat) const { return reinterpret_cast(Stat) - reinterpret_cast(Base); } - size_t TotalSlotsFromSize() const { + uint32_t TotalSlotsFromSize() const { return (CurrentSize - sizeof(FEXCore::Profiler::ThreadStatsHeader)) / sizeof(FEXCore::Profiler::ThreadStats) - 1; } - size_t SlotIndexFromOffset(uint32_t Offset) { + static uint32_t TotalSlotsFromSize(uint32_t Size) { + return (Size - sizeof(FEXCore::Profiler::ThreadStatsHeader)) / sizeof(FEXCore::Profiler::ThreadStats) - 1; + } + + static uint32_t SlotIndexFromOffset(uint32_t Offset) { return (Offset - sizeof(FEXCore::Profiler::ThreadStatsHeader)) / sizeof(FEXCore::Profiler::ThreadStats); } void SaveHeader(FEXCore::Profiler::AppType AppType); void* Base; - size_t CurrentSize {}; + uint32_t CurrentSize {}; FEXCore::Profiler::ThreadStatsHeader* Head {}; FEXCore::Profiler::ThreadStats* Stats; FEXCore::Profiler::ThreadStats* StatTail {}; - uint64_t RemainingSlots; + uint32_t RemainingSlots; // Limited to 4MB which should be a few hundred threads of tracking capability. // I (Sonicadvance1) wanted to reserve 128MB of VA space because it's cheap, but ran in to a bug when running WINE. @@ -57,10 +59,10 @@ protected: // This only occurs when this stat allocation size is large as the top-down allocation pushes the alt-stack further. // Additionally, only occurs on 48-bit VA systems, as mmap on lesser VA will fail regardless. // TODO: Bump allocation size up once FEXCore's allocator can first use the 128TB of blocked VA space on 48-bit systems. - constexpr static size_t MAX_STATS_SIZE = 4 * 1024 * 1024; + constexpr static uint32_t MAX_STATS_SIZE = 4 * 1024 * 1024; private: - virtual uint64_t AllocateMoreSlots(uint64_t NewSize) = 0; + virtual uint32_t FrontendAllocateSlots(uint32_t NewSize) = 0; bool AllocateMoreSlots(); }; diff --git a/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.cpp b/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.cpp index 3a1d156bc..742cac538 100644 --- a/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.cpp +++ b/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.cpp @@ -25,7 +25,7 @@ void ThreadManager::StatAlloc::Initialize() { } int fd = shm_open(fextl::fmt::format("fex-{}-stats", ::getpid()).c_str(), O_CREAT | O_TRUNC | O_RDWR, USER_PERMS); - if (!fd) { + if (fd == -1) { return; } CurrentSize = sysconf(_SC_PAGESIZE); @@ -38,7 +38,11 @@ void ThreadManager::StatAlloc::Initialize() { goto err; } - // 128MB ought to be enough for anyone. + // Reserve a region of MAX_STATS_SIZE so we can grow the allocation buffer. + // Number of thread slots when ThreadStatsHeader == 64bytes and ThreadStats == 40bytes: + // 1 page: 99 slots + // 1 MB: 26211 slots + // 128 MB: 3355440 slots Base = ::mmap(nullptr, MAX_STATS_SIZE, PROT_NONE, MAP_PRIVATE | MAP_ANONYMOUS | MAP_NORESERVE, -1, 0); if (Base == MAP_FAILED) { LogMan::Msg::EFmt("[StatAlloc] mmap base failed"); @@ -61,9 +65,10 @@ err: close(fd); } -uint64_t ThreadManager::StatAlloc::AllocateMoreSlots(uint64_t NewSize) { +uint32_t ThreadManager::StatAlloc::FrontendAllocateSlots(uint32_t NewSize) { if (CurrentSize == MAX_STATS_SIZE) { - // Nope. + // Allocator has reached maximum slots. We can't allocate anymore. + // New threads won't get stats. return CurrentSize; } NewSize = std::max(MAX_STATS_SIZE, NewSize); @@ -86,14 +91,6 @@ uint64_t ThreadManager::StatAlloc::AllocateMoreSlots(uint64_t NewSize) { LogMan::Msg::EFmt("[StatAlloc] allocate more mmap shm failed"); goto err; } - - // TODO: Just a sanity check. - const char* SharedTest = (const char*)Base; - for (size_t i = CurrentSize; i < NewSize; ++i) { - if (SharedTest[i] != 0) { - LogMan::Msg::EFmt("truncate and map shared resulted in not zero'd memory!"); - } - } } err: @@ -103,7 +100,7 @@ err: FEXCore::Profiler::ThreadStats* ThreadManager::StatAlloc::AllocateSlot(uint32_t TID) { std::scoped_lock lk(StatMutex); - return AllocateBaseSlot(TID); + return StatAllocBase::AllocateSlot(TID); } void ThreadManager::StatAlloc::DeallocateSlot(FEXCore::Profiler::ThreadStats* AllocatedSlot) { @@ -112,7 +109,7 @@ void ThreadManager::StatAlloc::DeallocateSlot(FEXCore::Profiler::ThreadStats* Al } std::scoped_lock lk(StatMutex); - DeallocateBaseSlot(AllocatedSlot); + StatAllocBase::DeallocateSlot(AllocatedSlot); } void ThreadManager::StatAlloc::CleanupForExit() { @@ -138,8 +135,8 @@ void ThreadManager::StatAlloc::UnlockAfterFork(FEXCore::Core::InternalThreadStat StatMutex.StealAndDropActiveLocks(); - // shm_memory tied to this process is now not owned by this process. - // Replace the shm region! Otherwise this process will keep reporting time in the original parent thread's stats region! + // shm_memory ownership is retained by the parent process, so the child must replace it with its own one. + // Otherwise this process will keep reporting in the original parent thread's stats region. munmap(Base, MAX_STATS_SIZE); Base = nullptr; CurrentSize = 0; @@ -380,7 +377,7 @@ void ThreadManager::UnlockAfterFork(FEXCore::Core::InternalThreadState* LiveThre // This function is called after fork // We need to cleanup some of the thread data that is dead for (auto& DeadThread : Threads) { - // This is not owned by the child after fork. + // The fork parent retains ownership of ThreadStats DeadThread->Thread->ThreadStats = nullptr; if (DeadThread->Thread == LiveThread) { diff --git a/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.h b/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.h index 949088853..8fae49713 100644 --- a/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.h +++ b/Source/Tools/LinuxEmulation/LinuxSyscalls/ThreadManager.h @@ -123,7 +123,7 @@ public: private: void Initialize(); - uint64_t AllocateMoreSlots(uint64_t NewSize) override; + uint32_t FrontendAllocateSlots(uint32_t NewSize) override; FEX_CONFIG_OPT(ProfileStats, PROFILESTATS); FEX_CONFIG_OPT(Is64BitMode, IS64BIT_MODE); diff --git a/Source/Windows/ARM64EC/Module.cpp b/Source/Windows/ARM64EC/Module.cpp index 39bb2710b..aa3104c28 100644 --- a/Source/Windows/ARM64EC/Module.cpp +++ b/Source/Windows/ARM64EC/Module.cpp @@ -580,9 +580,7 @@ NTSTATUS ProcessInit() { return STATUS_SUCCESS; } -void ProcessTerm(HANDLE Handle, BOOL After, NTSTATUS Status) { - StatAllocHandler.reset(); -} +void ProcessTerm(HANDLE Handle, BOOL After, NTSTATUS Status) {} class ScopedCallbackDisable { private: diff --git a/Source/Windows/Common/Profiler.cpp b/Source/Windows/Common/Profiler.cpp index 2dedde586..6e24be04c 100644 --- a/Source/Windows/Common/Profiler.cpp +++ b/Source/Windows/Common/Profiler.cpp @@ -20,7 +20,7 @@ __attribute__((naked)) uint64_t linux_getpid() { : "r0", "r8"); } -uint64_t StatAlloc::AllocateMoreSlots(uint64_t NewSize) { +uint32_t StatAlloc::FrontendAllocateSlots(uint32_t NewSize) { LogMan::Msg::DFmt("Ran out of slots. Can't allocate more"); return CurrentSize; } diff --git a/Source/Windows/Common/Profiler.h b/Source/Windows/Common/Profiler.h index b12ad9631..6bdd0f87f 100644 --- a/Source/Windows/Common/Profiler.h +++ b/Source/Windows/Common/Profiler.h @@ -10,7 +10,7 @@ public: virtual ~StatAlloc(); FEXCore::Profiler::ThreadStats* AllocateSlot(uint32_t TID) { - return AllocateBaseSlot(TID); + return StatAllocBase::AllocateSlot(TID); } void DeallocateSlot(FEXCore::Profiler::ThreadStats* AllocatedSlot) { @@ -18,11 +18,11 @@ public: return; } - DeallocateBaseSlot(AllocatedSlot); + StatAllocBase::DeallocateSlot(AllocatedSlot); } private: - uint64_t AllocateMoreSlots(uint64_t NewSize) override; + uint32_t FrontendAllocateSlots(uint32_t NewSize) override; }; } // namespace FEX::Windows diff --git a/Source/Windows/WOW64/Module.cpp b/Source/Windows/WOW64/Module.cpp index 8ca8144db..422e10ecd 100644 --- a/Source/Windows/WOW64/Module.cpp +++ b/Source/Windows/WOW64/Module.cpp @@ -510,9 +510,7 @@ void BTCpuProcessInit() { } } -void BTCpuProcessTerm(HANDLE Handle, BOOL After, ULONG Status) { - StatAllocHandler.reset(); -} +void BTCpuProcessTerm(HANDLE Handle, BOOL After, ULONG Status) {} void BTCpuThreadInit() { FEX::Windows::InitCRTThread(); @@ -534,7 +532,7 @@ void BTCpuThreadTerm(HANDLE Thread, LONG ExitCode) { return; } - auto* OldThreadState = TLS.ThreadState(); + auto* ThreadState = TLS.ThreadState(); THREAD_BASIC_INFORMATION Info; if (NTSTATUS Err = NtQueryInformationThread(Thread, ThreadBasicInformation, &Info, sizeof(Info), nullptr); Err) { @@ -546,11 +544,11 @@ void BTCpuThreadTerm(HANDLE Thread, LONG ExitCode) { std::scoped_lock Lock(ThreadCreationMutex); Threads.erase(ThreadTID); if (StatAllocHandler) { - StatAllocHandler->DeallocateSlot(OldThreadState->ThreadStats); + StatAllocHandler->DeallocateSlot(ThreadState->ThreadStats); } } - CTX->DestroyThread(OldThreadState); + CTX->DestroyThread(ThreadState); if (ThreadTID == GetCurrentThreadId()) { FEX::Windows::DeinitCRTThread(); }