From 53b42802818734a3b1915135ee653099700ee9a1 Mon Sep 17 00:00:00 2001 From: Alexander Van Craen Date: Wed, 12 Aug 2026 22:07:39 +0200 Subject: [PATCH 1/4] fix ROCm SMI device index resolution under device visibility masking gpu_amd_hardware_sampler used the HIP-relative device index directly as the ROCm SMI index for every rsmi_dev_* call. ROCm SMI's own device enumeration isn't filtered by HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES the way HIP's is, so under a non-default visibility mask the sampler measured the wrong physical GPU. Fix: resolve the actual ROCm SMI index once at construction via a PCI-bus-ID match against every ROCm SMI device, throwing if no match is found instead of silently falling back to the HIP-relative index. The original HIP-relative index is kept in a new hip_device_id_ member for the one HIP call that still needs HIP-space. Also adds the missing find_dependency(MPI) to hwsConfig.cmake.in, alongside the existing ones for CUDA/HIP/ROCm-SMI/Level-Zero. --- cmake/hwsConfig.cmake.in | 9 +++ include/hws/gpu_amd/hardware_sampler.hpp | 40 ++++++++++++- include/hws/gpu_amd/utility.hpp | 25 +++++++- include/hws/utility.hpp | 17 ++++++ src/hws/gpu_amd/hardware_sampler.cpp | 75 +++++++++++++++++++++++- src/hws/gpu_amd/utility.cpp | 49 +++++++++++++--- src/hws/utility.cpp | 7 +++ 7 files changed, 209 insertions(+), 13 deletions(-) diff --git a/cmake/hwsConfig.cmake.in b/cmake/hwsConfig.cmake.in index bde52ae..e14e3c8 100644 --- a/cmake/hwsConfig.cmake.in +++ b/cmake/hwsConfig.cmake.in @@ -112,6 +112,15 @@ if (HWS_HAS_CPU_SUPPORT) endif () endif () +# check whether MPI support is enabled +string_contains("${HWS_COMPILE_DEFINITIONS}" "HWS_MPI_SUPPORT_ENABLED" HWS_HAS_MPI_SUPPORT) +if (HWS_HAS_MPI_SUPPORT) + find_dependency(MPI COMPONENTS CXX) + if (NOT hws_FIND_QUIETLY) + message(STATUS "Enabled MPI support via hws.") + endif () +endif () + # check whether NVIDIA GPUs are supported string_contains("${HWS_COMPILE_DEFINITIONS}" "HWS_FOR_NVIDIA_GPUS_ENABLED" HWS_HAS_GPU_NVIDIA_SUPPORT) if (HWS_HAS_GPU_NVIDIA_SUPPORT) diff --git a/include/hws/gpu_amd/hardware_sampler.hpp b/include/hws/gpu_amd/hardware_sampler.hpp index 668cc9a..ce0e40f 100644 --- a/include/hws/gpu_amd/hardware_sampler.hpp +++ b/include/hws/gpu_amd/hardware_sampler.hpp @@ -116,6 +116,40 @@ class gpu_amd_hardware_sampler : public hardware_sampler { */ [[nodiscard]] const rocm_smi_temperature_samples &temperature_samples() const noexcept { return temperature_samples_; } + /** + * @brief Return the ROCm SMI device index this hardware sampler uses for all of its `rsmi_dev_*` calls. + * @details Resolved at construction time from the HIP-relative index passed to the constructor by matching + * PCI bus IDs, since ROCm SMI's own device enumeration is *not* affected by + * `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES` the way HIP's is - the two can otherwise disagree + * about which physical device a given index refers to. Purely local/informational: don't assume + * this index is stable across processes or reruns; use `pci_bus_id()` to identify the actual + * physical device. + * @return the ROCm SMI device index (`[[nodiscard]]`) + */ + [[nodiscard]] std::uint32_t device_id() const noexcept { return device_id_; } + + /** + * @brief Return the HIP-relative device index this hardware sampler was constructed with (i.e. the index + * into the process's `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES`-filtered device list). + * @details Unlike `device_id()`, this is exactly the constructor argument, unresolved - the right value to + * report when identifying "the Nth GPU visible to this process/rank" (e.g. for a per-rank device + * list), as opposed to `pci_bus_id()`/`device_id()` which identify the physical device itself. + * @return the HIP-relative device index (`[[nodiscard]]`) + */ + [[nodiscard]] std::uint32_t hip_device_id() const noexcept { return hip_device_id_; } + + /** + * @brief Return the PCI bus ID (e.g. `"0000:c1:00.0"`) of the physical device this hardware sampler actually + * measures. + * @details Queried via `rsmi_dev_pci_id_get()` using the same `device_id()` index this sampler already uses + * for every other `rsmi_dev_*` call, so - unlike combining `device_id()` with a different API family + * (e.g. HIP's `hipDeviceGetPCIBusId()`) - this is guaranteed to identify the exact physical device + * being sampled, even if `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES` causes ROCm SMI's and HIP's + * device enumerations to diverge. Matches the format used by `enumerate_all_amd_gpu_pci_bus_ids()`. + * @return the PCI bus ID (`[[nodiscard]]`) + */ + [[nodiscard]] std::string pci_bus_id() const; + /** * @copydoc hws::hardware_sampler::device_identification */ @@ -132,8 +166,12 @@ class gpu_amd_hardware_sampler : public hardware_sampler { */ void sampling_loop() final; - /// The ID of the device to sample. + /// The ROCm SMI device index to sample, resolved from hip_device_id_ via a PCI bus ID match (ROCm SMI's own + /// device enumeration isn't affected by HIP_VISIBLE_DEVICES/ROCR_VISIBLE_DEVICES, unlike HIP's). std::uint32_t device_id_{}; + /// The HIP-relative device index this hardware sampler was constructed with; only used for the one HIP call + /// (hipGetDeviceProperties) that needs a HIP-space rather than a ROCm-SMI-space index. + std::uint32_t hip_device_id_{}; /// The general AMD GPU samples. rocm_smi_general_samples general_samples_{}; diff --git a/include/hws/gpu_amd/utility.hpp b/include/hws/gpu_amd/utility.hpp index def0937..c0670a0 100644 --- a/include/hws/gpu_amd/utility.hpp +++ b/include/hws/gpu_amd/utility.hpp @@ -17,11 +17,10 @@ #include // std::runtime_error #include // std::string +#include // std::vector #if defined(HWS_MPI_SUPPORT_ENABLED) #include "hws/visible_gpu_device.hpp" // hws::detail::visible_gpu_device - - #include // std::vector #endif namespace hws::detail { @@ -74,6 +73,28 @@ namespace hws::detail { */ [[nodiscard]] std::string performance_level_to_string(rsmi_dev_perf_level_t perf_level); +/** + * @brief Return the PCI bus ID (e.g. `"0000:c1:00.0"`) of the AMD GPU device with the given HIP @p local_index. + * @details This is the same, stable identifier used by `enumerate_all_amd_gpu_pci_bus_ids()`, so the two can be + * matched against each other to locate a HIP-visible device within the full physical GPU topology. + * @param[in] local_index the local HIP device index + * @return the PCI bus ID (`[[nodiscard]]`) + */ +[[nodiscard]] std::string amd_device_pci_bus_id(int local_index); + +/** + * @brief Enumerate the PCI bus IDs of every AMD GPU physically present on the node, independent of any + * process-level device visibility filtering (e.g. `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES`). + * @details Reads the kernel's view of devices bound to the `amdgpu` driver directly from + * `/sys/bus/pci/drivers/amdgpu/`, since that sysfs directory - unlike the HIP runtime's device + * enumeration - isn't affected by per-process visible-device environment variables. It *can* still be + * restricted below the true physical device count in a batch job with kernel-level (cgroup) device + * isolation for a partial-node allocation, so callers must treat an unexpectedly low count as "topology + * unknown", not as ground truth. + * @return the sorted PCI bus IDs of all `amdgpu`-bound devices, or an empty vector if the directory doesn't exist + * or isn't readable (`[[nodiscard]]`) + */ +[[nodiscard]] std::vector enumerate_all_amd_gpu_pci_bus_ids(); #if defined(HWS_MPI_SUPPORT_ENABLED) diff --git a/include/hws/utility.hpp b/include/hws/utility.hpp index 2737418..921f29e 100644 --- a/include/hws/utility.hpp +++ b/include/hws/utility.hpp @@ -19,6 +19,7 @@ #include // std::chrono::duration #include // std::trunc #include // std::size_t +#include // std::uint32_t #include // std::optional #include // std::runtime_error #include // std::string, std::stof, std::stod, std::stold @@ -255,6 +256,22 @@ template */ [[nodiscard]] std::string indent_lines(const std::string &text, std::string_view prefix); +/** + * @brief Format a PCI domain/bus/device triplet as the canonical Linux sysfs PCI bus ID string + * `"::.0"` (e.g. `"0000:c1:00.0"`), lowercase hex, 4/2/2 digits. + * @details The function is fixed to `0`: on multi-die/multi-partition accelerators (e.g. AMD MI300-series + * "partitions") the PCI function field is repurposed by the vendor's management library for + * partition/die identification, but the function seen by the OS/sysfs for the *device* itself is + * always `0` - so vendor-provided domain/bus/device values should be combined with a hardcoded `0` + * function here rather than a vendor-reported function value, to stay comparable with sysfs PCI bus IDs + * (see e.g. `hws::detail::enumerate_all_amd_gpu_pci_bus_ids()`/`enumerate_all_nvidia_gpu_pci_bus_ids()`). + * @param[in] domain the PCI domain + * @param[in] bus the PCI bus number + * @param[in] device the PCI device (slot) number + * @return the formatted PCI bus ID string (`[[nodiscard]]`) + */ +[[nodiscard]] std::string format_pci_bus_id(std::uint32_t domain, std::uint32_t bus, std::uint32_t device); + /*****************************************************************************************************/ /** other free functions **/ /*****************************************************************************************************/ diff --git a/src/hws/gpu_amd/hardware_sampler.cpp b/src/hws/gpu_amd/hardware_sampler.cpp index 7c0a6a2..977b4f4 100644 --- a/src/hws/gpu_amd/hardware_sampler.cpp +++ b/src/hws/gpu_amd/hardware_sampler.cpp @@ -35,6 +35,53 @@ namespace hws { +namespace { + +/** + * @brief Convert a ROCm SMI BDFID (as returned by `rsmi_dev_pci_id_get()`) to a sysfs-style PCI bus ID string. + * @details BDFID = (DOMAIN << 32) | (PARTITION << 28) | (BUS << 8) | (DEVICE << 3) | FUNCTION (see ROCm SMI's + * `rsmi_dev_pci_id_get` documentation). On MI-series partitioned devices the function bits are + * repurposed for the partition ID instead of a real PCI function - but the OS/sysfs-visible PCI address + * for the device itself always has function 0, so the function is intentionally not extracted here; see + * `hws::detail::format_pci_bus_id()`. + */ +[[nodiscard]] std::string bdfid_to_pci_bus_id(const std::uint64_t bdfid) { + const auto domain = static_cast((bdfid >> 32) & 0xffffffffull); + const auto bus = static_cast((bdfid >> 8) & 0xffull); + const auto device = static_cast((bdfid >> 3) & 0x1full); + return detail::format_pci_bus_id(domain, bus, device); +} + +/** + * @brief Resolve the ROCm SMI device index that corresponds to the physical device HIP considers index + * @p hip_device_id, by matching PCI bus IDs. + * @details Necessary because ROCm SMI enumerates every physical AMD GPU on the node unconditionally, while HIP's + * enumeration is filtered/reordered by `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES` - the same index + * number in both APIs can refer to different physical devices. Requires `rsmi_init()` to have already + * been called. + * @throws std::runtime_error if ROCm SMI's device count can't be queried, or if none of its devices' PCI bus IDs + * match @p hip_device_id's - silently falling back to @p hip_device_id here would be exactly the + * HIP-index-used-as-RSMI-index bug this function exists to avoid, just triggered by a query failure + * instead of a visibility mask. + */ +[[nodiscard]] std::uint32_t resolve_rsmi_device_id(const std::uint32_t hip_device_id) { + std::uint32_t rsmi_count{}; + if (rsmi_num_monitor_devices(&rsmi_count) != RSMI_STATUS_SUCCESS) { + throw std::runtime_error{ "gpu_amd_hardware_sampler: couldn't query the number of ROCm SMI devices while resolving the physical device for HIP index " + std::to_string(hip_device_id) + "!" }; + } + + const std::string hip_bus_id = detail::amd_device_pci_bus_id(static_cast(hip_device_id)); + for (std::uint32_t rsmi_idx = 0; rsmi_idx < rsmi_count; ++rsmi_idx) { + std::uint64_t bdfid{}; + if (rsmi_dev_pci_id_get(rsmi_idx, &bdfid) == RSMI_STATUS_SUCCESS && bdfid_to_pci_bus_id(bdfid) == hip_bus_id) { + return rsmi_idx; + } + } + throw std::runtime_error{ "gpu_amd_hardware_sampler: couldn't find a ROCm SMI device with PCI bus ID " + hip_bus_id + " (HIP index " + std::to_string(hip_device_id) + ")!" }; +} + +} // namespace + gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const sample_category category) : gpu_amd_hardware_sampler{ 0, HWS_SAMPLING_INTERVAL, category } { } @@ -46,9 +93,10 @@ gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const std::chrono::millisecon gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const std::size_t device_id, const std::chrono::milliseconds sampling_interval, const sample_category category) : hardware_sampler{ sampling_interval, category }, - device_id_{ static_cast(device_id) } { + hip_device_id_{ static_cast(device_id) } { // make sure that rsmi_init is only called once for all instances - if (instances_++ == 0) { + const bool is_first_instance = (instances_++ == 0); + if (is_first_instance) { HWS_ROCM_SMI_ERROR_CHECK(rsmi_init(std::uint64_t{ 0 })) // notify that initialization has been finished init_finished_ = true; @@ -56,6 +104,21 @@ gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const std::size_t device_id, // wait until init has been finished! while (!init_finished_) { } } + + // resolve device_id_ only after rsmi_init() has definitely run (by this instance or a previous one); if + // resolution throws, this instance never finishes construction and its destructor never runs, so + // instances_/init_finished_ (and, if we were the one that just initialized ROCm SMI, the ROCm SMI runtime + // itself) must be rolled back manually here instead of leaking + try { + device_id_ = resolve_rsmi_device_id(hip_device_id_); + } catch (...) { + --instances_; + if (is_first_instance) { + init_finished_ = false; + rsmi_shut_down(); + } + throw; + } } gpu_amd_hardware_sampler::~gpu_amd_hardware_sampler() { @@ -94,7 +157,7 @@ void gpu_amd_hardware_sampler::sampling_loop() { general_samples_.byte_order_ = "Little Endian"; hipDeviceProp_t prop{}; - if (hipGetDeviceProperties(&prop, static_cast(device_id_)) == hipSuccess) { + if (hipGetDeviceProperties(&prop, static_cast(hip_device_id_)) == hipSuccess) { const std::string architecture{ prop.gcnArchName }; general_samples_.architecture_ = architecture.substr(0, architecture.find_first_of('\0')); } @@ -681,6 +744,12 @@ std::string gpu_amd_hardware_sampler::device_identification() const { return fmt::format("gpu_amd_device_{}", device_id_); } +std::string gpu_amd_hardware_sampler::pci_bus_id() const { + std::uint64_t bdfid{}; + HWS_ROCM_SMI_ERROR_CHECK(rsmi_dev_pci_id_get(device_id_, &bdfid)) + return bdfid_to_pci_bus_id(bdfid); +} + std::string gpu_amd_hardware_sampler::samples_only_as_yaml_string() const { // check whether it's safe to generate the YAML entry if (this->is_sampling()) { diff --git a/src/hws/gpu_amd/utility.cpp b/src/hws/gpu_amd/utility.cpp index 55d6932..a3dedf5 100644 --- a/src/hws/gpu_amd/utility.cpp +++ b/src/hws/gpu_amd/utility.cpp @@ -9,13 +9,16 @@ #include "rocm_smi/rocm_smi.h" // ROCm SMI runtime functions -#include // std::string -#include // std::vector +#include "hip/hip_runtime_api.h" // hipGetDeviceCount, hipDeviceGetPCIBusId + +#include // std::sort +#include // std::filesystem::{directory_iterator, exists, directory_options} +#include // std::string +#include // std::error_code +#include // std::vector #if defined(HWS_MPI_SUPPORT_ENABLED) && defined(HWS_FOR_AMD_GPUS_ENABLED) #include "hws/visible_gpu_device.hpp" // hws::detail::visible_gpu_device, hws::detail::device_backend_kind - - #include "hip/hip_runtime_api.h" // hipGetDeviceCount, hipDeviceGetPCIBusId #endif namespace hws::detail { @@ -46,6 +49,40 @@ std::string performance_level_to_string(const rsmi_dev_perf_level_t perf_level) } } +std::string amd_device_pci_bus_id(const int local_index) { + char bus_id[64] = {}; + HWS_HIP_ERROR_CHECK(hipDeviceGetPCIBusId(bus_id, sizeof(bus_id), local_index)); + return std::string{ bus_id }; +} + +std::vector enumerate_all_amd_gpu_pci_bus_ids() { + std::vector bus_ids{}; + + const std::filesystem::path amdgpu_driver_dir{ "/sys/bus/pci/drivers/amdgpu" }; + std::error_code ec{}; + if (!std::filesystem::exists(amdgpu_driver_dir, ec) || ec) { + return bus_ids; + } + + for (const std::filesystem::directory_entry &entry : std::filesystem::directory_iterator(amdgpu_driver_dir, std::filesystem::directory_options::skip_permission_denied, ec)) { + if (ec) { + break; + } + // every PCI device bound to the amdgpu driver shows up here as a symlink named after its PCI bus ID, + // e.g. "0000:c1:00.0" -> ../../../devices/.../0000:c1:00.0 + if (!entry.is_symlink(ec)) { + continue; + } + const std::string name = entry.path().filename().string(); + if (name.find(':') != std::string::npos && name.find('.') != std::string::npos) { + bus_ids.push_back(name); + } + } + + std::sort(bus_ids.begin(), bus_ids.end()); + return bus_ids; +} + #if defined(HWS_MPI_SUPPORT_ENABLED) && defined(HWS_FOR_AMD_GPUS_ENABLED) namespace { @@ -58,9 +95,7 @@ namespace { * @return the physical ID of the AMD GPU device */ [[nodiscard]] std::string amd_physical_id(const int local_index) { - char bus_id[64] = {}; - HWS_HIP_ERROR_CHECK(hipDeviceGetPCIBusId(bus_id, sizeof(bus_id), local_index)); - return std::string{ "amd:" } + bus_id; + return std::string{ "amd:" } + amd_device_pci_bus_id(local_index); } } // namespace diff --git a/src/hws/utility.cpp b/src/hws/utility.cpp index 406089a..8138ea5 100644 --- a/src/hws/utility.cpp +++ b/src/hws/utility.cpp @@ -7,8 +7,11 @@ #include "hws/utility.hpp" +#include "fmt/format.h" // fmt::format + #include // std::min, std::transform, std::all_of #include // std::tolower, std::isdigit +#include // std::uint32_t #include // std::stringstream #include // std::string #include // std::string_view @@ -77,4 +80,8 @@ std::string indent_lines(const std::string &text, const std::string_view prefix) return out; } +std::string format_pci_bus_id(const std::uint32_t domain, const std::uint32_t bus, const std::uint32_t device) { + return fmt::format("{:04x}:{:02x}:{:02x}.0", domain, bus, device); +} + } // namespace hws::detail From 27cf62c18ca4dbe8d7fff2d2d443e4daff1092e4 Mon Sep 17 00:00:00 2001 From: Alexander Van Craen Date: Thu, 13 Aug 2026 09:00:02 +0200 Subject: [PATCH 2/4] address review findings on the ROCm SMI index fix The constructor's rollback on a failed device resolution kept ROCm SMI's shared instances_/init_finished_ lifecycle keyed off is_first_instance. Under concurrent construction of several gpu_amd_hardware_sampler instances, that could shut ROCm SMI down while a sibling instance was still using it, or leave a sibling stuck forever waiting on init_finished_. Fixed by mirroring the destructor's own "last instance out" check (instances_ reaching zero, and only if ROCm SMI was actually initialized) instead, and by moving rsmi_init() itself into the try block so its own failure is handled the same way. resolve_rsmi_device_id() also silently used the first ROCm SMI device whose PCI bus ID matched. bdfid_to_pci_bus_id() intentionally drops the BDFID's partition bits, so on a partitioned MI-series accelerator several ROCm SMI entries can share one normalized bus ID - silently picking the first would reintroduce the exact silently-wrong-device failure mode this fix exists to close. Now throws on an ambiguous match instead. Also drops the device_id()/hip_device_id()/pci_bus_id() getters and enumerate_all_amd_gpu_pci_bus_ids(): unused on this branch, they only exist to serve the accel<->GPU correlation-hints feature that stays on feature/cray-pm-counters. --- include/hws/gpu_amd/hardware_sampler.hpp | 34 -------------- include/hws/gpu_amd/utility.hpp | 16 ------- src/hws/gpu_amd/hardware_sampler.cpp | 60 +++++++++++++----------- src/hws/gpu_amd/utility.cpp | 36 ++------------ 4 files changed, 36 insertions(+), 110 deletions(-) diff --git a/include/hws/gpu_amd/hardware_sampler.hpp b/include/hws/gpu_amd/hardware_sampler.hpp index ce0e40f..4cdfbcc 100644 --- a/include/hws/gpu_amd/hardware_sampler.hpp +++ b/include/hws/gpu_amd/hardware_sampler.hpp @@ -116,40 +116,6 @@ class gpu_amd_hardware_sampler : public hardware_sampler { */ [[nodiscard]] const rocm_smi_temperature_samples &temperature_samples() const noexcept { return temperature_samples_; } - /** - * @brief Return the ROCm SMI device index this hardware sampler uses for all of its `rsmi_dev_*` calls. - * @details Resolved at construction time from the HIP-relative index passed to the constructor by matching - * PCI bus IDs, since ROCm SMI's own device enumeration is *not* affected by - * `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES` the way HIP's is - the two can otherwise disagree - * about which physical device a given index refers to. Purely local/informational: don't assume - * this index is stable across processes or reruns; use `pci_bus_id()` to identify the actual - * physical device. - * @return the ROCm SMI device index (`[[nodiscard]]`) - */ - [[nodiscard]] std::uint32_t device_id() const noexcept { return device_id_; } - - /** - * @brief Return the HIP-relative device index this hardware sampler was constructed with (i.e. the index - * into the process's `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES`-filtered device list). - * @details Unlike `device_id()`, this is exactly the constructor argument, unresolved - the right value to - * report when identifying "the Nth GPU visible to this process/rank" (e.g. for a per-rank device - * list), as opposed to `pci_bus_id()`/`device_id()` which identify the physical device itself. - * @return the HIP-relative device index (`[[nodiscard]]`) - */ - [[nodiscard]] std::uint32_t hip_device_id() const noexcept { return hip_device_id_; } - - /** - * @brief Return the PCI bus ID (e.g. `"0000:c1:00.0"`) of the physical device this hardware sampler actually - * measures. - * @details Queried via `rsmi_dev_pci_id_get()` using the same `device_id()` index this sampler already uses - * for every other `rsmi_dev_*` call, so - unlike combining `device_id()` with a different API family - * (e.g. HIP's `hipDeviceGetPCIBusId()`) - this is guaranteed to identify the exact physical device - * being sampled, even if `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES` causes ROCm SMI's and HIP's - * device enumerations to diverge. Matches the format used by `enumerate_all_amd_gpu_pci_bus_ids()`. - * @return the PCI bus ID (`[[nodiscard]]`) - */ - [[nodiscard]] std::string pci_bus_id() const; - /** * @copydoc hws::hardware_sampler::device_identification */ diff --git a/include/hws/gpu_amd/utility.hpp b/include/hws/gpu_amd/utility.hpp index c0670a0..f72f4dc 100644 --- a/include/hws/gpu_amd/utility.hpp +++ b/include/hws/gpu_amd/utility.hpp @@ -75,27 +75,11 @@ namespace hws::detail { /** * @brief Return the PCI bus ID (e.g. `"0000:c1:00.0"`) of the AMD GPU device with the given HIP @p local_index. - * @details This is the same, stable identifier used by `enumerate_all_amd_gpu_pci_bus_ids()`, so the two can be - * matched against each other to locate a HIP-visible device within the full physical GPU topology. * @param[in] local_index the local HIP device index * @return the PCI bus ID (`[[nodiscard]]`) */ [[nodiscard]] std::string amd_device_pci_bus_id(int local_index); -/** - * @brief Enumerate the PCI bus IDs of every AMD GPU physically present on the node, independent of any - * process-level device visibility filtering (e.g. `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES`). - * @details Reads the kernel's view of devices bound to the `amdgpu` driver directly from - * `/sys/bus/pci/drivers/amdgpu/`, since that sysfs directory - unlike the HIP runtime's device - * enumeration - isn't affected by per-process visible-device environment variables. It *can* still be - * restricted below the true physical device count in a batch job with kernel-level (cgroup) device - * isolation for a partial-node allocation, so callers must treat an unexpectedly low count as "topology - * unknown", not as ground truth. - * @return the sorted PCI bus IDs of all `amdgpu`-bound devices, or an empty vector if the directory doesn't exist - * or isn't readable (`[[nodiscard]]`) - */ -[[nodiscard]] std::vector enumerate_all_amd_gpu_pci_bus_ids(); - #if defined(HWS_MPI_SUPPORT_ENABLED) /** diff --git a/src/hws/gpu_amd/hardware_sampler.cpp b/src/hws/gpu_amd/hardware_sampler.cpp index 977b4f4..3672143 100644 --- a/src/hws/gpu_amd/hardware_sampler.cpp +++ b/src/hws/gpu_amd/hardware_sampler.cpp @@ -59,9 +59,11 @@ namespace { * enumeration is filtered/reordered by `HIP_VISIBLE_DEVICES`/`ROCR_VISIBLE_DEVICES` - the same index * number in both APIs can refer to different physical devices. Requires `rsmi_init()` to have already * been called. - * @throws std::runtime_error if ROCm SMI's device count can't be queried, or if none of its devices' PCI bus IDs - * match @p hip_device_id's - silently falling back to @p hip_device_id here would be exactly the - * HIP-index-used-as-RSMI-index bug this function exists to avoid, just triggered by a query failure + * @throws std::runtime_error if ROCm SMI's device count can't be queried, if none of its devices' PCI bus IDs + * match @p hip_device_id's, or if more than one does - `bdfid_to_pci_bus_id()` deliberately drops the + * BDFID's partition bits (see its docs), so on a partitioned MI-series accelerator several ROCm SMI + * entries can share one normalized bus ID; silently returning the first match there would be exactly the + * HIP-index-used-as-RSMI-index bug this function exists to avoid, just triggered by partition mode * instead of a visibility mask. */ [[nodiscard]] std::uint32_t resolve_rsmi_device_id(const std::uint32_t hip_device_id) { @@ -71,13 +73,20 @@ namespace { } const std::string hip_bus_id = detail::amd_device_pci_bus_id(static_cast(hip_device_id)); + std::optional resolved{}; for (std::uint32_t rsmi_idx = 0; rsmi_idx < rsmi_count; ++rsmi_idx) { std::uint64_t bdfid{}; if (rsmi_dev_pci_id_get(rsmi_idx, &bdfid) == RSMI_STATUS_SUCCESS && bdfid_to_pci_bus_id(bdfid) == hip_bus_id) { - return rsmi_idx; + if (resolved.has_value()) { + throw std::runtime_error{ "gpu_amd_hardware_sampler: found more than one ROCm SMI device with PCI bus ID " + hip_bus_id + " (HIP index " + std::to_string(hip_device_id) + ") - likely a partitioned accelerator, which isn't supported yet!" }; + } + resolved = rsmi_idx; } } - throw std::runtime_error{ "gpu_amd_hardware_sampler: couldn't find a ROCm SMI device with PCI bus ID " + hip_bus_id + " (HIP index " + std::to_string(hip_device_id) + ")!" }; + if (!resolved.has_value()) { + throw std::runtime_error{ "gpu_amd_hardware_sampler: couldn't find a ROCm SMI device with PCI bus ID " + hip_bus_id + " (HIP index " + std::to_string(hip_device_id) + ")!" }; + } + return resolved.value(); } } // namespace @@ -94,26 +103,29 @@ gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const std::chrono::millisecon gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const std::size_t device_id, const std::chrono::milliseconds sampling_interval, const sample_category category) : hardware_sampler{ sampling_interval, category }, hip_device_id_{ static_cast(device_id) } { - // make sure that rsmi_init is only called once for all instances + // make sure that rsmi_init is only called once for all instances; rsmi_init() itself is inside the try so that + // a failing first instance is rolled back the exact same way as a failing resolve_rsmi_device_id() below, + // instead of leaking instances_/init_finished_ through an early, uncaught throw const bool is_first_instance = (instances_++ == 0); - if (is_first_instance) { - HWS_ROCM_SMI_ERROR_CHECK(rsmi_init(std::uint64_t{ 0 })) - // notify that initialization has been finished - init_finished_ = true; - } else { - // wait until init has been finished! - while (!init_finished_) { } - } - - // resolve device_id_ only after rsmi_init() has definitely run (by this instance or a previous one); if - // resolution throws, this instance never finishes construction and its destructor never runs, so - // instances_/init_finished_ (and, if we were the one that just initialized ROCm SMI, the ROCm SMI runtime - // itself) must be rolled back manually here instead of leaking try { + if (is_first_instance) { + HWS_ROCM_SMI_ERROR_CHECK(rsmi_init(std::uint64_t{ 0 })) + // notify that initialization has been finished + init_finished_ = true; + } else { + // wait until init has been finished! + while (!init_finished_) { } + } + + // resolve device_id_ only after rsmi_init() has definitely run (by this instance or a previous one) device_id_ = resolve_rsmi_device_id(hip_device_id_); } catch (...) { - --instances_; - if (is_first_instance) { + // mirror the destructor's "last instance out shuts ROCm SMI down" logic (keyed off instances_ reaching + // zero, not off is_first_instance) - by the time this runs, another constructor may already have passed + // the init_finished_ gate and be actively using ROCm SMI, so only the instance that brings the shared + // count back to zero may touch its lifetime, and only if it was actually initialized (init_finished_) - + // otherwise rsmi_init() itself is what failed and there is nothing to shut down + if (--instances_ == 0 && init_finished_) { init_finished_ = false; rsmi_shut_down(); } @@ -744,12 +756,6 @@ std::string gpu_amd_hardware_sampler::device_identification() const { return fmt::format("gpu_amd_device_{}", device_id_); } -std::string gpu_amd_hardware_sampler::pci_bus_id() const { - std::uint64_t bdfid{}; - HWS_ROCM_SMI_ERROR_CHECK(rsmi_dev_pci_id_get(device_id_, &bdfid)) - return bdfid_to_pci_bus_id(bdfid); -} - std::string gpu_amd_hardware_sampler::samples_only_as_yaml_string() const { // check whether it's safe to generate the YAML entry if (this->is_sampling()) { diff --git a/src/hws/gpu_amd/utility.cpp b/src/hws/gpu_amd/utility.cpp index a3dedf5..603922f 100644 --- a/src/hws/gpu_amd/utility.cpp +++ b/src/hws/gpu_amd/utility.cpp @@ -11,14 +11,12 @@ #include "hip/hip_runtime_api.h" // hipGetDeviceCount, hipDeviceGetPCIBusId -#include // std::sort -#include // std::filesystem::{directory_iterator, exists, directory_options} -#include // std::string -#include // std::error_code -#include // std::vector +#include // std::string #if defined(HWS_MPI_SUPPORT_ENABLED) && defined(HWS_FOR_AMD_GPUS_ENABLED) #include "hws/visible_gpu_device.hpp" // hws::detail::visible_gpu_device, hws::detail::device_backend_kind + + #include // std::vector #endif namespace hws::detail { @@ -55,34 +53,6 @@ std::string amd_device_pci_bus_id(const int local_index) { return std::string{ bus_id }; } -std::vector enumerate_all_amd_gpu_pci_bus_ids() { - std::vector bus_ids{}; - - const std::filesystem::path amdgpu_driver_dir{ "/sys/bus/pci/drivers/amdgpu" }; - std::error_code ec{}; - if (!std::filesystem::exists(amdgpu_driver_dir, ec) || ec) { - return bus_ids; - } - - for (const std::filesystem::directory_entry &entry : std::filesystem::directory_iterator(amdgpu_driver_dir, std::filesystem::directory_options::skip_permission_denied, ec)) { - if (ec) { - break; - } - // every PCI device bound to the amdgpu driver shows up here as a symlink named after its PCI bus ID, - // e.g. "0000:c1:00.0" -> ../../../devices/.../0000:c1:00.0 - if (!entry.is_symlink(ec)) { - continue; - } - const std::string name = entry.path().filename().string(); - if (name.find(':') != std::string::npos && name.find('.') != std::string::npos) { - bus_ids.push_back(name); - } - } - - std::sort(bus_ids.begin(), bus_ids.end()); - return bus_ids; -} - #if defined(HWS_MPI_SUPPORT_ENABLED) && defined(HWS_FOR_AMD_GPUS_ENABLED) namespace { From 8695fa4c43761a2cdf23106a6434a9831f0aa165 Mon Sep 17 00:00:00 2001 From: Alexander Van Craen Date: Thu, 13 Aug 2026 11:22:20 +0200 Subject: [PATCH 3/4] fix NVML device index resolution under device visibility masking gpu_nvidia_hardware_sampler had the same bug just fixed for AMD: nvmlDeviceGetHandleByIndex() was called with the raw CUDA-relative device index. NVML's own device enumeration is not filtered by CUDA_VISIBLE_DEVICES the way CUDA's is, so under a non-default visibility mask the sampler silently measured the wrong physical GPU. Confirmed on real hardware (2x NVIDIA RTX 3090): masking CUDA_VISIBLE_DEVICES to the second GPU and constructing with CUDA-relative index 0 resolved to the first physical GPU before this fix, and correctly to the second after. Fix mirrors gpu_amd_hardware_sampler's resolve_rsmi_device_id(): resolve the true NVML index once at construction via a PCI-bus-ID match against every NVML device, throwing on zero or more than one match instead of guessing. NVIDIA MIG instances are not disambiguated by this - NVML's device enumeration only exposes physical parent GPUs, not MIG device handles, so a MIG-sliced CUDA device still resolves to (and samples) its whole parent GPU; documented as a known limitation rather than silently claimed as handled. The pre-existing instances_/init_finished_ busy-wait (shared by both the AMD and NVIDIA backends, unchanged by either fix) can still deadlock a concurrent waiter if the first instance's rsmi_init()/nvmlInit() call itself fails, or race a destructor's shutdown against a new constructor's init. Both are latent, pre-existing issues unreachable through hws's own API (system_hardware_sampler constructs sequentially) - tracked separately rather than fixed here to keep this change scoped to device index resolution. --- include/hws/gpu_nvidia/utility.hpp | 7 ++ src/hws/gpu_nvidia/hardware_sampler.cpp | 86 +++++++++++++++++++++---- src/hws/gpu_nvidia/utility.cpp | 10 ++- 3 files changed, 89 insertions(+), 14 deletions(-) diff --git a/include/hws/gpu_nvidia/utility.hpp b/include/hws/gpu_nvidia/utility.hpp index b0b3811..7de8cfc 100644 --- a/include/hws/gpu_nvidia/utility.hpp +++ b/include/hws/gpu_nvidia/utility.hpp @@ -69,6 +69,13 @@ namespace hws::detail { #endif +/** + * @brief Return the PCI bus ID (e.g. `"0000:c1:00.0"`) of the NVIDIA GPU device with the given CUDA @p local_index. + * @param[in] local_index the local CUDA device index + * @return the PCI bus ID (`[[nodiscard]]`) + */ +[[nodiscard]] std::string nvidia_device_pci_bus_id(int local_index); + #if defined(HWS_MPI_SUPPORT_ENABLED) /** diff --git a/src/hws/gpu_nvidia/hardware_sampler.cpp b/src/hws/gpu_nvidia/hardware_sampler.cpp index f3c6f53..c0de1c5 100644 --- a/src/hws/gpu_nvidia/hardware_sampler.cpp +++ b/src/hws/gpu_nvidia/hardware_sampler.cpp @@ -22,6 +22,7 @@ #include // std::min_element, std::sort, std::transform #include // std::chrono::{steady_clock, duration_cast, milliseconds} #include // std::size_t +#include // std::uint32_t #include // std::exception, std::terminate #include // std::ios_base #include // std::cerr, std::endl @@ -35,6 +36,51 @@ namespace hws { +namespace { + +/** + * @brief Resolve the NVML device index that corresponds to the physical device CUDA considers index + * @p cuda_device_id, by matching PCI bus IDs. + * @details Necessary because NVML enumerates every physical NVIDIA GPU on the node unconditionally, while CUDA's + * enumeration is filtered/reordered by `CUDA_VISIBLE_DEVICES` - the same index number in both APIs can + * refer to different physical devices. Requires `nvmlInit()` to have already been called. + * @throws std::runtime_error if NVML's device count can't be queried, if none of its devices' PCI bus IDs match + * @p cuda_device_id's, or if more than one does. Note: NVIDIA MIG instances are *not* caught by the + * "more than one" check below - `nvmlDeviceGetCount_v2()`/`nvmlDeviceGetHandleByIndex_v2()` enumerate + * physical parent GPUs only (MIG device handles are a separate, unrelated API surface, + * `nvmlDeviceGetMigDeviceHandleByIndex()`, not used anywhere in this codebase), so a CUDA-visible MIG + * instance matches exactly one parent's PCI bus ID and resolves to that whole parent GPU instead of + * throwing. MIG is not supported/disambiguated by this function. + */ +[[nodiscard]] unsigned int resolve_nvml_device_id(const std::size_t cuda_device_id) { + unsigned int nvml_count{}; + if (nvmlDeviceGetCount_v2(&nvml_count) != NVML_SUCCESS) { + throw std::runtime_error{ "gpu_nvidia_hardware_sampler: couldn't query the number of NVML devices while resolving the physical device for CUDA index " + std::to_string(cuda_device_id) + "!" }; + } + + const std::string cuda_bus_id = detail::nvidia_device_pci_bus_id(static_cast(cuda_device_id)); + std::optional resolved{}; + for (unsigned int nvml_idx = 0; nvml_idx < nvml_count; ++nvml_idx) { + nvmlDevice_t device{}; + nvmlPciInfo_st pcie_info{}; + if (nvmlDeviceGetHandleByIndex_v2(nvml_idx, &device) == NVML_SUCCESS && nvmlDeviceGetPciInfo_v3(device, &pcie_info) == NVML_SUCCESS) { + const std::string nvml_bus_id = detail::format_pci_bus_id(static_cast(pcie_info.domain), static_cast(pcie_info.bus), static_cast(pcie_info.device)); + if (nvml_bus_id == cuda_bus_id) { + if (resolved.has_value()) { + throw std::runtime_error{ "gpu_nvidia_hardware_sampler: found more than one NVML device reporting PCI bus ID " + cuda_bus_id + " (CUDA index " + std::to_string(cuda_device_id) + ")!" }; + } + resolved = nvml_idx; + } + } + } + if (!resolved.has_value()) { + throw std::runtime_error{ "gpu_nvidia_hardware_sampler: couldn't find an NVML device with PCI bus ID " + cuda_bus_id + " (CUDA index " + std::to_string(cuda_device_id) + ")!" }; + } + return resolved.value(); +} + +} // namespace + gpu_nvidia_hardware_sampler::gpu_nvidia_hardware_sampler(const sample_category category) : gpu_nvidia_hardware_sampler{ 0, HWS_SAMPLING_INTERVAL, category } { } @@ -46,18 +92,36 @@ gpu_nvidia_hardware_sampler::gpu_nvidia_hardware_sampler(const std::chrono::mill gpu_nvidia_hardware_sampler::gpu_nvidia_hardware_sampler(const std::size_t device_id, const std::chrono::milliseconds sampling_interval, const sample_category category) : hardware_sampler{ sampling_interval, category } { - // make sure that nvmlInit is only called once for all instances - if (instances_++ == 0) { - HWS_NVML_ERROR_CHECK(nvmlInit()) - // notify that initialization has been finished - init_finished_ = true; - } else { - // wait until init has been finished! - while (!init_finished_) { } + // make sure that nvmlInit is only called once for all instances; nvmlInit() itself is inside the try so that a + // failing first instance is rolled back the exact same way as a failing resolve_nvml_device_id() below, + // instead of leaking instances_/init_finished_ through an early, uncaught throw + const bool is_first_instance = (instances_++ == 0); + try { + if (is_first_instance) { + HWS_NVML_ERROR_CHECK(nvmlInit()) + // notify that initialization has been finished + init_finished_ = true; + } else { + // wait until init has been finished! + while (!init_finished_) { } + } + + // initialize samples -> can't be done beforehand since the device handle can only be initialized after a + // call to nvmlInit; resolve the CUDA-relative device_id to the matching NVML index first (see + // resolve_nvml_device_id()) since NVML's own enumeration isn't affected by CUDA_VISIBLE_DEVICES + device_ = detail::nvml_device_handle{ resolve_nvml_device_id(device_id) }; + } catch (...) { + // mirror the destructor's "last instance out shuts NVML down" logic (keyed off instances_ reaching zero, + // not off is_first_instance) - by the time this runs, another constructor may already have passed the + // init_finished_ gate and be actively using NVML, so only the instance that brings the shared count back + // to zero may touch its lifetime, and only if it was actually initialized (init_finished_) - otherwise + // nvmlInit() itself is what failed and there is nothing to shut down + if (--instances_ == 0 && init_finished_) { + init_finished_ = false; + nvmlShutdown(); + } + throw; } - - // initialize samples -> can't be done beforehand since the device handle can only be initialized after a call to nvmlInit - device_ = detail::nvml_device_handle{ device_id }; } gpu_nvidia_hardware_sampler::~gpu_nvidia_hardware_sampler() { diff --git a/src/hws/gpu_nvidia/utility.cpp b/src/hws/gpu_nvidia/utility.cpp index 97d8c1e..10c2f70 100644 --- a/src/hws/gpu_nvidia/utility.cpp +++ b/src/hws/gpu_nvidia/utility.cpp @@ -60,6 +60,12 @@ std::string throttle_event_reason_to_string(const unsigned long long clocks_even #endif +std::string nvidia_device_pci_bus_id(const int local_index) { + char bus_id[64] = {}; + HWS_CUDA_ERROR_CHECK(cudaDeviceGetPCIBusId(bus_id, sizeof(bus_id), local_index)); + return std::string{ bus_id }; +} + #if defined(HWS_MPI_SUPPORT_ENABLED) && defined(HWS_FOR_NVIDIA_GPUS_ENABLED) namespace { @@ -72,9 +78,7 @@ namespace { * @return the physical ID of the NVIDIA GPU device */ [[nodiscard]] std::string nvidia_physical_id(const int local_index) { - char bus_id[64] = {}; - HWS_CUDA_ERROR_CHECK(cudaDeviceGetPCIBusId(bus_id, sizeof(bus_id), local_index)); - return std::string{ "nvidia:" } + bus_id; + return std::string{ "nvidia:" } + nvidia_device_pci_bus_id(local_index); } } // namespace From 3030cffee138ca625a0e9acb8e6217360e04a30c Mon Sep 17 00:00:00 2001 From: Alexander Van Craen Date: Thu, 13 Aug 2026 11:37:17 +0200 Subject: [PATCH 4/4] fix init/shutdown lifecycle race and deadlock in all three GPU backends gpu_amd_hardware_sampler, gpu_nvidia_hardware_sampler and gpu_intel_hardware_sampler each used an atomic instances_ counter plus an atomic init_finished_ flag to lazily call rsmi_init()/nvmlInit()/zeInit() exactly once, with later constructors busy-waiting on the flag. This has two bugs, both pre-existing (unrelated to and unchanged by the device index resolution fixes elsewhere on this branch): - if the first instance's init call itself throws, init_finished_ never becomes true, and any other constructor already spinning on `while (!init_finished_) {}` blocks forever - the failure is never signaled to waiters. - a destructor decrementing instances_ to zero and beginning rsmi_shut_down()/nvmlShutdown() can race a new constructor concurrently incrementing from zero and calling rsmi_init()/nvmlInit() again - the atomics serialize the counter, not the actual init/shutdown calls. Both are only reachable by constructing multiple samplers of the same backend concurrently from different threads - system_hardware_sampler itself always constructs sequentially - but they're part of the public API surface (nothing stops a caller from doing this directly). Fix: replace the atomic counter/flag pair with a single mutex that's held for the whole "is this the first/last instance?" decision plus the actual init/shutdown call, for all three backends. This makes the decision and the call one atomic step, so: - a failing init call leaves the shared count untouched and the mutex unlocked - the next constructor to acquire it simply retries the init call itself instead of ever spinning on a flag that might never be set. - a shutdown and a concurrent init can no longer interleave, since both hold the same mutex around their respective counter check and call. gpu_intel_hardware_sampler has no equivalent shut down call (Level Zero has none), so it only needed the simpler "has zeInit() ever succeeded" half of this - a single mutex-guarded bool instead of a counter. Verified with a 16-thread x 200-iteration concurrent construct/destruct stress test against the NVIDIA backend on real hardware (2x RTX 3090): 0 failures, no hangs. The AMD and Intel changes are the identical pattern with different underlying API calls, reviewed by inspection but not independently stress-tested on real ROCm/Level-Zero hardware this round. --- include/hws/gpu_amd/hardware_sampler.hpp | 14 ++++--- include/hws/gpu_intel/hardware_sampler.hpp | 14 ++++--- include/hws/gpu_nvidia/hardware_sampler.hpp | 14 ++++--- src/hws/gpu_amd/hardware_sampler.cpp | 40 ++++++++----------- src/hws/gpu_intel/hardware_sampler.cpp | 20 +++++----- src/hws/gpu_nvidia/hardware_sampler.cpp | 44 +++++++++------------ 6 files changed, 74 insertions(+), 72 deletions(-) diff --git a/include/hws/gpu_amd/hardware_sampler.hpp b/include/hws/gpu_amd/hardware_sampler.hpp index 4cdfbcc..3cf5058 100644 --- a/include/hws/gpu_amd/hardware_sampler.hpp +++ b/include/hws/gpu_amd/hardware_sampler.hpp @@ -18,11 +18,11 @@ #include "fmt/ostream.h" // fmt::formatter, fmt::ostream_formatter -#include // std::atomic #include // std::chrono::milliseconds, std::chrono_literals namespace #include // std::size_t #include // std::uint32_t #include // std::ostream forward declaration +#include // std::mutex namespace hws { @@ -150,10 +150,14 @@ class gpu_amd_hardware_sampler : public hardware_sampler { /// The temperature related AMD GPU samples. rocm_smi_temperature_samples temperature_samples_{}; - /// The total number of currently active AMD GPU hardware samplers. - inline static std::atomic instances_{ 0 }; - /// True if the ROCm SMI environment has been successfully initialized (only done by a single hardware sampler). - inline static std::atomic init_finished_{ false }; + /// Guards `instances_` and every `rsmi_init()`/`rsmi_shut_down()` call, so that the "first instance + /// initializes, last instance shuts down" decision and the actual init/shutdown call happen as one atomic + /// step - a busy-wait on a plain flag can't do this: a failing first `rsmi_init()` never sets it, permanently + /// stranding every waiter, and nothing prevents a shutdown from racing a concurrent init. + inline static std::mutex lifecycle_mutex_{}; + /// The total number of currently active AMD GPU hardware samplers; only ever read/written while holding + /// `lifecycle_mutex_`. + inline static int instances_{ 0 }; }; /** diff --git a/include/hws/gpu_intel/hardware_sampler.hpp b/include/hws/gpu_intel/hardware_sampler.hpp index db068fe..2f2b05d 100644 --- a/include/hws/gpu_intel/hardware_sampler.hpp +++ b/include/hws/gpu_intel/hardware_sampler.hpp @@ -19,10 +19,10 @@ #include "fmt/format.h" // fmt::formatter, fmt::ostream_formatter -#include // std::atomic #include // std::chrono::milliseconds, std::chrono_literals namespace #include // std::size_t #include // std::ostream forward declaration +#include // std::mutex #include // std::string namespace hws { @@ -146,10 +146,14 @@ class gpu_intel_hardware_sampler : public hardware_sampler { /// The temperature related Intel GPU samples. level_zero_temperature_samples temperature_samples_{}; - /// The total number of currently active Intel GPU hardware samplers. - inline static std::atomic instances_{ 0 }; - /// True if the Level Zero environment has been successfully initialized (only done by a single hardware sampler). - inline static std::atomic init_finished_{ false }; + /// Guards `initialized_` and the `zeInit()` call, so that the "has anyone already initialized?" check and the + /// actual `zeInit()` call happen as one atomic step - a busy-wait on a plain flag can't do this: a failing + /// first `zeInit()` never sets it, permanently stranding every waiter. There is no matching shut down call + /// (Level Zero has none), so unlike the AMD/NVIDIA backends this only ever needs to run `zeInit()` once + /// successfully, never again for the lifetime of the process. + inline static std::mutex lifecycle_mutex_{}; + /// True once `zeInit()` has completed successfully; only ever read/written while holding `lifecycle_mutex_`. + inline static bool initialized_{ false }; }; /** diff --git a/include/hws/gpu_nvidia/hardware_sampler.hpp b/include/hws/gpu_nvidia/hardware_sampler.hpp index 59a5e31..6aaea42 100644 --- a/include/hws/gpu_nvidia/hardware_sampler.hpp +++ b/include/hws/gpu_nvidia/hardware_sampler.hpp @@ -19,10 +19,10 @@ #include "fmt/format.h" // fmt::formatter, fmt::ostream_formatter -#include // std::atomic #include // std::chrono::milliseconds, std::chrono_literals namespace #include // std::size_t #include // std::ostream forward declaration +#include // std::mutex #include // std::string namespace hws { @@ -147,10 +147,14 @@ class gpu_nvidia_hardware_sampler : public hardware_sampler { /// The temperature related NVIDIA GPU samples. nvml_temperature_samples temperature_samples_{}; - /// The total number of currently active NVIDIA GPU hardware samplers. - inline static std::atomic instances_{ 0 }; - /// True if the NVML environment has been successfully initialized (only done by a single hardware sampler). - inline static std::atomic init_finished_{ false }; + /// Guards `instances_` and every `nvmlInit()`/`nvmlShutdown()` call, so that the "first instance initializes, + /// last instance shuts down" decision and the actual init/shutdown call happen as one atomic step - a + /// busy-wait on a plain flag can't do this: a failing first `nvmlInit()` never sets it, permanently stranding + /// every waiter, and nothing prevents a shutdown from racing a concurrent init. + inline static std::mutex lifecycle_mutex_{}; + /// The total number of currently active NVIDIA GPU hardware samplers; only ever read/written while holding + /// `lifecycle_mutex_`. + inline static int instances_{ 0 }; }; /** diff --git a/src/hws/gpu_amd/hardware_sampler.cpp b/src/hws/gpu_amd/hardware_sampler.cpp index 3672143..566a272 100644 --- a/src/hws/gpu_amd/hardware_sampler.cpp +++ b/src/hws/gpu_amd/hardware_sampler.cpp @@ -103,30 +103,25 @@ gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const std::chrono::millisecon gpu_amd_hardware_sampler::gpu_amd_hardware_sampler(const std::size_t device_id, const std::chrono::milliseconds sampling_interval, const sample_category category) : hardware_sampler{ sampling_interval, category }, hip_device_id_{ static_cast(device_id) } { - // make sure that rsmi_init is only called once for all instances; rsmi_init() itself is inside the try so that - // a failing first instance is rolled back the exact same way as a failing resolve_rsmi_device_id() below, - // instead of leaking instances_/init_finished_ through an early, uncaught throw - const bool is_first_instance = (instances_++ == 0); - try { - if (is_first_instance) { + // make sure that rsmi_init is only called once for all instances; holding lifecycle_mutex_ for the whole + // "am I first?" decision plus the rsmi_init() call itself serializes it against every other constructor and + // destructor, so a failing rsmi_init() can never strand a waiter the way a busy-wait on a flag could - the + // next constructor to acquire the mutex simply sees instances_ still 0 and retries rsmi_init() itself + { + const std::lock_guard lock{ lifecycle_mutex_ }; + if (instances_ == 0) { HWS_ROCM_SMI_ERROR_CHECK(rsmi_init(std::uint64_t{ 0 })) - // notify that initialization has been finished - init_finished_ = true; - } else { - // wait until init has been finished! - while (!init_finished_) { } } + ++instances_; + } - // resolve device_id_ only after rsmi_init() has definitely run (by this instance or a previous one) + // resolve device_id_ only after rsmi_init() has definitely run (by this instance or a previous one, guaranteed + // since we're now a counted instance); if resolution throws, roll the count back under the same mutex + try { device_id_ = resolve_rsmi_device_id(hip_device_id_); } catch (...) { - // mirror the destructor's "last instance out shuts ROCm SMI down" logic (keyed off instances_ reaching - // zero, not off is_first_instance) - by the time this runs, another constructor may already have passed - // the init_finished_ gate and be actively using ROCm SMI, so only the instance that brings the shared - // count back to zero may touch its lifetime, and only if it was actually initialized (init_finished_) - - // otherwise rsmi_init() itself is what failed and there is nothing to shut down - if (--instances_ == 0 && init_finished_) { - init_finished_ = false; + const std::lock_guard lock{ lifecycle_mutex_ }; + if (--instances_ == 0) { rsmi_shut_down(); } throw; @@ -140,12 +135,11 @@ gpu_amd_hardware_sampler::~gpu_amd_hardware_sampler() { this->stop_sampling(); } - // the last instance must shut down the ROCm SMI runtime - // make sure that rsmi_shut_down is only called once + // the last instance must shut down the ROCm SMI runtime; guarded by the same mutex as the constructor so + // this can't race a concurrent constructor's "am I first?" check + const std::lock_guard lock{ lifecycle_mutex_ }; if (--instances_ == 0) { HWS_ROCM_SMI_ERROR_CHECK(rsmi_shut_down()) - // reset init_finished flag - init_finished_ = false; } } catch (const std::exception &e) { std::cerr << e.what() << std::endl; diff --git a/src/hws/gpu_intel/hardware_sampler.cpp b/src/hws/gpu_intel/hardware_sampler.cpp index 2c703e9..8023b65 100644 --- a/src/hws/gpu_intel/hardware_sampler.cpp +++ b/src/hws/gpu_intel/hardware_sampler.cpp @@ -44,17 +44,19 @@ gpu_intel_hardware_sampler::gpu_intel_hardware_sampler(const std::chrono::millis gpu_intel_hardware_sampler::gpu_intel_hardware_sampler(const std::size_t device_id, const std::chrono::milliseconds sampling_interval, const sample_category category) : hardware_sampler{ sampling_interval, category } { - // make sure that zeInit is only called once for all instances - if (instances_++ == 0) { - HWS_LEVEL_ZERO_ERROR_CHECK(zeInit(ZE_INIT_FLAG_GPU_ONLY)) - // notify that initialization has been finished - init_finished_ = true; - } else { - // wait until init has been finished! - while (!init_finished_) { } + // make sure that zeInit is only called once for all instances; holding lifecycle_mutex_ for the whole + // "already initialized?" check plus the zeInit() call itself serializes it against every other constructor, + // so a failing zeInit() can never strand a waiter the way a busy-wait on a flag could - the next constructor + // to acquire the mutex simply sees initialized_ still false and retries zeInit() itself + { + const std::lock_guard lock{ lifecycle_mutex_ }; + if (!initialized_) { + HWS_LEVEL_ZERO_ERROR_CHECK(zeInit(ZE_INIT_FLAG_GPU_ONLY)) + initialized_ = true; + } } - // initialize samples -> can't be done beforehand since the device handle can only be initialized after a call to nvmlInit + // initialize samples -> can't be done beforehand since the device handle can only be initialized after a call to zeInit device_ = detail::level_zero_device_handle{ device_id }; } diff --git a/src/hws/gpu_nvidia/hardware_sampler.cpp b/src/hws/gpu_nvidia/hardware_sampler.cpp index c0de1c5..c406ea9 100644 --- a/src/hws/gpu_nvidia/hardware_sampler.cpp +++ b/src/hws/gpu_nvidia/hardware_sampler.cpp @@ -92,32 +92,27 @@ gpu_nvidia_hardware_sampler::gpu_nvidia_hardware_sampler(const std::chrono::mill gpu_nvidia_hardware_sampler::gpu_nvidia_hardware_sampler(const std::size_t device_id, const std::chrono::milliseconds sampling_interval, const sample_category category) : hardware_sampler{ sampling_interval, category } { - // make sure that nvmlInit is only called once for all instances; nvmlInit() itself is inside the try so that a - // failing first instance is rolled back the exact same way as a failing resolve_nvml_device_id() below, - // instead of leaking instances_/init_finished_ through an early, uncaught throw - const bool is_first_instance = (instances_++ == 0); - try { - if (is_first_instance) { + // make sure that nvmlInit is only called once for all instances; holding lifecycle_mutex_ for the whole + // "am I first?" decision plus the nvmlInit() call itself serializes it against every other constructor and + // destructor, so a failing nvmlInit() can never strand a waiter the way a busy-wait on a flag could - the + // next constructor to acquire the mutex simply sees instances_ still 0 and retries nvmlInit() itself + { + const std::lock_guard lock{ lifecycle_mutex_ }; + if (instances_ == 0) { HWS_NVML_ERROR_CHECK(nvmlInit()) - // notify that initialization has been finished - init_finished_ = true; - } else { - // wait until init has been finished! - while (!init_finished_) { } } + ++instances_; + } - // initialize samples -> can't be done beforehand since the device handle can only be initialized after a - // call to nvmlInit; resolve the CUDA-relative device_id to the matching NVML index first (see - // resolve_nvml_device_id()) since NVML's own enumeration isn't affected by CUDA_VISIBLE_DEVICES + // initialize samples -> can't be done beforehand since the device handle can only be initialized after a call + // to nvmlInit (guaranteed to have already run, since we're now a counted instance); resolve the CUDA-relative + // device_id to the matching NVML index first (see resolve_nvml_device_id()) since NVML's own enumeration + // isn't affected by CUDA_VISIBLE_DEVICES; if resolution throws, roll the count back under the same mutex + try { device_ = detail::nvml_device_handle{ resolve_nvml_device_id(device_id) }; } catch (...) { - // mirror the destructor's "last instance out shuts NVML down" logic (keyed off instances_ reaching zero, - // not off is_first_instance) - by the time this runs, another constructor may already have passed the - // init_finished_ gate and be actively using NVML, so only the instance that brings the shared count back - // to zero may touch its lifetime, and only if it was actually initialized (init_finished_) - otherwise - // nvmlInit() itself is what failed and there is nothing to shut down - if (--instances_ == 0 && init_finished_) { - init_finished_ = false; + const std::lock_guard lock{ lifecycle_mutex_ }; + if (--instances_ == 0) { nvmlShutdown(); } throw; @@ -131,12 +126,11 @@ gpu_nvidia_hardware_sampler::~gpu_nvidia_hardware_sampler() { this->stop_sampling(); } - // the last instance must shut down the NVML runtime - // make sure that nvmlShutdown is only called once + // the last instance must shut down the NVML runtime; guarded by the same mutex as the constructor so this + // can't race a concurrent constructor's "am I first?" check + const std::lock_guard lock{ lifecycle_mutex_ }; if (--instances_ == 0) { HWS_NVML_ERROR_CHECK(nvmlShutdown()) - // reset init_finished flag - init_finished_ = false; } } catch (const std::exception &e) { std::cerr << e.what() << std::endl;