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

projects/rccl/src/include/amdsmi_wrap.h

@@ -3,8 +3,11 @@
 #ifndef AMDSMI_WRAP_H_
 #define AMDSMI_WRAP_H_
 
+#include <cstddef>
 #include <cstdint>
 #include <ctime>
+#include <type_traits>
+#include <utility>
 
 #include "nccl.h"
 #include "device.h"
@@ -462,6 +465,20 @@ 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 pin the layout here: a mismatch must be a deliberate update, not a
+ * silent memory bug. Values measured against amdsmi 26.5.0 (ROCm 7.15).
+ ************************************************************************/
+static_assert(sizeof(amdsmi_fabric_info_v1_t) == 244, "amdsmi fabric v1 payload layout changed");
+static_assert(sizeof(amdsmi_fabric_info_t) == 320, "amdsmi fabric info struct size changed");
+static_assert(offsetof(amdsmi_fabric_info_t, reserved) == 256, "amdsmi fabric payload region moved");
+
 /*************************************************************************
  * AMD SMI Fabric Info Cache
  *
@@ -472,6 +489,83 @@ 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. Field offsets did not move
+ * (version at byte 8, v1 at 12), so this is a source-level rename only and the
+ * ABI guard above holds for both.
+ *
+ * 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 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

@@ -134,14 +134,21 @@ thread_local bool threadFabricInitialized = false;
 ncclResult_t fabricInitResult = ncclSuccess;
 ncclResult_t amdSmiInitResult = ncclSuccess;
 std::atomic<bool> amdSmiInitCalled{false}; // Track if amd_smi_init has been called
+
+// 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;
@@ -239,15 +246,18 @@ ncclResult_t amd_smi_init() {
     // 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;
 }
@@ -640,17 +650,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 +770,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) {