diff --git a/src/client/Topology.hpp b/src/client/Topology.hpp index 1f5501eb..4c4441f3 100644 --- a/src/client/Topology.hpp +++ b/src/client/Topology.hpp @@ -52,16 +52,21 @@ static void PrintNicToGPUTopo(bool outputToCsv) int numGpus = TransferBench::GetNumExecutors(EXE_GPU_GFX); auto const& ibvDeviceList = GetIbvDeviceList(); - for (int i = 0; i < ibvDeviceList.size(); i++) { - std::string closestGpusStr = ""; - for (int j = 0; j < numGpus; j++) { - if (TransferBench::GetClosestNicToGpu(j) == i) { - if (closestGpusStr != "") closestGpusStr += ","; - closestGpusStr += std::to_string(j); - } + // Build inverse map: for each NIC, which GPUs list it as closest? + std::vector closestGpusForNic(ibvDeviceList.size(), ""); + for (int j = 0; j < numGpus; j++) { + std::vector nicsForGpu; + TransferBench::GetClosestNicsToGpu(nicsForGpu, j); + for (int nicIdx : nicsForGpu) { + if (nicIdx < 0 || nicIdx >= (int)closestGpusForNic.size()) continue; + if (!closestGpusForNic[nicIdx].empty()) closestGpusForNic[nicIdx] += ","; + closestGpusForNic[nicIdx] += std::to_string(j); } + } + for (int i = 0; i < ibvDeviceList.size(); i++) { + std::string closestGpusStr = closestGpusForNic[i]; printf(" %-3d | %-11s | %-6s | %-12s | %-4d | %-14s | %-9s | %-20s\n", i, ibvDeviceList[i].name.c_str(), ibvDeviceList[i].hasActivePort ? "Yes" : "No", @@ -191,13 +196,23 @@ void DisplaySingleRankTopology(bool outputToCsv) char pciBusId[20]; HIP_CALL(hipDeviceGetPCIBusId(pciBusId, 20, i)); - printf(" %-11s %c %-4d %c %-4d %c %-4d %c %-4d %c %-4d\n", + // Space-separated so the field stays a single column when sep is a comma (CSV mode), + // matching how the "Closest GPU(s)" column above is emitted + std::vector nicsForGpu; + TransferBench::GetClosestNicsToGpu(nicsForGpu, i); + std::string nicStr; + for (int nicIdx : nicsForGpu) { + if (!nicStr.empty()) nicStr += ' '; + nicStr += std::to_string(nicIdx); + } + if (nicStr.empty()) nicStr = "-1"; + printf(" %-11s %c %-4d %c %-4d %c %-4d %c %-4d %c %s\n", pciBusId, sep, TransferBench::GetNumSubExecutors({EXE_GPU_GFX, i}), sep, TransferBench::GetClosestCpuNumaToGpu(i), sep, TransferBench::GetNumExecutorSubIndices({EXE_GPU_DMA, i}), sep, TransferBench::GetNumExecutorSubIndices({EXE_GPU_GFX, i}), sep, - TransferBench::GetClosestNicToGpu(i)); + nicStr.c_str()); } } #endif diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index 259fc4dc..1736d016 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -3242,8 +3242,11 @@ const auto& AmdSmiFabricInfoV1(const T& info) return -1; } - // Function to extract the bus number from a PCIe address (domain:bus:device.function) - static int ExtractBusNumber(std::string const& pcieAddress) + // Function to extract the domain number from a PCIe address (domain:bus:device.function) + // All four fields are read (not just the domain) so that an address with too few fields, or + // with a non-hex field, is rejected. The separator characters themselves are consumed but + // not checked, so this is a well-formedness guard rather than strict format validation + static int ExtractDomain(std::string const& pcieAddress) { int domain, bus, device, function; char delimiter; @@ -3256,16 +3259,31 @@ const auto& AmdSmiFabricInfoV1(const T& info) #endif return -1; } - return bus; - } - - // Function to compute the distance between two bus IDs - static int GetBusIdDistance(std::string const& pcieAddress1, - std::string const& pcieAddress2) - { - int bus1 = ExtractBusNumber(pcieAddress1); - int bus2 = ExtractBusNumber(pcieAddress2); - return (bus1 < 0 || bus2 < 0) ? -1 : std::abs(bus1 - bus2); + return domain; + } + + // Computes a proximity distance between two PCIe addresses. Used as a secondary + // tiebreaker when candidates share the same LCA depth in the PCIe tree, and as the + // sole metric in the fallback path when the PCIe tree yields no match at all. + // Returns -1 if either address cannot be parsed. + // + // Same domain (0): devices in one PCIe domain share a root complex, so the LCA tree + // already captures their structural proximity. Bus numbers are firmware-assigned and + // do not reliably track physical closeness, so they are deliberately NOT used to + // discriminate within a domain -- doing so would break ties between NICs that are + // genuinely equidistant from a GPU (e.g. two NICs hanging off the same root complex + // at different bus numbers), which is exactly the case this metric must preserve. + // + // Cross domain (|delta domain|): any non-zero value ranks behind every same-domain + // candidate. The magnitude only provides a deterministic ordering among cross-domain + // candidates; it carries no physical meaning. + static int GetDomainDistance(std::string const& pcieAddress1, + std::string const& pcieAddress2) + { + int domain1 = ExtractDomain(pcieAddress1); + int domain2 = ExtractDomain(pcieAddress2); + if (domain1 < 0 || domain2 < 0) return -1; + return std::abs(domain1 - domain2); } // Given a target busID and a set of candidate devices, returns a set of indices @@ -3285,11 +3303,15 @@ const auto& AmdSmiFabricInfoV1(const T& info) if (!lca) continue; int depth = GetLcaDepth(lca->address, GetPCIeTreeRoot()); - int currDistance = GetBusIdDistance(targetBusId, candidateBusId); + int currDistance = GetDomainDistance(targetBusId, candidateBusId); + + // A candidate whose address could not be parsed (-1) remains eligible, but treat its + // distance as the largest possible so it can never outrank a candidate whose distance + // is actually known. It can still be selected when nothing else matches at this depth, + // and still ties with other unparseable candidates. + if (currDistance < 0) currDistance = std::numeric_limits::max(); - // When more than one LCA match is found, choose the one with smallest busId difference - // NOTE: currDistance could be -1, which signals problem with parsing, however still - // remains a valid "closest" candidate, so is included + // When more than one LCA match is found, choose the one with smallest domain difference if (depth > maxDepth || (depth == maxDepth && depth >= 0 && currDistance < minDistance)) { maxDepth = depth; matches.clear(); @@ -7482,19 +7504,6 @@ const auto& AmdSmiFabricInfoV1(const T& info) for (auto const& ibvDevice : ibvDeviceList) ibvAddressList.push_back(ibvDevice.hasActivePort ? ibvDevice.busId : ""); - // Track how many times a device has been assigned as "closest" - // This allows distributed work across devices using multiple ports (sharing the same busID) - // NOTE: This isn't necessarily optimal, but likely to work in most cases involving multi-port - // Counter example: - // - // G0 prefers (N0,N1), picks N0 - // G1 prefers (N1,N2), picks N1 - // G2 prefers N0, picks N0 - // - // instead of G0->N1, G1->N2, G2->N0 - - std::vector assignedCount(ibvDeviceList.size(), 0); - // Loop over each GPU to find the closest NIC(s) based on PCIe address for (int gpuIndex = 0; gpuIndex < numGpus; gpuIndex++) { if (gpuAddressList[gpuIndex].empty()) continue; @@ -7503,34 +7512,31 @@ const auto& AmdSmiFabricInfoV1(const T& info) // Find closest NICs std::set closestNicIdxs = GetNearestDevicesInTree(hipPciBusId, ibvAddressList); - // Pick the least-used NIC to assign as closest - int closestIdx = -1; - for (auto idx : closestNicIdxs) { - if (closestIdx == -1 || assignedCount[idx] < assignedCount[closestIdx]) - closestIdx = idx; - } - - // The following will only use distance between bus IDs + // The following will only use distance between PCIe domains // to determine the closest NIC to GPU if the PCIe tree approach fails - if (closestIdx < 0) { + if (closestNicIdxs.empty()) { #ifdef VERBS_DEBUG - Log("[WARN] Falling back to PCIe bus ID distance to determine proximity\n"); + Log("[WARN] Falling back to PCIe domain distance to determine proximity\n"); #endif int minDistance = std::numeric_limits::max(); for (int nicIndex = 0; nicIndex < numNics; nicIndex++) { - if (ibvDeviceList[nicIndex].busId != "") { - int distance = GetBusIdDistance(hipPciBusId, ibvDeviceList[nicIndex].busId); - if (distance < minDistance && distance >= 0) { + // Use ibvAddressList rather than the raw device list: it is already blanked out + // for NICs without an active port, so this stays consistent with the tree path + // above and never maps a GPU to a NIC that cannot execute a Transfer + if (ibvAddressList[nicIndex] != "") { + int distance = GetDomainDistance(hipPciBusId, ibvAddressList[nicIndex]); + if (distance >= 0 && distance < minDistance) { minDistance = distance; - closestIdx = nicIndex; + closestNicIdxs.clear(); + closestNicIdxs.insert(nicIndex); + } else if (distance >= 0 && distance == minDistance) { + closestNicIdxs.insert(nicIndex); } } } } - if (closestIdx != -1) { - topo.closestNicsToGpu[gpuIndex].push_back(closestIdx); - assignedCount[closestIdx]++; - } + for (auto idx : closestNicIdxs) + topo.closestNicsToGpu[gpuIndex].push_back(idx); } // Compute the reverse mapping: closest GPU(s) for each NIC @@ -7544,28 +7550,22 @@ const auto& AmdSmiFabricInfoV1(const T& info) std::set closestGpuIdxs = GetNearestDevicesInTree(ibvDeviceList[nicIndex].busId, gpuAddressList); if (closestGpuIdxs.empty()) { - // Fallback: use bus ID distance + // Fallback: use PCIe domain distance int minDistance = std::numeric_limits::max(); - int closestIdx = -1; - for (int gpuIdx = 0; gpuIdx < numGpus; gpuIdx++) { if (gpuAddressList[gpuIdx].empty()) continue; - - int distance = GetBusIdDistance(ibvDeviceList[nicIndex].busId, gpuAddressList[gpuIdx]); + int distance = GetDomainDistance(ibvDeviceList[nicIndex].busId, gpuAddressList[gpuIdx]); if (distance >= 0 && distance < minDistance) { minDistance = distance; - closestIdx = gpuIdx; + closestGpuIdxs.clear(); + closestGpuIdxs.insert(gpuIdx); + } else if (distance >= 0 && distance == minDistance) { + closestGpuIdxs.insert(gpuIdx); } } - - if (closestIdx != -1) { - topo.closestGpusToNic[nicIndex].push_back(closestIdx); - } - } else { - // Store all GPUs that are equally close - for (int idx : closestGpuIdxs) { - topo.closestGpusToNic[nicIndex].push_back(idx); - } + } + for (int idx : closestGpuIdxs) { + topo.closestGpusToNic[nicIndex].push_back(idx); } } }