From 278ceb7a7be02f7eeb928ff43af80f0674fc0088 Mon Sep 17 00:00:00 2001 From: Ming Zheng Date: Thu, 17 Aug 2023 10:27:31 -0400 Subject: [PATCH] Address code review comments Move ScopedDestroyLock class in its own dedicated source file. --- android/framework/encode/CMakeLists.txt | 2 + framework/encode/CMakeLists.txt | 2 + .../encode/vulkan_handle_wrapper_util.cpp | 3 - framework/encode/vulkan_handle_wrapper_util.h | 75 +--------------- .../encode/vulkan_scoped_destroy_lock.cpp | 56 ++++++++++++ framework/encode/vulkan_scoped_destroy_lock.h | 85 +++++++++++++++++++ 6 files changed, 146 insertions(+), 77 deletions(-) create mode 100644 framework/encode/vulkan_scoped_destroy_lock.cpp create mode 100644 framework/encode/vulkan_scoped_destroy_lock.h diff --git a/android/framework/encode/CMakeLists.txt b/android/framework/encode/CMakeLists.txt index 3d8c1a927..1db12cd3b 100644 --- a/android/framework/encode/CMakeLists.txt +++ b/android/framework/encode/CMakeLists.txt @@ -23,6 +23,8 @@ target_sources(gfxrecon_encode ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_capture_manager.h ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_capture_manager.cpp ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_handle_wrappers.h + ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_scoped_destroy_lock.h + ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_scoped_destroy_lock.cpp ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_handle_wrapper_util.h ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_handle_wrapper_util.cpp ${GFXRECON_SOURCE_DIR}/framework/encode/vulkan_state_info.h diff --git a/framework/encode/CMakeLists.txt b/framework/encode/CMakeLists.txt index fcb9a1ddf..d83666a59 100644 --- a/framework/encode/CMakeLists.txt +++ b/framework/encode/CMakeLists.txt @@ -90,6 +90,8 @@ target_sources(gfxrecon_encode ${CMAKE_CURRENT_LIST_DIR}/vulkan_capture_manager.h ${CMAKE_CURRENT_LIST_DIR}/vulkan_capture_manager.cpp ${CMAKE_CURRENT_LIST_DIR}/vulkan_handle_wrappers.h + ${CMAKE_CURRENT_LIST_DIR}/vulkan_scoped_destroy_lock.h + ${CMAKE_CURRENT_LIST_DIR}/vulkan_scoped_destroy_lock.cpp ${CMAKE_CURRENT_LIST_DIR}/vulkan_handle_wrapper_util.h ${CMAKE_CURRENT_LIST_DIR}/vulkan_handle_wrapper_util.cpp ${CMAKE_CURRENT_LIST_DIR}/vulkan_state_info.h diff --git a/framework/encode/vulkan_handle_wrapper_util.cpp b/framework/encode/vulkan_handle_wrapper_util.cpp index 9956d7e55..2de20e35a 100644 --- a/framework/encode/vulkan_handle_wrapper_util.cpp +++ b/framework/encode/vulkan_handle_wrapper_util.cpp @@ -1,6 +1,5 @@ /* ** Copyright (c) 2020 LunarG, Inc. -** Copyright (c) 2023 Advanced Micro Devices, Inc. All rights reserved. ** ** Permission is hereby granted, free of charge, to any person obtaining a ** copy of this software and associated documentation files (the "Software"), @@ -28,8 +27,6 @@ GFXRECON_BEGIN_NAMESPACE(encode) VulkanStateHandleTable state_handle_table_; -std::shared_mutex ScopedDestroyLock::mutex_for_create_destroy_handle_; - uint64_t GetWrappedId(uint64_t object, VkObjectType object_type) { switch (object_type) diff --git a/framework/encode/vulkan_handle_wrapper_util.h b/framework/encode/vulkan_handle_wrapper_util.h index 30ade853d..1d66daf78 100644 --- a/framework/encode/vulkan_handle_wrapper_util.h +++ b/framework/encode/vulkan_handle_wrapper_util.h @@ -26,6 +26,7 @@ #include "encode/handle_unwrap_memory.h" #include "encode/vulkan_handle_wrappers.h" +#include "vulkan_scoped_destroy_lock.h" #include "format/format.h" #include "format/format_util.h" #include "generated/generated_vulkan_dispatch_table.h" @@ -56,80 +57,6 @@ typedef format::HandleId (*PFN_GetHandleId)(); extern VulkanStateHandleTable state_handle_table_; -/* -Regarding mutex_for_create_destroy_handle_ and related lock/unlock functions. These are used to address the following -race condition during capture: - -Sometimes an app will destroy some Vulkan handles in one thread (A) and create same type of Vulkan handle in another -thread (B). There is a gap of time in between when the real handle is destroyed, and when its wrappers were deleted from -map in thread A. If during this time period, thread B was able to run, and creates same type of handles, and if any of -the newly-created handles had the same value of those destroyed by thread A, we crash. - -For example, lets say an app's thread A calls vkFreeCommandBuffers, and in thread B it calls vkAllocateCommandBuffers. - -GFXR's default API lock is AcquireSharedApiCallLock(), but if every API handling only request shared lock, that means -there is actually no lock for them. Therefore execution could switch from one thread to another thread. If thread A -calls vkFreeCommandBuffers to free command buffer group GC-X, those real Vulkan handles get destroyed by the driver, and -GFXR will proceed to delete wrapper objects of GC-X from the corresponding map. During this time, thread B was able to -run, and calls vkAllocateCommandBuffers to create a group of command buffers GC-Y. But because GC-X was already -destroyed, the driver may return some of the same former handle values of GC-X, but their wrapper still exists, and -GFXR's insertion of the new wrapper into its map will fail. And thread B will delete the wrapper later, so for any -following process, there would be no wrapper for the real handle which will eventually provoke a crash. - -Note: destruction of other things could also potentially have this problem. For example, replace the above -vkFreeCommandBuffers with vkDestroyCommandPool. This call will free all command buffers of the command pool. - -Regarding mutex_for_create_destroy_handle_ : - -For any create wrapper operation, the operation which delete real Vulkan handle and its wrapper in map must be atomic. -This means a real handle and its wrapper must both exist, or both not exist, for any create wrapper operation. In the -following code, shared locks were already added to create wrapper functions. - -The functions LockForDestroyHandle and UnlockForDestroyHandle should be used during capture. This will add exclusive -lock to the deletion of handles and their wrapper. -*/ - -class ScopedDestroyLock -{ - public: - ScopedDestroyLock(bool shared = false) - { - lock_shared_ = shared; - if (shared) - { - mutex_for_create_destroy_handle_.lock_shared(); - } - else - { - mutex_for_create_destroy_handle_.lock(); - } - }; - - ~ScopedDestroyLock() - { - if (lock_shared_) - { - mutex_for_create_destroy_handle_.unlock_shared(); - } - else - { - mutex_for_create_destroy_handle_.unlock(); - } - }; - - ScopedDestroyLock(const ScopedDestroyLock&) = delete; - - ScopedDestroyLock(ScopedDestroyLock&&) = delete; - - ScopedDestroyLock& operator=(const ScopedDestroyLock&) = delete; - - ScopedDestroyLock& operator=(ScopedDestroyLock&&) = delete; - - private: - bool lock_shared_ = false; - static std::shared_mutex mutex_for_create_destroy_handle_; -}; - template format::HandleId GetTempWrapperId(const typename Wrapper::HandleType& handle) { diff --git a/framework/encode/vulkan_scoped_destroy_lock.cpp b/framework/encode/vulkan_scoped_destroy_lock.cpp new file mode 100644 index 000000000..7a38b832b --- /dev/null +++ b/framework/encode/vulkan_scoped_destroy_lock.cpp @@ -0,0 +1,56 @@ +/* +** Copyright (c) 2023 Advanced Micro Devices, Inc. All rights reserved. +** +** Permission is hereby granted, free of charge, to any person obtaining a +** copy of this software and associated documentation files (the "Software"), +** to deal in the Software without restriction, including without limitation +** the rights to use, copy, modify, merge, publish, distribute, sublicense, +** and/or sell copies of the Software, and to permit persons to whom the +** Software is furnished to do so, subject to the following conditions: +** +** The above copyright notice and this permission notice shall be included in +** all copies or substantial portions of the Software. +** +** THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +** IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +** FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +** AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +** LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +** FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +** DEALINGS IN THE SOFTWARE. +*/ + +#include "vulkan_scoped_destroy_lock.h" + +GFXRECON_BEGIN_NAMESPACE(gfxrecon) +GFXRECON_BEGIN_NAMESPACE(encode) + +std::shared_mutex ScopedDestroyLock::mutex_for_create_destroy_handle_; + +ScopedDestroyLock::ScopedDestroyLock(bool shared) +{ + lock_shared_ = shared; + if (shared) + { + mutex_for_create_destroy_handle_.lock_shared(); + } + else + { + mutex_for_create_destroy_handle_.lock(); + } +}; + +ScopedDestroyLock::~ScopedDestroyLock() +{ + if (lock_shared_) + { + mutex_for_create_destroy_handle_.unlock_shared(); + } + else + { + mutex_for_create_destroy_handle_.unlock(); + } +}; + +GFXRECON_END_NAMESPACE(encode) +GFXRECON_END_NAMESPACE(gfxrecon) diff --git a/framework/encode/vulkan_scoped_destroy_lock.h b/framework/encode/vulkan_scoped_destroy_lock.h new file mode 100644 index 000000000..e6c4fe644 --- /dev/null +++ b/framework/encode/vulkan_scoped_destroy_lock.h @@ -0,0 +1,85 @@ +/* +** Copyright (c) 2023 Advanced Micro Devices, Inc. All rights reserved. +** +** Permission is hereby granted, free of charge, to any person obtaining a +** copy of this software and associated documentation files (the "Software"), +** to deal in the Software without restriction, including without limitation +** the rights to use, copy, modify, merge, publish, distribute, sublicense, +** and/or sell copies of the Software, and to permit persons to whom the +** Software is furnished to do so, subject to the following conditions: +** +** The above copyright notice and this permission notice shall be included in +** all copies or substantial portions of the Software. +** +** THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +** IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +** FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +** AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +** LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING +** FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER +** DEALINGS IN THE SOFTWARE. +*/ + +#ifndef GFXRECON_ENCODE_VULKAN_SCOPED_DESTROY_LOCK_H +#define GFXRECON_ENCODE_VULKAN_SCOPED_DESTROY_LOCK_H + +#include "util/defines.h" + +#include + +GFXRECON_BEGIN_NAMESPACE(gfxrecon) +GFXRECON_BEGIN_NAMESPACE(encode) + +/* +Regarding ScopedDestroyLock class and its related methods. These are used to address the +following race condition during capture: + +Sometimes an app will destroy some Vulkan handles in one thread (A) and create same type of Vulkan handle in another +thread (B). There is a gap of time in between when the real handle is destroyed, and when its wrappers were deleted from +map in thread A. If during this time period, thread B was able to run, and creates same type of handles, and if any of +the newly-created handles had the same value of those destroyed by thread A, we crash. + +For example, lets say an app's thread A calls vkFreeCommandBuffers, and in thread B it calls vkAllocateCommandBuffers. + +GFXR's default API lock is AcquireSharedApiCallLock(), but if every API handling only request shared lock, that means +there is actually no lock for them. Therefore execution could switch from one thread to another thread. If thread A +calls vkFreeCommandBuffers to free command buffer group GC-X, those real Vulkan handles get destroyed by the driver, and +GFXR will proceed to delete wrapper objects of GC-X from the corresponding map. During this time, thread B was able to +run, and calls vkAllocateCommandBuffers to create a group of command buffers GC-Y. But because GC-X was already +destroyed, the driver may return some of the same former handle values of GC-X, but their wrapper still exists, and +GFXR's insertion of the new wrapper into its map will fail. And thread B will delete the wrapper later, so for any +following process, there would be no wrapper for the real handle which will eventually provoke a crash. + +Note: destruction of other things could also potentially have this problem. For example, replace the above +vkFreeCommandBuffers with vkDestroyCommandPool. This call will free all command buffers of the command pool. + +Regarding mutex_for_create_destroy_handle_ : + +For any create wrapper operation, the operation which delete real Vulkan handle and its wrapper in map must be atomic. +This means a real handle and its wrapper must both exist, or both not exist, for any create wrapper operation. +*/ + +class ScopedDestroyLock +{ + public: + ScopedDestroyLock(bool shared = false); + + ~ScopedDestroyLock(); + + ScopedDestroyLock(const ScopedDestroyLock&) = delete; + + ScopedDestroyLock(ScopedDestroyLock&&) = delete; + + ScopedDestroyLock& operator=(const ScopedDestroyLock&) = delete; + + ScopedDestroyLock& operator=(ScopedDestroyLock&&) = delete; + + private: + bool lock_shared_ = false; + static std::shared_mutex mutex_for_create_destroy_handle_; +}; + +GFXRECON_END_NAMESPACE(encode) +GFXRECON_END_NAMESPACE(gfxrecon) + +#endif // GFXRECON_ENCODE_VULKAN_SCOPED_DESTROY_LOCK_H