From 0865293d274ca42049143d9a3192e1bba6931a47 Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Fri, 12 Jul 2024 08:41:25 -0500 Subject: [PATCH 01/12] [SWDEV-472641] Set file opening and writing encoding to utf-8 Signed-off-by: Maisam Arif Change-Id: Ic5cdca41b588341f7894de72dfe3d949378f9a61 [ROCm/amdsmi commit: c83bcb7ac9f66f1d017c46fdc18386c49b57957f] --- projects/amdsmi/amdsmi_cli/amdsmi_cli.py | 2 +- projects/amdsmi/amdsmi_cli/amdsmi_commands.py | 2 +- projects/amdsmi/amdsmi_cli/amdsmi_logger.py | 20 +++++++++---------- 3 files changed, 12 insertions(+), 12 deletions(-) diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_cli.py b/projects/amdsmi/amdsmi_cli/amdsmi_cli.py index 8311b86cb5..a5eeded872 100755 --- a/projects/amdsmi/amdsmi_cli/amdsmi_cli.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_cli.py @@ -42,7 +42,7 @@ def _print_error(e, destination): if destination in ['stdout', 'json', 'csv']: print(e) else: - f = open(destination, "w") + f = open(destination, "w", encoding="utf-8") f.write(e) f.close() print("Error occured. Result written to " + str(destination) + " file") diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_commands.py b/projects/amdsmi/amdsmi_cli/amdsmi_commands.py index 16823c4b20..4c65ee0ee4 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_commands.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_commands.py @@ -122,7 +122,7 @@ class AMDSMICommands(): if self.logger.destination == 'stdout': print(human_readable_output) else: - with self.logger.destination.open('a') as output_file: + with self.logger.destination.open('a', encoding="utf-8") as output_file: output_file.write(human_readable_output + '\n') elif self.logger.is_json_format() or self.logger.is_csv_format(): self.logger.print_output() diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_logger.py b/projects/amdsmi/amdsmi_cli/amdsmi_logger.py index b54d669897..12fdd0faf8 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_logger.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_logger.py @@ -480,10 +480,10 @@ class AMDSMILogger(): print(json_std_output) else: # Write output to file if watching_output: # Flush the full JSON output to the file on watch command completion - with self.destination.open('w') as output_file: + with self.destination.open('w', encoding="utf-8") as output_file: json.dump(self.watch_output, output_file, indent=4) else: - with self.destination.open('a') as output_file: + with self.destination.open('a', encoding="utf-8") as output_file: json.dump(json_output, output_file, indent=4) @@ -516,7 +516,7 @@ class AMDSMILogger(): print(str(csv_stdout_output)) else: if watching_output: - with self.destination.open('w', newline = '') as output_file: + with self.destination.open('w', newline = '', encoding="utf-8") as output_file: if self.watch_output: csv_keys = set() for output in self.watch_output: @@ -534,7 +534,7 @@ class AMDSMILogger(): writer.writeheader() writer.writerows(self.watch_output) else: - with self.destination.open('a', newline = '') as output_file: + with self.destination.open('a', newline = '', encoding="utf-8") as output_file: # Get the header as a list of the first element to maintain order csv_header = stored_csv_output[0].keys() writer = csv.DictWriter(output_file, csv_header) @@ -622,7 +622,7 @@ class AMDSMILogger(): print() else: if watching_output: - with self.destination.open('w', newline = '') as output_file: + with self.destination.open('w', newline = '', encoding="utf-8") as output_file: primary_csv_output = [] secondary_csv_output = [] if self.watch_output: @@ -687,7 +687,7 @@ class AMDSMILogger(): writer.writeheader() writer.writerows(secondary_csv_output) else: - with self.destination.open('a', newline = '') as output_file: + with self.destination.open('a', newline = '', encoding="utf-8") as output_file: if primary_csv_output: # Get the header as a list of the first element to maintain order csv_header = primary_csv_output[0].keys() @@ -724,13 +724,13 @@ class AMDSMILogger(): print(human_readable_output.encode('ascii', 'ignore').decode('ascii')) else: if watching_output: - with self.destination.open('w') as output_file: + with self.destination.open('w', encoding="utf-8") as output_file: human_readable_output = '' for output in self.watch_output: human_readable_output += self._convert_json_to_human_readable(output) output_file.write(human_readable_output + '\n') else: - with self.destination.open('a') as output_file: + with self.destination.open('a', encoding="utf-8") as output_file: output_file.write(human_readable_output + '\n') @@ -806,7 +806,7 @@ class AMDSMILogger(): print("\n") else: if watching_output: # Write all stored watched output to a file - with self.destination.open('w') as output_file: + with self.destination.open('w', encoding="utf-8") as output_file: primary_table = '' secondary_table = '' # Add process_list to the secondary_table @@ -851,6 +851,6 @@ class AMDSMILogger(): if secondary_table: output_file.write("\n" + secondary_table) else: # Write all singular output to a file - with self.destination.open('a') as output_file: + with self.destination.open('a', encoding="utf-8") as output_file: output_file.write(primary_table + '\n') output_file.write(secondary_table) From a7ea5365242e43a71ce5a1fbbace04ef96068c28 Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Fri, 12 Jul 2024 09:11:56 -0500 Subject: [PATCH 02/12] [SWDEV-439701] Add missing parameter CLI error cases Signed-off-by: Maisam Arif Change-Id: I8a6623ccdd7b16be320ea0f6f48b7b6cb3511983 [ROCm/amdsmi commit: 0fc779b14f7990a868e6c08b01c0c995e125e8fc] --- projects/amdsmi/amdsmi_cli/amdsmi_parser.py | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_parser.py b/projects/amdsmi/amdsmi_cli/amdsmi_parser.py index 4d2984d620..4b9c55124b 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_parser.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_parser.py @@ -140,7 +140,10 @@ class AMDSMIParser(argparse.ArgumentParser): return int(int_value) outputformat = self.helpers.get_output_format() - raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException(int_value, outputformat) + if int_value == "": + raise amdsmi_cli_exceptions.AmdSmiMissingParameterValueException(int_value, outputformat) + else: + raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException(int_value, outputformat) def _positive_int(self, int_value): @@ -150,7 +153,10 @@ class AMDSMIParser(argparse.ArgumentParser): return int(int_value) outputformat = self.helpers.get_output_format() - raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException(int_value, outputformat) + if int_value == "": + raise amdsmi_cli_exceptions.AmdSmiMissingParameterValueException(int_value, outputformat) + else: + raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException(int_value, outputformat) def _is_valid_string(self, string_value): @@ -160,8 +166,10 @@ class AMDSMIParser(argparse.ArgumentParser): return string_value outputformat = self.helpers.get_output_format() - raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException(string_value, outputformat) - + if string_value == "": + raise amdsmi_cli_exceptions.AmdSmiMissingParameterValueException(string_value, outputformat) + else: + raise amdsmi_cli_exceptions.AmdSmiInvalidParameterValueException(string_value, outputformat) def _check_output_file_path(self): """ Argument action validator: From ad3ba88ba527ea6648e4ac3d1aaecee69c2c1ec6 Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Wed, 17 Jul 2024 17:10:37 -0500 Subject: [PATCH 03/12] [SWDEV-474474] - Changed Monitor PCIE_REPLAY count to use gpu_metrics Signed-off-by: Maisam Arif Change-Id: I4351a23e8412875bb4b23b30747ac6d0bf3d3c56 [ROCm/amdsmi commit: 5b7be3bf99e89bc99572a9be8d098a6bbcfc6007] --- projects/amdsmi/amdsmi_cli/amdsmi_commands.py | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_commands.py b/projects/amdsmi/amdsmi_cli/amdsmi_commands.py index 4c65ee0ee4..d376401ac9 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_commands.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_commands.py @@ -4300,11 +4300,19 @@ class AMDSMICommands(): self.logger.table_header += 'DOUBLE_ECC'.rjust(12) try: - pcie_replay = amdsmi_interface.amdsmi_get_gpu_pci_replay_counter(args.gpu) - monitor_values['pcie_replay'] = pcie_replay + pcie_metric = amdsmi_interface.amdsmi_get_pcie_info(args.gpu)['pcie_metric'] + logging.debug("PCIE Metric for %s | %s", gpu_id, pcie_metric) + monitor_values['pcie_replay'] = pcie_metric['pcie_replay_count'] except amdsmi_exception.AmdSmiLibraryException as e: monitor_values['pcie_replay'] = "N/A" - logging.debug("Failed to get pcie replay counter on gpu %s | %s", gpu_id, e.get_error_info()) + logging.debug("Failed to get gpu_metrics pcie replay counter on gpu %s | %s", gpu_id, e.get_error_info()) + + if monitor_values['pcie_replay'] == "N/A": + try: + pcie_replay = amdsmi_interface.amdsmi_get_gpu_pci_replay_counter(args.gpu) + monitor_values['pcie_replay'] = pcie_replay + except amdsmi_exception.AmdSmiLibraryException as e: + logging.debug("Failed to get sysfs pcie replay counter on gpu %s | %s", gpu_id, e.get_error_info()) self.logger.table_header += 'PCIE_REPLAY'.rjust(13) if args.vram_usage: From 244587525a07160aa4e0e43d5de2df5d43cce130 Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Thu, 18 Jul 2024 13:10:56 -0500 Subject: [PATCH 04/12] Corrected passthrough platform logic Signed-off-by: Maisam Arif Change-Id: Ibbc4f5dd6300efd16f26efe17bc68eb8c22ed8be [ROCm/amdsmi commit: b5f9e6a91dd86fdbcbc58b7192d385e31e24aeab] --- projects/amdsmi/amdsmi_cli/amdsmi_helpers.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py index 93beabcb9c..952f9c57db 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py @@ -68,9 +68,10 @@ class AMDSMIHelpers(): output = run(["lspci", "-nn"], stdout=PIPE, stderr=STDOUT, encoding="UTF-8").stdout passthrough_device_ids = ["7460", "73c8", "74a0", "74a1", "74a2"] if any(device_id in output for device_id in passthrough_device_ids): - self._is_baremetal = True - self._is_virtual_os = False - self._is_passthrough = True + if self._is_virtual_os: + self._is_baremetal = True + self._is_virtual_os = False + self._is_passthrough = True def os_info(self, string_format=True): From df6b8386cf2e05ddc33a30c7e1ca313a678c9f2e Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Thu, 18 Jul 2024 13:12:03 -0500 Subject: [PATCH 05/12] Fix amd-smi event from reading NONE event type Signed-off-by: Maisam Arif Change-Id: I7acf91eb682b3f0873ca34a98191eafd3925a344 [ROCm/amdsmi commit: 8f15c2260621547b09e722cc30bb5be06206d943] --- projects/amdsmi/py-interface/amdsmi_interface.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/projects/amdsmi/py-interface/amdsmi_interface.py b/projects/amdsmi/py-interface/amdsmi_interface.py index 391a742238..1b3cb3ec6d 100644 --- a/projects/amdsmi/py-interface/amdsmi_interface.py +++ b/projects/amdsmi/py-interface/amdsmi_interface.py @@ -442,13 +442,14 @@ class AmdSmiEventReader: for i in range(0, num_elem): unique_event_values = set(event.value for event in AmdSmiEvtNotificationType) if self.event_info[i].event in unique_event_values: - ret.append( - { - "processor_handle": self.event_info[i].processor_handle, - "event": AmdSmiEvtNotificationType(self.event_info[i].event).name, - "message": self.event_info[i].message.decode("utf-8"), - } - ) + if AmdSmiEvtNotificationType(self.event_info[i].event).name != "NONE": + ret.append( + { + "processor_handle": self.event_info[i].processor_handle, + "event": AmdSmiEvtNotificationType(self.event_info[i].event).name, + "message": self.event_info[i].message.decode("utf-8"), + } + ) return ret From 3fc94657d9db09419f1c2db10103d6276ee83a8f Mon Sep 17 00:00:00 2001 From: "Bill(Shuzhou) Liu" Date: Wed, 17 Jul 2024 08:58:55 -0500 Subject: [PATCH 06/12] Remove const to avoid compile error Fix the compile error Change-Id: I422b606b2b969b418c2e77b47a3afad0cfc732a1 [ROCm/amdsmi commit: 33dab0c232caa90b9436266ad29e9e0d9b79ee07] --- projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_utils.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_utils.h b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_utils.h index 75ba165a45..eb53dfbba3 100755 --- a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_utils.h +++ b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_utils.h @@ -242,7 +242,7 @@ class ScopeGuard { __forceinline ~ScopeGuard() { if (!dismiss_) release_(); } - __forceinline ScopeGuard& operator=(const ScopeGuard& rhs) { + __forceinline ScopeGuard& operator=(ScopeGuard& rhs) { dismiss_ = rhs.dismiss_; release_ = rhs.release_; rhs.dismiss_ = true; From eb04dbc1c922e7e6500259eb32ada3b0919797fc Mon Sep 17 00:00:00 2001 From: "Bill(Shuzhou) Liu" Date: Thu, 21 Mar 2024 15:25:35 -0500 Subject: [PATCH 07/12] Unlock the mutex when process is dead After the dead process is detected, pthread_mutex_consistent() will be called. After that, the pthread_mutex_unlock() should also be called to unlock it: "It is the responsibility of the application to recover the state so it can be reused." Change-Id: I45d3e2e68c3b06779f3acb1e908dbec0c6a39297 [ROCm/amdsmi commit: 06b9232a562bfdca467822a6d37e81996b5bf60c] --- .../third_party/shared_mutex/shared_mutex.cc | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc index cc61042fa3..ed66124171 100755 --- a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc +++ b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc @@ -48,10 +48,13 @@ static std::vector lsof(const char* filename) { DIR *dp = nullptr; std::vector process_id; + pid_t cur_pid = getpid(); dp = opendir("/proc"); if (dp != nullptr) { while ((entry = readdir(dp))) { std::string id(entry->d_name); + // ignore current process + if (id == std::to_string(cur_pid)) continue; // the process id should be a number if (std::all_of(id.begin(), id.end(), ::isdigit)) { process_id.push_back(entry->d_name); @@ -132,6 +135,7 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { int ret; ret = pthread_mutex_timedlock(mutex_ptr, &expireTime); + pid_t cur_pid = getpid(); if (ret == EOWNERDEAD) { ret = pthread_mutex_consistent(mutex_ptr); @@ -149,6 +153,12 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { throw amd::smi::rsmi_exception(RSMI_STATUS_BUSY, __FUNCTION__); return mutex; } + + fprintf(stderr, "%s: %d detected dead process, and make mutex consistent.\n", name, cur_pid); + // The mutex is locked even if EOWNERDEAD was returned,and need to unlock it. + if (pthread_mutex_unlock(mutex_ptr)) { + perror("pthread_mutex_unlock"); + } } else if (ret || (mutex.created == 0 && reinterpret_cast(addr)->ptr == NULL)) { // Something is out of sync. @@ -158,6 +168,7 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { if (!retried) { std::vector ids = lsof(name); if (ids.size() == 0) { // no process is using it + fprintf(stderr, "%s: %d re-init the mutex since no one use it.\n", name, cur_pid); memset(mutex_ptr, 0, sizeof(pthread_mutex_t)); // Set mutex.created == 1 so that it can be initialized latter. mutex.created = 1; @@ -181,7 +192,8 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { } } - if (mutex.created) { + // also need to set the attribute when retried as the mutex is re-initialized. + if (mutex.created || retried) { pthread_mutexattr_t attr; if (pthread_mutexattr_init(&attr)) { perror("pthread_mutexattr_init"); From a1aec9c971b8da5f731894604a8ba75fbdb93155 Mon Sep 17 00:00:00 2001 From: "Bill(Shuzhou) Liu" Date: Fri, 19 Apr 2024 13:16:29 -0500 Subject: [PATCH 08/12] Support thread only mutex The environment variable RSMI_MUTEX_THREAD_ONLY=1 to enable thread only mutex. The RSMI_INIT_FLAG_THRAD_ONLY_MUTEX can also be pass to rsmi_init() to enable thread only mutex. Change-Id: I2d9844039b774e386f03bb9bb130d8c342504ea6 [ROCm/amdsmi commit: dbba33d3f5485fe192a9de51759e559e55fdafc1] --- .../rocm_smi/include/rocm_smi/rocm_smi.h | 1 + .../rocm_smi/include/rocm_smi/rocm_smi_main.h | 3 + .../third_party/shared_mutex/shared_mutex.cc | 217 ++++++++++++------ 3 files changed, 152 insertions(+), 69 deletions(-) diff --git a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h index de0b65a806..acbd678232 100755 --- a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h +++ b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi.h @@ -149,6 +149,7 @@ typedef enum { //!< information can be retrieved. By //!< default, only AMD devices are //!< enumerated by RSMI. + RSMI_INIT_FLAG_THRAD_ONLY_MUTEX = 0x400000000000000, //!< The mutex limit to thread RSMI_INIT_FLAG_RESRV_TEST1 = 0x800000000000000, //!< Reserved for test } rsmi_init_flags_t; diff --git a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_main.h b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_main.h index c957f5123c..0a66ea227c 100755 --- a/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_main.h +++ b/projects/amdsmi/rocm_smi/include/rocm_smi/rocm_smi_main.h @@ -88,6 +88,9 @@ class RocmSMI { void set_init_options(uint64_t options) {init_options_ = options;} uint64_t init_options() const {return init_options_;} + uint64_t is_thread_only_mutex() const { + return init_options_ & RSMI_INIT_FLAG_THRAD_ONLY_MUTEX; + } uint32_t euid() const {return euid_;} diff --git a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc index ed66124171..d9aa1a20b5 100755 --- a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc +++ b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc @@ -41,6 +41,22 @@ THE SOFTWARE. #include #include "rocm_smi/rocm_smi_exception.h" +#include "rocm_smi/rocm_smi_main.h" + +#define THREAD_ONLY_ENV_VAR "RSMI_MUTEX_THREAD_ONLY" +#define MUTEX_TIME_OUT_ENV_VAR "RSMI_MUTEX_TIMEOUT" +#define DEFAULT_MUTEX_TIMEOUT_SECONDS 5 + +static int GetEnvVarUInteger(const char *ev_str) { + ev_str = getenv(ev_str); + + if (ev_str) { + const int ret = atoi(ev_str); + return ret; + } + + return -1; +} // find which processes are using the file by searching /proc/*/fd static std::vector lsof(const char* filename) { @@ -81,9 +97,53 @@ static std::vector lsof(const char* filename) { return matched_process; } -shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { - shared_mutex_t mutex = {NULL, 0, NULL, 0}; +// RSMI_MUTEX_THREAD_ONLY = 1 to enable thread safe mutex +shared_mutex_t init_thread_safe_only(const char *name) { + shared_mutex_t mutex = {nullptr, 0, nullptr, 0}; errno = 0; + mutex.shm_fd = -1; + mutex.created = 0; + pthread_mutex_t *mutex_ptr = new pthread_mutex_t(); + + pthread_mutexattr_t attr; + if (pthread_mutexattr_init(&attr)) { + perror("pthread_mutexattr_init"); + return mutex; + } + if (pthread_mutexattr_setpshared(&attr, PTHREAD_PROCESS_SHARED)) { + perror("pthread_mutexattr_setpshared"); + return mutex; + } + + if (pthread_mutexattr_settype(&attr, PTHREAD_MUTEX_RECURSIVE)) { + perror("pthread_mutexattr_settype"); + return mutex; + } + if (pthread_mutexattr_setrobust(&attr, PTHREAD_MUTEX_ROBUST)) { + perror("pthread_mutexattr_setrobust"); + return mutex; + } + if (pthread_mutex_init(mutex_ptr, &attr)) { + perror("pthread_mutex_init"); + return mutex; + } + + mutex.ptr = mutex_ptr; + mutex.name = reinterpret_cast(malloc(NAME_MAX+1)); + (void)snprintf(mutex.name, NAME_MAX + 1, "%s", name); + return mutex; +} + +shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { + shared_mutex_t mutex = {nullptr, 0, nullptr, 0}; + errno = 0; + + amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance(); + + if (GetEnvVarUInteger(THREAD_ONLY_ENV_VAR) == 1 || smi.is_thread_only_mutex()) { + fprintf(stderr, "rocm-smi: using thread safe only mutex\n"); + return init_thread_safe_only(name); + } // Open existing shared memory object, or create one. // Two separate calls are needed here, to mark fact of creation @@ -112,7 +172,7 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { // Map pthread mutex into the shared memory. void *addr = mmap( - NULL, + nullptr, sizeof(pthread_mutex_t), PROT_READ|PROT_WRITE, MAP_SHARED, @@ -130,67 +190,12 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { // acquire it in 5 sec., re-do everything. struct timespec expireTime; clock_gettime(CLOCK_REALTIME, &expireTime); - expireTime.tv_sec += 5; + int time_out = GetEnvVarUInteger(MUTEX_TIME_OUT_ENV_VAR); + time_out = time_out < DEFAULT_MUTEX_TIMEOUT_SECONDS ? DEFAULT_MUTEX_TIMEOUT_SECONDS: time_out; + expireTime.tv_sec += time_out; - int ret; - - ret = pthread_mutex_timedlock(mutex_ptr, &expireTime); pid_t cur_pid = getpid(); - - if (ret == EOWNERDEAD) { - ret = pthread_mutex_consistent(mutex_ptr); - // This function should not fail unless mutex_ptr is not robust - // or mutex_ptr is not in an inconsistent state. Neither scenario - // should ever be true at this point in the code. - assert(!ret); - - // ...but if there are undocumented failure cases for - // pthread_mutex_consistent() handle them for release builds. - if (ret) { - fprintf(stderr, "pthread_mutex_consistent() returned %d\n", ret); - free(mutex.name); - - throw amd::smi::rsmi_exception(RSMI_STATUS_BUSY, __FUNCTION__); - return mutex; - } - - fprintf(stderr, "%s: %d detected dead process, and make mutex consistent.\n", name, cur_pid); - // The mutex is locked even if EOWNERDEAD was returned,and need to unlock it. - if (pthread_mutex_unlock(mutex_ptr)) { - perror("pthread_mutex_unlock"); - } - } else if (ret || (mutex.created == 0 && - reinterpret_cast(addr)->ptr == NULL)) { - // Something is out of sync. - - // When process crash before unlock the mutex, the mutex is in bad status. - // reset the mutex if no process is using it, and then retry lock - if (!retried) { - std::vector ids = lsof(name); - if (ids.size() == 0) { // no process is using it - fprintf(stderr, "%s: %d re-init the mutex since no one use it.\n", name, cur_pid); - memset(mutex_ptr, 0, sizeof(pthread_mutex_t)); - // Set mutex.created == 1 so that it can be initialized latter. - mutex.created = 1; - free(mutex.name); - return shared_mutex_init(name, mode, true); - } - } - - fprintf(stderr, "pthread_mutex_timedlock() returned %d\n", ret); - perror("Failed to initialize RSMI device mutex after 5 seconds. Previous " - "execution may not have shutdown cleanly. To fix problem, stop all " - "rocm_smi programs, and then delete the rocm_smi* shared memory files in" - " /dev/shm."); - free(mutex.name); - - throw amd::smi::rsmi_exception(RSMI_STATUS_BUSY, __FUNCTION__); - return mutex; - } else { - if (pthread_mutex_unlock(mutex_ptr)) { - perror("pthread_mutex_unlock"); - } - } + int ret; // also need to set the attribute when retried as the mutex is re-initialized. if (mutex.created || retried) { @@ -218,6 +223,70 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { } } + ret = pthread_mutex_timedlock(mutex_ptr, &expireTime); + + + if (ret == EOWNERDEAD) { + ret = pthread_mutex_consistent(mutex_ptr); + // This function should not fail unless mutex_ptr is not robust + // or mutex_ptr is not in an inconsistent state. Neither scenario + // should ever be true at this point in the code. + assert(!ret); + + // ...but if there are undocumented failure cases for + // pthread_mutex_consistent() handle them for release builds. + if (ret) { + fprintf(stderr, "pthread_mutex_consistent() returned %d\n", ret); + free(mutex.name); + + throw amd::smi::rsmi_exception(RSMI_STATUS_BUSY, __FUNCTION__); + return mutex; + } + + fprintf(stderr, "%d detected dead process, and making mutex %s consistent.\n", + cur_pid, name); + // The mutex is locked even if EOWNERDEAD was returned,and need to unlock it. + if (pthread_mutex_unlock(mutex_ptr)) { + perror("pthread_mutex_unlock"); + } + } else if (ret || (mutex.created == 0 && + reinterpret_cast(addr)->ptr == nullptr)) { + // Something is out of sync. + + // When process crash before unlock the mutex, the mutex is in bad status. + // reset the mutex if no process is using it, and then retry lock + if (!retried) { + std::vector ids = lsof(name); + if (ids.size() == 0) { // no process is using it + fprintf(stderr, "%d re-init the mutex %s since no one use it. ret:%d ptr:%p\n", + cur_pid, name, ret, reinterpret_cast(addr)->ptr); + memset(mutex_ptr, 0, sizeof(pthread_mutex_t)); + // Set mutex.created == 1 so that it can be initialized latter. + mutex.created = 1; + free(mutex.name); + return shared_mutex_init(name, mode, true); + } + } + + fprintf(stderr, "pthread_mutex_timedlock() returned %d\n", ret); + perror("Failed to initialize RSMI device mutex after 5 seconds. Previous " + "execution may not have shutdown cleanly. To fix problem, stop all " + "rocm_smi programs, and then delete the rocm_smi* shared memory files in" + " /dev/shm."); + free(mutex.name); + + throw amd::smi::rsmi_exception(RSMI_STATUS_BUSY, __FUNCTION__); + return mutex; + } else { + const int ret = pthread_mutex_unlock(mutex_ptr); + if (ret) { + perror("pthread_mutex_unlock"); + fprintf(stderr, "%d init_mutex %s: unlock timed lock, ret: %d\n", cur_pid, name, ret); + } + } + + + mutex.ptr = mutex_ptr; mutex.name = reinterpret_cast(malloc(NAME_MAX+1)); (void)snprintf(mutex.name, NAME_MAX + 1, "%s", name); @@ -225,12 +294,17 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { } int shared_mutex_close(shared_mutex_t mutex) { - if (munmap(reinterpret_cast(mutex.ptr), sizeof(pthread_mutex_t))) { + amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance(); + const bool is_thread_only = GetEnvVarUInteger(THREAD_ONLY_ENV_VAR) == 1 || + smi.is_thread_only_mutex(); + if (is_thread_only) { + delete mutex.ptr; + } else if (munmap(reinterpret_cast(mutex.ptr), sizeof(pthread_mutex_t))) { perror("munmap"); return -1; } - mutex.ptr = NULL; - if (close(mutex.shm_fd)) { + mutex.ptr = nullptr; + if (!is_thread_only && close(mutex.shm_fd)) { perror("close"); return -1; } @@ -241,21 +315,26 @@ int shared_mutex_close(shared_mutex_t mutex) { } int shared_mutex_destroy(shared_mutex_t mutex) { + amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance(); + const bool is_thread_only = GetEnvVarUInteger(THREAD_ONLY_ENV_VAR) == 1 || + smi.is_thread_only_mutex(); if ((errno = pthread_mutex_destroy(mutex.ptr))) { perror("pthread_mutex_destroy"); return -1; } - if (munmap(reinterpret_cast(mutex.ptr), sizeof(pthread_mutex_t))) { + if (is_thread_only) { + delete mutex.ptr; + } else if (munmap(reinterpret_cast(mutex.ptr), sizeof(pthread_mutex_t))) { perror("munmap"); return -1; } - mutex.ptr = NULL; - if (close(mutex.shm_fd)) { + mutex.ptr = nullptr; + if (!is_thread_only && close(mutex.shm_fd)) { perror("close"); return -1; } mutex.shm_fd = 0; - if (shm_unlink(mutex.name)) { + if (!is_thread_only && shm_unlink(mutex.name)) { perror("shm_unlink"); return -1; } From 39f37c5d33f99cbb9ba71cebeb2fbb14909b4e02 Mon Sep 17 00:00:00 2001 From: "Bill(Shuzhou) Liu" Date: Thu, 2 May 2024 11:09:57 -0500 Subject: [PATCH 09/12] Remove thread safe only mutex warning message In multiple GPUs environment, too many warning messages generated, and then need to be removed. Change-Id: I275de2397eb0e6b189e2e17e94335cb1e8f97815 [ROCm/amdsmi commit: 78ee3f5e5afaedccad306870c9427cf7d50f6486] --- projects/amdsmi/third_party/shared_mutex/shared_mutex.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc index d9aa1a20b5..414faa1352 100755 --- a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc +++ b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc @@ -141,7 +141,6 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { amd::smi::RocmSMI& smi = amd::smi::RocmSMI::getInstance(); if (GetEnvVarUInteger(THREAD_ONLY_ENV_VAR) == 1 || smi.is_thread_only_mutex()) { - fprintf(stderr, "rocm-smi: using thread safe only mutex\n"); return init_thread_safe_only(name); } From 12143a4ec8c8e7f7c378b2a78abe71a534cddc3d Mon Sep 17 00:00:00 2001 From: "Galantsev, Dmitrii" Date: Fri, 19 Jul 2024 00:13:45 -0500 Subject: [PATCH 10/12] [SWDEV-471523] - Replace lspci and lscpu with sysfs reads Change-Id: Ia5a12c04d3064787a0f36a961e68533b79e17ed0 Signed-off-by: Galantsev, Dmitrii [ROCm/amdsmi commit: baaa1cc4b69b69240dce9bde894a84a78de45292] --- projects/amdsmi/amdsmi_cli/amdsmi_helpers.py | 32 +++++++++++++++----- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py index 952f9c57db..a6fec6d318 100644 --- a/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py +++ b/projects/amdsmi/amdsmi_cli/amdsmi_helpers.py @@ -22,6 +22,7 @@ import logging import math +import os import platform import sys import time @@ -58,16 +59,19 @@ class AMDSMIHelpers(): self._is_linux = True logging.debug(f"AMDSMIHelpers: Platform is linux:{self._is_linux}") - output = run(["lscpu"], stdout=PIPE, stderr=STDOUT, encoding="UTF-8").stdout - if "hypervisor" not in output: - self._is_baremetal = True - else: - self._is_virtual_os = True + try: + with open('/proc/cpuinfo', 'r') as f: + if 'hypervisor' in f.read(): + self._is_virtual_os = True + except IOError: + pass + + self._is_baremetal = not self._is_virtual_os # Check for passthrough system filtering by device id - output = run(["lspci", "-nn"], stdout=PIPE, stderr=STDOUT, encoding="UTF-8").stdout + output = self.get_pci_device_ids() passthrough_device_ids = ["7460", "73c8", "74a0", "74a1", "74a2"] - if any(device_id in output for device_id in passthrough_device_ids): + if any(('0x' + device_id) in output for device_id in passthrough_device_ids): if self._is_virtual_os: self._is_baremetal = True self._is_virtual_os = False @@ -784,3 +788,17 @@ class AMDSMIHelpers(): int : converted SI unit of value requested """ return int(float(val) * unit_in / unit_out) + + def get_pci_device_ids(self) -> set[str]: + pci_devices_path = "/sys/bus/pci/devices" + pci_devices: set[str] = set() + for device in os.listdir(pci_devices_path): + subsystem_device_path = os.path.join(pci_devices_path, device, "subsystem_device") + try: + with open(subsystem_device_path, 'r') as f: + subsystem_device = f.read().strip() + pci_devices.add(subsystem_device) + except Exception as _: + continue + return pci_devices + From 49e4e8f2345a57ac2dce3165baec702fefb693a0 Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Fri, 19 Jul 2024 09:43:22 -0500 Subject: [PATCH 11/12] Updated Changelog with Mutex Fix Signed-off-by: Maisam Arif Change-Id: I0aee284ce7600efc66b0ad5392c11bb6a502a929 [ROCm/amdsmi commit: 3a9c93bfa6d2b6404eebc01a83707699cfbb95e7] --- projects/amdsmi/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/projects/amdsmi/CHANGELOG.md b/projects/amdsmi/CHANGELOG.md index 11ca692f3a..da6bd5d8a6 100644 --- a/projects/amdsmi/CHANGELOG.md +++ b/projects/amdsmi/CHANGELOG.md @@ -145,6 +145,9 @@ ASIC products. This requires users to update any ABIs using this structure. ### Fixes +- **Fixed Leftover Mutex deadlock when running multiple instances of the CLI tool**. +When running `amd-smi reset --gpureset --gpu all` and then running an instance of `amd-smi static` (or any other subcommand that access the GPUs) a mutex would lock and not return requiring either a clear of the mutex in /dev/shm or rebooting the machine. + - **Fixed multiple processes not being registered in `amd-smi process` with json and csv format**. Multiple process outputs in the CLI tool were not being registered correctly. The json output did not handle multiple processes and is now in a new valid json format: From c63ea0e5e00a73af21e61c09ac7e8f6866dd4113 Mon Sep 17 00:00:00 2001 From: Maisam Arif Date: Fri, 19 Jul 2024 10:47:03 -0500 Subject: [PATCH 12/12] [SWDEV-474450] Removed DEVICE_MUTEX from gpu_reset Signed-off-by: Maisam Arif Change-Id: I706fb47288738bfbde94b56fee66bbf807b3c0cb [ROCm/amdsmi commit: 8bc8307c60027812224eea175ca261ba5c62c033] --- projects/amdsmi/rocm_smi/src/rocm_smi.cc | 3 ++- projects/amdsmi/third_party/shared_mutex/shared_mutex.cc | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/projects/amdsmi/rocm_smi/src/rocm_smi.cc b/projects/amdsmi/rocm_smi/src/rocm_smi.cc index 12e269590f..fd4ace7f9f 100755 --- a/projects/amdsmi/rocm_smi/src/rocm_smi.cc +++ b/projects/amdsmi/rocm_smi/src/rocm_smi.cc @@ -3435,7 +3435,8 @@ rsmi_dev_gpu_reset(uint32_t dv_ind) { ss << __PRETTY_FUNCTION__ << "| ======= start ======="; LOG_TRACE(ss); REQUIRE_ROOT_ACCESS - DEVICE_MUTEX + // No longer using DEVICE_MUTEX as it blocks long running processes + // DEVICE_MUTEX rsmi_status_t ret; uint64_t status_code = 0; diff --git a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc index 414faa1352..59504ee1fc 100755 --- a/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc +++ b/projects/amdsmi/third_party/shared_mutex/shared_mutex.cc @@ -255,10 +255,11 @@ shared_mutex_t shared_mutex_init(const char *name, mode_t mode, bool retried) { // When process crash before unlock the mutex, the mutex is in bad status. // reset the mutex if no process is using it, and then retry lock if (!retried) { - std::vector ids = lsof(name); + std::string shared_mutex_filename = "/dev/shm" + std::string(name); + std::vector ids = lsof(shared_mutex_filename.c_str()); if (ids.size() == 0) { // no process is using it fprintf(stderr, "%d re-init the mutex %s since no one use it. ret:%d ptr:%p\n", - cur_pid, name, ret, reinterpret_cast(addr)->ptr); + cur_pid, shared_mutex_filename, ret, reinterpret_cast(addr)->ptr); memset(mutex_ptr, 0, sizeof(pthread_mutex_t)); // Set mutex.created == 1 so that it can be initialized latter. mutex.created = 1;