From f95b81574f3bba2da01ad81a1e7e1d0832911265 Mon Sep 17 00:00:00 2001 From: Eladash Date: Mon, 11 May 2020 21:24:04 +0300 Subject: [PATCH] sys_spu: Fix race in sys_spu_thread_group_destroy and other minor fixes (#8182) * sys_spu: Fix race in sys_spu_thread_group_destroy and other minor fixes * SPU: Wait for all threads to have error codes if exited by sys_spu_thread_exit On last thread in group to run. * sys_spu: Fix sys_spu_thread_group_start * fixup ad fix sys_spu_thread_group_terminate idk why "- !group->running" was put in the first place but its probably no longer relevant due to other changes and was causing other issues such as not always waiting for last SPU thread to set group state to INITIALIZED. --- rpcs3/Emu/Cell/SPUThread.cpp | 15 ++- rpcs3/Emu/Cell/SPUThread.h | 2 +- rpcs3/Emu/Cell/lv2/sys_spu.cpp | 229 +++++++++++++++++++++++++-------- rpcs3/Emu/Cell/lv2/sys_spu.h | 1 + 4 files changed, 191 insertions(+), 56 deletions(-) diff --git a/rpcs3/Emu/Cell/SPUThread.cpp b/rpcs3/Emu/Cell/SPUThread.cpp index 6eee07a8cc..c41452ad18 100644 --- a/rpcs3/Emu/Cell/SPUThread.cpp +++ b/rpcs3/Emu/Cell/SPUThread.cpp @@ -991,6 +991,19 @@ void spu_thread::cpu_stop() group->join_state = SYS_SPU_THREAD_GROUP_JOIN_ALL_THREADS_EXIT; } + for (const auto& thread : group->threads) + { + if (thread && thread.get() != this && thread->status_npc.load().status >> 16 == SYS_SPU_THREAD_STOP_THREAD_EXIT) + { + // Wait for all threads to have error codes if exited by sys_spu_thread_exit + for (u64 status; ((status = thread->exit_status.data) & spu_channel::bit_count) == 0 + || static_cast(status) != thread->last_exit_status;) + { + _mm_pause(); + } + } + } + if (status_npc.load().status >> 16 == SYS_SPU_THREAD_STOP_THREAD_EXIT) { // Set exit status now, in conjunction with group state changes @@ -3135,7 +3148,7 @@ bool spu_thread::stop_and_signal(u32 code) const u32 value = ch_out_mbox.get_value(); spu_log.trace("sys_spu_thread_exit(status=0x%x)", value); - last_exit_status = value; + last_exit_status.release(value); set_status_npc(); state += cpu_flag::stop; check_state(); diff --git a/rpcs3/Emu/Cell/SPUThread.h b/rpcs3/Emu/Cell/SPUThread.h index 2dffd26620..34767fde85 100644 --- a/rpcs3/Emu/Cell/SPUThread.h +++ b/rpcs3/Emu/Cell/SPUThread.h @@ -600,7 +600,7 @@ public: std::array>, 32> spuq; // Event Queue Keys for SPU Thread std::weak_ptr spup[64]; // SPU Ports spu_channel exit_status{}; // Threaded SPU exit status (not a channel, but the interface fits) - u32 last_exit_status; // Value to be written in exit_status after checking group termination + atomic_t last_exit_status; // Value to be written in exit_status after checking group termination const u32 index; // SPU index const u32 offset; // SPU LS offset diff --git a/rpcs3/Emu/Cell/lv2/sys_spu.cpp b/rpcs3/Emu/Cell/lv2/sys_spu.cpp index 5168563c4c..56114f3d16 100644 --- a/rpcs3/Emu/Cell/lv2/sys_spu.cpp +++ b/rpcs3/Emu/Cell/lv2/sys_spu.cpp @@ -40,6 +40,7 @@ void fmt_class_string::format(std::string& out, u64 arg) case SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED: return "waiting and suspended"; case SPU_THREAD_GROUP_STATUS_RUNNING: return "running"; case SPU_THREAD_GROUP_STATUS_STOPPED: return "stopped"; + case SPU_THREAD_GROUP_STATUS_DESTROYED: return "destroyed"; case SPU_THREAD_GROUP_STATUS_UNKNOWN: break; } @@ -376,7 +377,17 @@ error_code sys_spu_thread_initialize(ppu_thread& ppu, vm::ptr thread, u32 g std::lock_guard lock(group->mutex); - if (group->threads_map[spu_num] != -1 || group->run_state != SPU_THREAD_GROUP_STATUS_NOT_INITIALIZED) + if (auto state = +group->run_state; state != SPU_THREAD_GROUP_STATUS_NOT_INITIALIZED) + { + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } + + return CELL_EBUSY; + } + + if (group->threads_map[spu_num] != -1) { return CELL_EBUSY; } @@ -426,7 +437,12 @@ error_code sys_spu_thread_initialize(ppu_thread& ppu, vm::ptr thread, u32 g } } - group->run_state = SPU_THREAD_GROUP_STATUS_INITIALIZED; + const auto old = group->run_state.compare_and_swap(SPU_THREAD_GROUP_STATUS_NOT_INITIALIZED, SPU_THREAD_GROUP_STATUS_INITIALIZED); + + if (old == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } } sys_spu.warning(u8"sys_spu_thread_initialize(): Thread “%s” created (id=0x%x)", thread_name, tid); @@ -631,9 +647,16 @@ error_code sys_spu_thread_group_destroy(ppu_thread& ppu, u32 id) const auto group = idm::withdraw(id, [](lv2_spu_group& group) -> CellError { - const auto _old = group.run_state.compare_and_swap(SPU_THREAD_GROUP_STATUS_INITIALIZED, SPU_THREAD_GROUP_STATUS_NOT_INITIALIZED); + if (!group.run_state.fetch_op([](spu_group_status& state) + { + if (state <= SPU_THREAD_GROUP_STATUS_INITIALIZED) + { + state = SPU_THREAD_GROUP_STATUS_DESTROYED; + return true; + } - if (_old > SPU_THREAD_GROUP_STATUS_INITIALIZED) + return false; + }).second) { return CELL_EBUSY; } @@ -652,6 +675,8 @@ error_code sys_spu_thread_group_destroy(ppu_thread& ppu, u32 id) return group.ret; } + group->mutex.lock_unlock(); + for (const auto& t : group->threads) { if (auto thread = t.get()) @@ -670,24 +695,23 @@ error_code sys_spu_thread_group_start(ppu_thread& ppu, u32 id) sys_spu.trace("sys_spu_thread_group_start(id=0x%x)", id); - const auto group = idm::get(id, [](lv2_spu_group& group) - { - // SPU_THREAD_GROUP_STATUS_READY state is not used - return group.run_state.compare_and_swap_test(SPU_THREAD_GROUP_STATUS_INITIALIZED, SPU_THREAD_GROUP_STATUS_RUNNING); - }); + const auto group = idm::get(id); if (!group) { return CELL_ESRCH; } - if (!group.ret) - { - return CELL_ESTAT; - } - std::lock_guard lock(group->mutex); + // SPU_THREAD_GROUP_STATUS_READY state is not used + switch (group->run_state.compare_and_swap(SPU_THREAD_GROUP_STATUS_INITIALIZED, SPU_THREAD_GROUP_STATUS_RUNNING)) + { + case SPU_THREAD_GROUP_STATUS_INITIALIZED: break; + case SPU_THREAD_GROUP_STATUS_DESTROYED: return CELL_ESRCH; + default: return CELL_ESTAT; + } + const u32 max_threads = group->max_run; group->join_state = 0; @@ -757,28 +781,56 @@ error_code sys_spu_thread_group_suspend(ppu_thread& ppu, u32 id) std::lock_guard lock(group->mutex); - if (group->run_state <= SPU_THREAD_GROUP_STATUS_INITIALIZED || group->run_state == SPU_THREAD_GROUP_STATUS_STOPPED) - { - return CELL_ESTAT; - } + CellError error; - // SPU_THREAD_GROUP_STATUS_READY state is not used + group->run_state.fetch_op([&error](spu_group_status& state) + { + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + error = CELL_ESRCH; + return false; + } - if (group->run_state == SPU_THREAD_GROUP_STATUS_RUNNING) + if (state <= SPU_THREAD_GROUP_STATUS_INITIALIZED || state == SPU_THREAD_GROUP_STATUS_STOPPED) + { + error = CELL_ESTAT; + return false; + } + + // SPU_THREAD_GROUP_STATUS_READY state is not used + + if (state == SPU_THREAD_GROUP_STATUS_RUNNING) + { + state = SPU_THREAD_GROUP_STATUS_SUSPENDED; + } + else if (state == SPU_THREAD_GROUP_STATUS_WAITING) + { + state = SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED; + } + else if (state == SPU_THREAD_GROUP_STATUS_SUSPENDED || state == SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED) + { + error = {}; + return false; + } + else + { + + error = CELL_ESTAT; + return false; + } + + error = CellError{CELL_CANCEL}; + return true; + }); + + if (error != CELL_CANCEL + 0u) { - group->run_state = SPU_THREAD_GROUP_STATUS_SUSPENDED; - } - else if (group->run_state == SPU_THREAD_GROUP_STATUS_WAITING) - { - group->run_state = SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED; - } - else if (group->run_state == SPU_THREAD_GROUP_STATUS_SUSPENDED || group->run_state == SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED) - { - return CELL_OK; - } - else - { - return CELL_ESTAT; + if (!error) + { + return CELL_OK; + } + + return error; } for (auto& thread : group->threads) @@ -812,19 +864,39 @@ error_code sys_spu_thread_group_resume(ppu_thread& ppu, u32 id) std::lock_guard lock(group->mutex); - // SPU_THREAD_GROUP_STATUS_READY state is not used + CellError error; - if (group->run_state == SPU_THREAD_GROUP_STATUS_SUSPENDED) + group->run_state.fetch_op([&error](spu_group_status& state) { - group->run_state = SPU_THREAD_GROUP_STATUS_RUNNING; - } - else if (group->run_state == SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED) + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + error = CELL_ESRCH; + return false; + } + + // SPU_THREAD_GROUP_STATUS_READY state is not used + + if (state == SPU_THREAD_GROUP_STATUS_SUSPENDED) + { + state = SPU_THREAD_GROUP_STATUS_RUNNING; + } + else if (state == SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED) + { + state = SPU_THREAD_GROUP_STATUS_WAITING; + } + else + { + error = CELL_ESTAT; + return false; + } + + error = CellError{CELL_CANCEL}; + return true; + }); + + if (error != CELL_CANCEL + 0u) { - group->run_state = SPU_THREAD_GROUP_STATUS_WAITING; - } - else - { - return CELL_ESTAT; + return error; } for (auto& thread : group->threads) @@ -858,8 +930,13 @@ error_code sys_spu_thread_group_yield(ppu_thread& ppu, u32 id) return CELL_OK; } - if (group->run_state != SPU_THREAD_GROUP_STATUS_RUNNING) + if (auto state = +group->run_state; state != SPU_THREAD_GROUP_STATUS_RUNNING) { + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } + return CELL_ESTAT; } @@ -883,11 +960,30 @@ error_code sys_spu_thread_group_terminate(ppu_thread& ppu, u32 id, s32 value) std::unique_lock lock(group->mutex); - if (group->run_state <= SPU_THREAD_GROUP_STATUS_INITIALIZED || - group->run_state == SPU_THREAD_GROUP_STATUS_WAITING || - group->run_state == SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED || - group->set_terminate) + if (auto state = +group->run_state; + state <= SPU_THREAD_GROUP_STATUS_INITIALIZED || + state == SPU_THREAD_GROUP_STATUS_WAITING || + state == SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED || + state == SPU_THREAD_GROUP_STATUS_DESTROYED) { + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } + + return CELL_ESTAT; + } + + if (group->set_terminate) + { + // Wait for termination, only then return error code + const u64 last_stop = group->stop_count; + + while (group->stop_count == last_stop) + { + group->cond.wait(lock); + } + return CELL_ESTAT; } @@ -913,7 +1009,7 @@ error_code sys_spu_thread_group_terminate(ppu_thread& ppu, u32 id, s32 value) group->join_state = SYS_SPU_THREAD_GROUP_JOIN_TERMINATED; // Wait until the threads are actually stopped - const u64 last_stop = group->stop_count - !group->running; + const u64 last_stop = group->stop_count; while (group->stop_count == last_stop) { @@ -940,7 +1036,14 @@ error_code sys_spu_thread_group_join(ppu_thread& ppu, u32 id, vm::ptr cause { std::unique_lock lock(group->mutex); - if (group->run_state < SPU_THREAD_GROUP_STATUS_INITIALIZED) + const auto state = +group->run_state; + + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } + + if (state < SPU_THREAD_GROUP_STATUS_INITIALIZED) { return CELL_ESTAT; } @@ -951,7 +1054,7 @@ error_code sys_spu_thread_group_join(ppu_thread& ppu, u32 id, vm::ptr cause return CELL_EBUSY; } - if (group->join_state && group->run_state == SPU_THREAD_GROUP_STATUS_INITIALIZED) + if (group->join_state && state == SPU_THREAD_GROUP_STATUS_INITIALIZED) { // Already signaled ppu.gpr[4] = group->join_state; @@ -1129,8 +1232,14 @@ error_code sys_spu_thread_write_ls(ppu_thread& ppu, u32 id, u32 lsa, u64 value, std::lock_guard lock(group->mutex); - if (group->run_state < SPU_THREAD_GROUP_STATUS_WAITING || group->run_state > SPU_THREAD_GROUP_STATUS_RUNNING) + if (auto state = +group->run_state; + state < SPU_THREAD_GROUP_STATUS_WAITING || state > SPU_THREAD_GROUP_STATUS_RUNNING) { + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } + return CELL_ESTAT; } @@ -1166,8 +1275,14 @@ error_code sys_spu_thread_read_ls(ppu_thread& ppu, u32 id, u32 lsa, vm::ptr std::lock_guard lock(group->mutex); - if (group->run_state < SPU_THREAD_GROUP_STATUS_WAITING || group->run_state > SPU_THREAD_GROUP_STATUS_RUNNING) + if (auto state = +group->run_state; + state < SPU_THREAD_GROUP_STATUS_WAITING || state > SPU_THREAD_GROUP_STATUS_RUNNING) { + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } + return CELL_ESTAT; } @@ -1529,8 +1644,14 @@ error_code sys_spu_thread_group_connect_event_all_threads(ppu_thread& ppu, u32 i std::lock_guard lock(group->mutex); - if (group->run_state < SPU_THREAD_GROUP_STATUS_INITIALIZED) + if (auto state = +group->run_state; + state < SPU_THREAD_GROUP_STATUS_INITIALIZED || state == SPU_THREAD_GROUP_STATUS_DESTROYED) { + if (state == SPU_THREAD_GROUP_STATUS_DESTROYED) + { + return CELL_ESRCH; + } + return CELL_ESTAT; } diff --git a/rpcs3/Emu/Cell/lv2/sys_spu.h b/rpcs3/Emu/Cell/lv2/sys_spu.h index d581948d1e..0f04d4733f 100644 --- a/rpcs3/Emu/Cell/lv2/sys_spu.h +++ b/rpcs3/Emu/Cell/lv2/sys_spu.h @@ -58,6 +58,7 @@ enum spu_group_status : u32 SPU_THREAD_GROUP_STATUS_WAITING_AND_SUSPENDED, SPU_THREAD_GROUP_STATUS_RUNNING, SPU_THREAD_GROUP_STATUS_STOPPED, + SPU_THREAD_GROUP_STATUS_DESTROYED, // Internal state SPU_THREAD_GROUP_STATUS_UNKNOWN, };