fix(rccl): correct UALoE fabric detection and diagnostics on the amdsmi path

projects/rccl/src/include/amdsmi_wrap.h

@@ -3,8 +3,12 @@
 #ifndef AMDSMI_WRAP_H_
 #define AMDSMI_WRAP_H_
 
+#include <cstddef>
 #include <cstdint>
+#include <cstring>
 #include <ctime>
+#include <type_traits>
+#include <utility>
 
 #include "nccl.h"
 #include "device.h"
@@ -462,6 +466,37 @@ amdsmi_status_t amdsmi_fabric_telem_id_to_string(uint64_t telem_id, const char**
 /** @} End rcclFabricCompat */
 #endif // !AMDSMI_FABRIC_DIRECT
 
+/*************************************************************************
+ * Fabric ABI guard
+ *
+ * amdsmi_get_gpu_fabric_info() is reached through dlopen, so it writes this
+ * struct using the shipped library's layout, not ours. If our declaration is
+ * smaller the library writes past the end of the caller's object, and any
+ * shift moves the fields we read. Neither shows up as a compiler diagnostic,
+ * so recognize the two shipped layouts here: a mismatch must be a deliberate
+ * update, not a silent memory bug. Some pre-release ROCm 7.14 snapshots used
+ * AMDSMI_FABRIC_MAX_LOCAL_GPUS=8, while final ROCm 7.14 and ROCm 7.15 use 16
+ * without changing the amd_smi major.
+ ************************************************************************/
+constexpr size_t kAmdSmiFabricInfo8GpuSize = 288;
+constexpr size_t kAmdSmiFabricInfo16GpuSize = 320;
+constexpr size_t kAmdSmiFabricState8GpuOffset = 208;
+constexpr size_t kAmdSmiFabricState16GpuOffset = 240;
+
+constexpr bool amdSmiFabricLayoutIs8Gpu =
+  sizeof(amdsmi_fabric_info_v1_t) == 212 && sizeof(amdsmi_fabric_info_t) == kAmdSmiFabricInfo8GpuSize &&
+  offsetof(amdsmi_fabric_info_v1_t, addr_mode) == kAmdSmiFabricState8GpuOffset - sizeof(uint32_t) &&
+  offsetof(amdsmi_fabric_info_v1_t, accel_state) == kAmdSmiFabricState8GpuOffset &&
+  offsetof(amdsmi_fabric_info_t, reserved) == 224;
+
+constexpr bool amdSmiFabricLayoutIs16Gpu =
+  sizeof(amdsmi_fabric_info_v1_t) == 244 && sizeof(amdsmi_fabric_info_t) == kAmdSmiFabricInfo16GpuSize &&
+  offsetof(amdsmi_fabric_info_v1_t, addr_mode) == kAmdSmiFabricState16GpuOffset - sizeof(uint32_t) &&
+  offsetof(amdsmi_fabric_info_v1_t, accel_state) == kAmdSmiFabricState16GpuOffset &&
+  offsetof(amdsmi_fabric_info_t, reserved) == 256;
+
+static_assert(amdSmiFabricLayoutIs8Gpu || amdSmiFabricLayoutIs16Gpu, "unsupported amdsmi fabric layout");
+
 /*************************************************************************
  * AMD SMI Fabric Info Cache
  *
@@ -472,6 +507,120 @@ amdsmi_status_t amdsmi_fabric_telem_id_to_string(uint64_t telem_id, const char**
 #define AMDSMI_FABRIC_INFO_VERSION_1 1
 #define AMDSMI_FABRIC_INFO_CURRENT_VERSION AMDSMI_FABRIC_INFO_VERSION_1
 
+// Sentinel amd_smi leaves in numeric fabric fields it could not source from sysfs
+constexpr uint32_t kAmdSmiFabricVersionUnreported = 0xFFFFFFFFu;
+
+// amd_smi never populates amdsmi_fabric_info_t's version field, so an unreported value
+// has to be read as v1 rather than as a reason to discard an otherwise valid payload.
+// A version the library does claim, but that we cannot interpret, is still refused.
+inline bool amdSmiFabricVersionUsable(uint32_t version) {
+  return version == kAmdSmiFabricVersionUnreported || version == AMDSMI_FABRIC_INFO_CURRENT_VERSION;
+}
+
+// Fabric is usable only over a link type we drive, and only once the accelerator has
+// reached a state where its vPoD can carry traffic.
+//
+// AMDSMI_FABRIC_TYPE_UALLINK is deprecated in favour of the correctly spelled
+// AMDSMI_FABRIC_TYPE_UALINK, but only the former exists in amd_smi before 27.0 and
+// the latter is an alias for it after, so the old name is the one that compiles
+// against every header we support.
+inline bool amdSmiFabricStateUsable(amdsmi_fabric_type_t type, amdsmi_fabric_accelerator_vpod_state_t state) {
+  return (type == AMDSMI_FABRIC_TYPE_UALOE || type == AMDSMI_FABRIC_TYPE_UALLINK) &&
+         (state == AMDSMI_FABRIC_ACCELERATOR_VPOD_STATE_ACTIVE || state == AMDSMI_FABRIC_ACCELERATOR_VPOD_STATE_READY);
+}
+
+/*************************************************************************
+ * amdsmi_fabric_info_t layout compatibility
+ *
+ * amd_smi 27.0 flattened this struct. The version moved from a nested
+ * fabric_info.version to a top-level fabric_version, and the payload from
+ * fabric_info.fabric_version.v1 to fabric_info.v1 -- note that fabric_version
+ * names different things either side of the change. The version and payload
+ * offsets did not move (bytes 8 and 12), so the shape change itself is only a
+ * source-level rename; the ABI guard above handles payload capacity separately.
+ *
+ * Detect the shape rather than test a version macro, so a header that carries
+ * the change under any version still resolves correctly. Only the flattened
+ * layout has a top-level integral fabric_version; in the nested layout that name
+ * exists solely inside fabric_info, as a union.
+ ************************************************************************/
+template <typename T, typename = void>
+struct amdSmiFabricInfoIsFlat : std::false_type {};
+
+template <typename T>
+struct amdSmiFabricInfoIsFlat<T, std::void_t<decltype(std::declval<const T&>().fabric_version)>>
+  : std::is_integral<std::decay_t<decltype(std::declval<const T&>().fabric_version)>> {};
+
+// Templates so that if constexpr discards the branch that does not compile against
+// the header in use; in a non-template function both branches are type-checked.
+template <typename FabricInfoT>
+inline uint32_t amdSmiFabricInfoVersion(const FabricInfoT& info) {
+  if constexpr (amdSmiFabricInfoIsFlat<FabricInfoT>::value) {
+    return info.fabric_version;
+  } else {
+    return info.fabric_info.version;
+  }
+}
+
+template <typename FabricInfoT>
+inline const amdsmi_fabric_info_v1_t* amdSmiFabricInfoV1(const FabricInfoT& info) {
+  if constexpr (amdSmiFabricInfoIsFlat<FabricInfoT>::value) {
+    return &info.fabric_info.v1;
+  } else {
+    return &info.fabric_info.fabric_version.v1;
+  }
+}
+
+// amd_smi 26.x used both supported payload sizes under the same SONAME. Use a
+// maximum-sized canary buffer so either runtime can write safely, then identify
+// how much it wrote. Both implementations value-initialize a local struct and
+// assign the complete object, making the final 32 bytes zero only for the
+// 16-GPU runtime.
+constexpr unsigned char kAmdSmiFabricBufferCanary = 0xA5;
+
+struct alignas(amdsmi_fabric_info_t) amdSmiFabricInfoBuffer {
+  unsigned char bytes[kAmdSmiFabricInfo16GpuSize];
+};
+
+enum class amdSmiFabricRuntimeLayout {
+  EightGpu,
+  SixteenGpu,
+  Unknown,
+};
+
+inline void amdSmiPrepareFabricInfoBuffer(amdSmiFabricInfoBuffer& buffer) {
+  memset(buffer.bytes, kAmdSmiFabricBufferCanary, sizeof(buffer.bytes));
+}
+
+inline amdsmi_fabric_info_t* amdSmiFabricInfoBufferAsInfo(amdSmiFabricInfoBuffer& buffer) {
+  return reinterpret_cast<amdsmi_fabric_info_t*>(buffer.bytes);
+}
+
+inline amdSmiFabricRuntimeLayout amdSmiDetectFabricRuntimeLayout(const amdSmiFabricInfoBuffer& buffer) {
+  bool tailIsCanary = true;
+  bool tailIsZero = true;
+  for (size_t i = kAmdSmiFabricInfo8GpuSize; i < kAmdSmiFabricInfo16GpuSize; ++i) {
+    tailIsCanary &= buffer.bytes[i] == kAmdSmiFabricBufferCanary;
+    tailIsZero &= buffer.bytes[i] == 0;
+  }
+  if (tailIsCanary) return amdSmiFabricRuntimeLayout::EightGpu;
+  if (tailIsZero) return amdSmiFabricRuntimeLayout::SixteenGpu;
+  return amdSmiFabricRuntimeLayout::Unknown;
+}
+
+// amd_smi 27.0 also changed amdsmi_fabric_telem_id_to_string from returning the name
+// to writing it through an out-parameter. The two are not call-compatible and we reach
+// it through dlopen, so the choice has to follow the version the loaded runtime
+// reports. An unknown version must assume the out-parameter form: calling a 27+
+// implementation the old way leaves it writing through an uninitialized pointer,
+// whereas calling an older one the new way merely leaves the name unwritten.
+constexpr uint32_t kAmdSmiLibVersionUnknown = 0;
+constexpr uint32_t kAmdSmiTelemOutParamMajor = 27;
+
+inline bool amdSmiTelemIdUsesOutParam(uint32_t libMajor) {
+  return libMajor == kAmdSmiLibVersionUnknown || libMajor >= kAmdSmiTelemOutParamMajor;
+}
+
 struct amdsmiFabricDeviceInfo {
   bool fabricSupported; //!< Whether UALoE fabric is available
   amdsmi_fabric_type_t fabricType; //!< Fabric type (UALOE or UALLINK)

projects/rccl/src/misc/amdsmi_wrap.cc

@@ -132,16 +132,25 @@ std::mutex fabricLock; // Thread safety for fabric operations
 bool fabricInitialized = false;
 thread_local bool threadFabricInitialized = false;
 ncclResult_t fabricInitResult = ncclSuccess;
+std::mutex amdSmiInitLock;
 ncclResult_t amdSmiInitResult = ncclSuccess;
-std::atomic<bool> amdSmiInitCalled{false}; // Track if amd_smi_init has been called
+bool amdSmiInitCalled = false;
+bool amdSmiLibInitialized = false;
+
+// Major version of the loaded amd_smi, as it reports itself. Selects the telemetry
+// name call convention; see amdSmiTelemIdUsesOutParam().
+std::atomic<uint32_t> amdSmiLibMajor{kAmdSmiLibVersionUnknown};
+
+// Signature amdsmi_fabric_telem_id_to_string had before amd_smi 27.0
+using AmdSmiTelemIdToStringLegacyFn = const char* (*)(uint64_t);
 } // namespace
 
 /*************************************************************************
  * Helper: Get processor handle for a device index
  ************************************************************************/
 static ncclResult_t getProcessorHandle(uint32_t deviceIndex, amdsmi_processor_handle* procHandle) {
   if (!rcclParamUseAmdSmiLib()) {
-    return ncclSystemError; // Fabric only supported with amd_smi_lib
+    return ncclSystemError; // processor handles are amd_smi_lib-only
   }
 
   uint32_t socket_count = 0;
@@ -179,10 +188,7 @@ static bool amd_smi_FabricFunctionsLoaded() {
  * Existing AMD SMI Wrapper Functions
  ************************************************************************/
 
-ncclResult_t amd_smi_init() {
-  // Ensure we only initialize once
-  if (amdSmiInitCalled.exchange(true)) return amdSmiInitResult;
-
+static ncclResult_t amd_smi_init_impl() {
   if (__atomic_load_n(&is_wsl2, __ATOMIC_ACQUIRE) == -1)
     __atomic_store_n(&is_wsl2, (access("/dev/dxg", F_OK) == -1) ? 0 : 1, __ATOMIC_RELEASE);
   if (__atomic_load_n(&is_wsl2, __ATOMIC_ACQUIRE)) {
@@ -197,7 +203,6 @@ ncclResult_t amd_smi_init() {
       static void* libhandle = dlopen(RCCL_AMDSMI_LIBNAME, RTLD_NOW);
       if (libhandle == nullptr) {
         WARN("Failed to open %s: %s", RCCL_AMDSMI_LIBNAME, dlerror());
-        amdSmiInitResult = ncclInternalError;
         return ncclInternalError;
       }
 
@@ -235,32 +240,51 @@ ncclResult_t amd_smi_init() {
 
     // initialize amd-smi for AMD GPUs
     AMDSMITRY(amdsmi_init, AMDSMI_INIT_AMD_GPUS);
+    amdSmiLibInitialized = true;
 
     // get amd-smi version
     amdsmi_version_t version;
     AMDSMITRY(amdsmi_get_lib_version, &version);
-    INFO(NCCL_INIT, "amdsmi_lib: version %d.%d.%d.%s", version.major, version.minor, version.release, version.build);
+    INFO(NCCL_INIT, "amdsmi_lib: version %u.%u.%u (build %s)", version.major, version.minor, version.release,
+         version.build);
+    // Retained because the telemetry name call convention depends on it
+    amdSmiLibMajor.store(version.major, std::memory_order_release);
   } else {
-#ifdef HIP_FABRIC_API
-    WARN("RCCL_USE_AMD_SMI_LIB not set, but HIP_FABRIC_API is defined. Fabric support is only available through AMD "
-         "SMI. Rerun with RCCL_USE_AMD_SMI_LIB=1 to enable AMD SMI and UALoE fabric support.");
-#endif
     // initialize alternate rsmi
     ARSMICHECK(ARSMI_init());
-    INFO(NCCL_INIT, "initialized internal alternative rsmi functionality");
+    // RCCL_USE_AMD_SMI_LIB only selects who performs fabric *discovery*: amd_smi_lib, or the
+    // ualink sysfs nodes via ARSMI_get_fabric_info(). Both populate amdsmiFabricDevices
+    // identically, so UALoE/UALLink works on this path.
+    INFO(NCCL_INIT, "initialized internal alternative rsmi functionality; UALoE/UALLink fabric discovery uses sysfs "
+                    "(set RCCL_USE_AMD_SMI_LIB=1 to use amd_smi_lib instead)");
   }
   return ncclSuccess;
 }
 
+ncclResult_t amd_smi_init() {
+  std::lock_guard<std::mutex> lock(amdSmiInitLock);
+  if (amdSmiInitCalled) return amdSmiInitResult;
+
+  // Cache failures too; shutdown explicitly resets this state for a retry.
+  amdSmiInitResult = amd_smi_init_impl();
+  amdSmiInitCalled = true;
+  return amdSmiInitResult;
+}
+
 ncclResult_t amd_smi_shutdown() {
-  // Only shutdown if we actually initialized with amd_smi_lib
-  if (!rcclParamUseAmdSmiLib() || pfn_amdsmi_shut_down == nullptr) {
-    return ncclSuccess;
+  std::lock_guard<std::mutex> lock(amdSmiInitLock);
+
+  if (!amdSmiInitCalled) return ncclSuccess;
+
+  // The backend may be initialized even if a later version query failed.
+  if (amdSmiLibInitialized) {
+    AMDSMITRY(amdsmi_shut_down);
+    amdSmiLibInitialized = false;
   }
-  amdSmiInitCalled.store(false);
-  amdSmiInitResult = ncclSuccess;
 
-  AMDSMITRY(amdsmi_shut_down);
+  amdSmiLibMajor.store(kAmdSmiLibVersionUnknown, std::memory_order_release);
+  amdSmiInitResult = ncclSuccess;
+  amdSmiInitCalled = false;
   return ncclSuccess;
 }
 
@@ -602,6 +626,30 @@ ncclResult_t amd_smi_ensureFabricInitialized() {
     fabricInitResult = ncclInternalError;
     return fabricInitResult;
   }
+
+  // amd_smi 26.x shipped both fabric payload sizes under the same SONAME, so
+  // the library version cannot identify a mixed header/runtime installation.
+  // Probe through an oversized canary buffer before using the typed structure.
+  // If the layouts differ (or cannot be identified), use the sysfs backend.
+  if (!useSysfs && amd_smi_FabricFunctionsLoaded()) {
+    amdsmi_processor_handle probeHandle;
+    if (getProcessorHandle(0, &probeHandle) == ncclSuccess) {
+      amdSmiFabricInfoBuffer probeBuffer;
+      amdSmiPrepareFabricInfoBuffer(probeBuffer);
+      pfn_amdsmi_get_gpu_fabric_info(probeHandle, amdSmiFabricInfoBufferAsInfo(probeBuffer));
+      const amdSmiFabricRuntimeLayout runtimeLayout = amdSmiDetectFabricRuntimeLayout(probeBuffer);
+      const bool runtimeLayoutIs8Gpu = runtimeLayout == amdSmiFabricRuntimeLayout::EightGpu;
+      if (runtimeLayout == amdSmiFabricRuntimeLayout::Unknown) {
+        WARN("AMD SMI fabric: unable to verify the loaded library's fabric ABI; falling back to sysfs");
+        useSysfs = true;
+      } else if (runtimeLayoutIs8Gpu != amdSmiFabricLayoutIs8Gpu) {
+        WARN("AMD SMI fabric ABI mismatch: RCCL was built for the %u-GPU layout, but the loaded library uses the "
+             "%u-GPU layout; falling back to sysfs",
+             amdSmiFabricLayoutIs8Gpu ? 8 : 16, runtimeLayoutIs8Gpu ? 8 : 16);
+        useSysfs = true;
+      }
+    }
+  }
   amdsmiFabricDeviceCount = (int)numDevs;
 
   for (uint32_t d = 0; d < numDevs; d++) {
@@ -640,17 +688,15 @@ ncclResult_t amd_smi_ensureFabricInitialized() {
         devInfo->fabricSupported = false;
         continue;
       }
-      if (fabricInfo.fabric_version != AMDSMI_FABRIC_INFO_CURRENT_VERSION) {
-        WARN("AMD SMI fabric: unexpected fabric info version %u for device %u, expected %u",
-             fabricInfo.fabric_version, d, AMDSMI_FABRIC_INFO_CURRENT_VERSION);
+      const uint32_t fabricVersion = amdSmiFabricInfoVersion(fabricInfo);
+      if (!amdSmiFabricVersionUsable(fabricVersion)) {
+        WARN("AMD SMI fabric: unexpected fabric info version %u for device %u, expected %u", fabricVersion, d,
+             AMDSMI_FABRIC_INFO_CURRENT_VERSION);
         devInfo->fabricSupported = false;
         continue;
       }
-      const amdsmi_fabric_info_v1_t* v1 = &fabricInfo.fabric_info.v1;
-      devInfo->fabricSupported =
-        ((v1->fabric_type == AMDSMI_FABRIC_TYPE_UALOE || v1->fabric_type == AMDSMI_FABRIC_TYPE_UALLINK) &&
-         (v1->accel_state == AMDSMI_FABRIC_ACCELERATOR_VPOD_STATE_ACTIVE ||
-          v1->accel_state == AMDSMI_FABRIC_ACCELERATOR_VPOD_STATE_READY));
+      const amdsmi_fabric_info_v1_t* v1 = amdSmiFabricInfoV1(fabricInfo);
+      devInfo->fabricSupported = amdSmiFabricStateUsable(v1->fabric_type, v1->accel_state);
       devInfo->fabricType = v1->fabric_type;
       devInfo->state = v1->accel_state;
       devInfo->acceleratorId = v1->accelerator_id;
@@ -762,6 +808,13 @@ const char* amd_smi_fabricTelemIdToString(uint64_t telemId) {
   if (pfn_amdsmi_fabric_telem_id_to_string == nullptr) {
     return ARSMI_fabric_telem_id_to_string(telemId);
   }
+  if (!amdSmiTelemIdUsesOutParam(amdSmiLibMajor.load(std::memory_order_acquire))) {
+    // dlsym gave us an address, not a prototype: on a pre-27 runtime the symbol has
+    // to be called through its own signature or the extra argument is read as one.
+    auto legacyFn = reinterpret_cast<AmdSmiTelemIdToStringLegacyFn>(pfn_amdsmi_fabric_telem_id_to_string);
+    const char* legacyName = legacyFn(telemId);
+    return legacyName == nullptr ? "UNKNOWN" : legacyName;
+  }
   const char* telemName = nullptr;
   amdsmi_status_t ret = pfn_amdsmi_fabric_telem_id_to_string(telemId, &telemName);
   if (ret != AMDSMI_STATUS_SUCCESS || telemName == nullptr) {