From 080f87fa0ba4f74122e3f733d0c93c1e94781dea Mon Sep 17 00:00:00 2001 From: Sam Webster <13457618+samwebster@users.noreply.github.com> Date: Wed, 8 Jan 2025 20:56:30 +0000 Subject: [PATCH] [QNN EP] Make sure everything gets cleaned up (#23275) ### Description Always make sure resources and callbacks are cleaned up ### Motivation and Context We've seen problems where the log callback isn't deregistered which can lead to crashes --------- Co-authored-by: Adrian Lizarraga --- .../qnn/builder/qnn_backend_manager.cc | 94 ++++++++++++------- .../qnn/builder/qnn_backend_manager.h | 12 +-- 2 files changed, 67 insertions(+), 39 deletions(-) diff --git a/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.cc b/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.cc index fa38fad8ee..94374ca91a 100644 --- a/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.cc +++ b/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.cc @@ -306,7 +306,7 @@ QnnLog_Level_t QnnBackendManager::MapOrtSeverityToQNNLogLevel(logging::Severity } Status QnnBackendManager::ResetQnnLogLevel(std::optional ort_log_level) { - std::lock_guard lock(logger_mutex_); + std::lock_guard lock(logger_recursive_mutex_); if (!backend_setup_completed_ || logger_ == nullptr) { return Status::OK(); } @@ -796,50 +796,78 @@ Status QnnBackendManager::LoadCachedQnnContextFromBuffer(char* buffer, uint64_t Status QnnBackendManager::SetupBackend(const logging::Logger& logger, bool load_from_cached_context, bool need_load_system_lib) { - std::lock_guard lock(logger_mutex_); + std::lock_guard lock(logger_recursive_mutex_); if (backend_setup_completed_) { LOGS(logger, VERBOSE) << "Backend setup already!"; return Status::OK(); } + Status status = Status::OK(); if (qnn_saver_path_.empty()) { - ORT_RETURN_IF_ERROR(LoadBackend()); + status = LoadBackend(); } else { - ORT_RETURN_IF_ERROR(LoadQnnSaverBackend()); + status = LoadQnnSaverBackend(); + } + if (status.IsOK()) { + LOGS(logger, VERBOSE) << "LoadBackend succeed."; } - LOGS(logger, VERBOSE) << "LoadBackend succeed."; - - if (load_from_cached_context || need_load_system_lib) { - ORT_RETURN_IF_ERROR(LoadQnnSystemLib()); + if (status.IsOK() && (load_from_cached_context || need_load_system_lib)) { + status = LoadQnnSystemLib(); } - sdk_build_version_ = GetBackendBuildId(); - LOGS(logger, VERBOSE) << "Backend build version: " - << sdk_build_version_; + if (status.IsOK()) { + sdk_build_version_ = GetBackendBuildId(); + LOGS(logger, VERBOSE) << "Backend build version: " + << sdk_build_version_; + } - ORT_RETURN_IF_ERROR(InitializeQnnLog(logger)); - LOGS(logger, VERBOSE) << "SetLogger succeed."; + if (status.IsOK()) { + status = InitializeQnnLog(logger); + } + if (status.IsOK()) { + LOGS(logger, VERBOSE) << "SetLogger succeed."; + } - ORT_RETURN_IF_ERROR(InitializeBackend()); - LOGS(logger, VERBOSE) << "InitializeBackend succeed."; + if (status.IsOK()) { + status = InitializeBackend(); + } + if (status.IsOK()) { + LOGS(logger, VERBOSE) << "InitializeBackend succeed."; + } - ORT_RETURN_IF_ERROR(CreateDevice()); - LOGS(logger, VERBOSE) << "CreateDevice succeed."; + if (status.IsOK()) { + status = CreateDevice(); + } + if (status.IsOK()) { + LOGS(logger, VERBOSE) << "CreateDevice succeed."; + } - ORT_RETURN_IF_ERROR(InitializeProfiling()); - LOGS(logger, VERBOSE) << "InitializeProfiling succeed."; + if (status.IsOK()) { + status = InitializeProfiling(); + } + if (status.IsOK()) { + LOGS(logger, VERBOSE) << "InitializeProfiling succeed."; + } if (!load_from_cached_context) { - ORT_RETURN_IF_ERROR(CreateContext()); - LOGS(logger, VERBOSE) << "CreateContext succeed."; + if (status.IsOK()) { + status = CreateContext(); + } + if (status.IsOK()) { + LOGS(logger, VERBOSE) << "CreateContext succeed."; + } } - LOGS(logger, VERBOSE) << "QNN SetupBackend succeed"; + if (status.IsOK()) { + LOGS(logger, VERBOSE) << "QNN SetupBackend succeed"; + backend_setup_completed_ = true; + } else { + LOGS_DEFAULT(WARNING) << "Failed to setup so cleaning up"; + ReleaseResources(); + } - backend_setup_completed_ = true; - - return Status::OK(); + return status; } Status QnnBackendManager::CreateHtpPowerCfgId(uint32_t device_id, uint32_t core_id, uint32_t& htp_power_config_id) { @@ -1045,7 +1073,7 @@ Status QnnBackendManager::DestroyHTPPowerConfigID(uint32_t htp_power_config_id) } Status QnnBackendManager::TerminateQnnLog() { - std::lock_guard lock(logger_mutex_); + std::lock_guard lock(logger_recursive_mutex_); if (logger_ == nullptr) { return Status::OK(); } @@ -1053,7 +1081,7 @@ Status QnnBackendManager::TerminateQnnLog() { if (nullptr != qnn_interface_.logFree && nullptr != log_handle_) { auto ret_val = qnn_interface_.logFree(log_handle_); - // Reset QNN log handle to nullptr so other threads that are waiting on logger_mutex_ know it was freed. + // Reset QNN log handle to nullptr so other threads that are waiting on logger_recursive_mutex_ know it was freed. log_handle_ = nullptr; ORT_RETURN_IF(QNN_SUCCESS != ret_val, "Unable to terminate logging in the backend."); @@ -1069,33 +1097,33 @@ void QnnBackendManager::ReleaseResources() { auto result = ReleaseContext(); if (Status::OK() != result) { - ORT_THROW("Failed to ReleaseContext."); + LOGS_DEFAULT(ERROR) << "Failed to ReleaseContext."; } result = ReleaseProfilehandle(); if (Status::OK() != result) { - ORT_THROW("Failed to ReleaseProfilehandle."); + LOGS_DEFAULT(ERROR) << "Failed to ReleaseProfilehandle."; } result = ReleaseDevice(); if (Status::OK() != result) { - ORT_THROW("Failed to ReleaseDevice."); + LOGS_DEFAULT(ERROR) << "Failed to ReleaseDevice."; } result = ShutdownBackend(); if (Status::OK() != result) { - ORT_THROW("Failed to ShutdownBackend."); + LOGS_DEFAULT(ERROR) << "Failed to ShutdownBackend."; } result = TerminateQnnLog(); if (Status::OK() != result) { - ORT_THROW("Failed to TerminateQnnLog."); + LOGS_DEFAULT(ERROR) << "Failed to TerminateQnnLog."; } if (backend_lib_handle_) { result = UnloadLib(backend_lib_handle_); if (Status::OK() != result) { - ORT_THROW("Failed to unload backend library."); + LOGS_DEFAULT(ERROR) << "Failed to unload backend library."; } } diff --git a/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.h b/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.h index beabc9bd71..8222d54395 100644 --- a/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.h +++ b/onnxruntime/core/providers/qnn/builder/qnn_backend_manager.h @@ -97,7 +97,7 @@ class QnnBackendManager { int64_t max_spill_fill_size); // Initializes handles to QNN resources (device, logger, etc.). - // NOTE: This function locks the internal `logger_mutex_`. + // NOTE: This function locks the internal `logger_recursive_mutex_`. Status SetupBackend(const logging::Logger& logger, bool load_from_cached_context, bool need_load_system_lib); Status CreateHtpPowerCfgId(uint32_t deviceId, uint32_t coreId, uint32_t& htp_power_config_id); @@ -125,7 +125,7 @@ class QnnBackendManager { // Resets the QNN log level to the given ORT log level or to the default log level if the argument is // std::nullopt. - // NOTE: This function locks the internal `logger_mutex_`. + // NOTE: This function locks the internal `logger_recursive_mutex_`. Status ResetQnnLogLevel(std::optional ort_log_level = std::nullopt); Status ExtractBackendProfilingInfo(); @@ -150,15 +150,15 @@ class QnnBackendManager { private: // Sets the ORT logger and creates a corresponding QNN logger with the same log level. - // NOTE: caller must lock the `logger_mutex_` before calling this function. + // NOTE: caller must lock the `logger_recursive_mutex_` before calling this function. Status InitializeQnnLog(const logging::Logger& logger); // Terminate logging in the backend - // NOTE: This function locks the internal `logger_mutex_`. + // NOTE: This function locks the internal `logger_recursive_mutex_`. Status TerminateQnnLog(); // Releases all QNN resources. Called in the destructor. - // NOTE: This function indirectly locks the internal `logger_mutex_` via nested function calls. + // NOTE: This function indirectly locks the internal `logger_recursive_mutex_` via nested function calls. void ReleaseResources(); void* LoadLib(const char* file_name, int flags, std::string& error_msg); @@ -232,7 +232,7 @@ class QnnBackendManager { private: const std::string backend_path_; - std::mutex logger_mutex_; + std::recursive_mutex logger_recursive_mutex_; const logging::Logger* logger_ = nullptr; QNN_INTERFACE_VER_TYPE qnn_interface_ = QNN_INTERFACE_VER_TYPE_INIT; QNN_SYSTEM_INTERFACE_VER_TYPE qnn_sys_interface_ = QNN_SYSTEM_INTERFACE_VER_TYPE_INIT;