From 7933817e7b61767f059312a2a96e77c015e1264a Mon Sep 17 00:00:00 2001 From: paklui <5041261+paklui@users.noreply.github.com> Date: Fri, 19 Jun 2026 18:43:16 -0700 Subject: [PATCH 1/6] include inactive NICs in PCIe proximity tree remove hasActivePort from PCIe tree insertion and ibvAddressList because proximity relates to hardware property, for inactive NICs that are still physically close to their GPU and should be included for correct topology mapping --- src/header/TransferBench.hpp | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index fa536155..1ba816d6 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -3128,7 +3128,15 @@ static bool IsConfiguredGid(union ibv_gid const& gid) // Add NICs to the tree auto const& ibvDeviceList = GetIbvDeviceList(); for (IbvDevice const& ibvDevice : ibvDeviceList) { - if (!ibvDevice.hasActivePort || ibvDevice.busId == "") continue; + // Include all NICs in the PCIe proximity tree regardless of link state. + // PCIe proximity is a hardware property determined by physical wiring through + // root complexes and PCIe switches -- it does not depend on whether a port is + // currently active. Excluding inactive NICs would cause GPUs that share the + // same root complex with them to find no LCA match, falling back to a coarser + // distance heuristic and potentially selecting a more distant NIC instead. + // hasActivePort is enforced separately: it is stored in topo.nicIsActive and + // checked via NicIsActive() during transfer validation, before any QP is set up. + if (ibvDevice.busId == "") continue; InsertPCIePathToTree(ibvDevice.busId, ibvDevice.name, pcieRoot); } From d12537cf284fca753f7c7f5b09a75ccff7a92462 Mon Sep 17 00:00:00 2001 From: paklui <5041261+paklui@users.noreply.github.com> Date: Fri, 19 Jun 2026 18:45:58 -0700 Subject: [PATCH 2/6] use PCIe domain number for NIC-GPU distance metric rename ExtractBusNumber to ExtractDomainAndBus and update GetBusIdDistance to return 0 for same-domain pairs and abs(domain_diff)*256 for cross-domain pairs each GPU occupies its own PCIe domain the old bus-number-only metric produced wrong distances when GPUs and NICs share a domain but sit under different root complexes --- src/header/TransferBench.hpp | 61 ++++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index 1ba816d6..063917e6 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -3214,18 +3214,36 @@ static bool IsConfiguredGid(union ibv_gid const& gid) #ifdef VERBS_DEBUG System::Get().Log("Invalid PCIe address format: %s\n", pcieAddress.c_str()); #endif - return -1; - } - return bus; - } - - // Function to compute the distance between two bus IDs + return {-1, -1}; + } + return {domain, bus}; + } + + // Compute a proximity distance between two PCIe addresses, used as a secondary + // tiebreaker when candidates share the same LCA depth in the PCIe tree. + // + // Same domain (returns 0): within one PCIe domain all devices share a root + // complex, so the LCA tree already captures their true structural proximity. + // Bus numbers within a domain are firmware-assigned and do not reliably reflect + // physical closeness, so they are intentionally not used here (bus is extracted + // by ExtractDomainAndBus but unused in this function). + // + // Cross domain (returns |delta_domain| * 256): a PCIe domain holds at most 256 + // bus numbers (0x00-0xFF), so scaling by 256 guarantees any cross-domain + // distance exceeds the maximum possible same-domain bus difference of 255. + // This creates a hard separation: a same-domain device is always ranked closer + // than any cross-domain device, regardless of individual bus numbers. + // + // On platforms where all devices share domain 0000 (standard x86), all + // distances collapse to 0 and the LCA tree is the sole proximity discriminator. 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); + auto [domain1, bus1] = ExtractDomainAndBus(pcieAddress1); + auto [domain2, bus2] = ExtractDomainAndBus(pcieAddress2); + if (domain1 < 0 || domain2 < 0) return -1; + if (domain1 == domain2) return 0; + return std::abs(domain1 - domain2) * 256; } // Given a target busID and a set of candidate devices, returns a set of indices @@ -3247,14 +3265,16 @@ static bool IsConfiguredGid(union ibv_gid const& gid) int depth = GetLcaDepth(lca->address, GetPCIeTreeRoot()); int currDistance = GetBusIdDistance(targetBusId, candidateBusId); - // 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 if (depth > maxDepth || (depth == maxDepth && depth >= 0 && currDistance < minDistance)) { maxDepth = depth; + // minDistance must be updated before matches.clear() so that any subsequent + // candidate at the same depth and distance correctly passes the == minDistance + // tie-check below. With the old order (clear then update), a candidate + // arriving immediately after a depth-improving entry would compare against the + // stale minDistance from the previous depth level and be incorrectly excluded. + minDistance = currDistance; matches.clear(); matches.insert(i); - minDistance = currDistance; } else if (depth == maxDepth && depth >= 0 && currDistance == minDistance) { matches.insert(i); } @@ -7403,20 +7423,7 @@ static bool IsConfiguredGid(union ibv_gid const& gid) std::vector ibvAddressList; auto const& ibvDeviceList = GetIbvDeviceList(); 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); + ibvAddressList.push_back(ibvDevice.busId); // Loop over each GPU to find the closest NIC(s) based on PCIe address for (int gpuIndex = 0; gpuIndex < numGpus; gpuIndex++) { From afd3e659c8f5c4a02dda49e06391535aa90a1e17 Mon Sep 17 00:00:00 2001 From: paklui <5041261+paklui@users.noreply.github.com> Date: Fri, 19 Jun 2026 18:48:00 -0700 Subject: [PATCH 3/6] map all equally-close NICs per GPU in both directions Replace single-winner NIC selection with all equally-close NICs Previously, when multiple NICs tied as closest to a GPU, only the least-used NIC (tracked by assignedCount) was recorded on system that each GPU has 2 NICs at equal PCIe distance, so the old code reported only 1 NIC per GPU instead of 2 because closestNicsToGpu is a topology map, not a traffic assignment Reporting all equally-close NICs is the correct semantic remove the dead assignedCount load-balancing code apply the same fix to the reverse mapping: the bus-ID-distance fallback now collects all GPUs at minimum distance rather than stopping at the first one found also remove the hasActivePort guard from ibvAddressList and the NIC-to-GPU reverse mapping loop, because proximity relates to hardware property inactive NICs are still physically close to their nearest GPU --- src/header/TransferBench.hpp | 50 +++++++++++++----------------------- 1 file changed, 18 insertions(+), 32 deletions(-) diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index 063917e6..67268e3b 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -3202,8 +3202,8 @@ static bool IsConfiguredGid(union ibv_gid const& gid) 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 and bus number from a PCIe address (domain:bus:device.function) + static std::pair ExtractDomainAndBus(std::string const& pcieAddress) { int domain, bus, device, function; char delimiter; @@ -7433,16 +7433,9 @@ static bool IsConfiguredGid(union ibv_gid const& gid) // 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 // 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"); #endif @@ -7450,25 +7443,24 @@ static bool IsConfiguredGid(union ibv_gid const& gid) for (int nicIndex = 0; nicIndex < numNics; nicIndex++) { if (ibvDeviceList[nicIndex].busId != "") { int distance = GetBusIdDistance(hipPciBusId, ibvDeviceList[nicIndex].busId); - if (distance < minDistance && distance >= 0) { + 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 // Loop over each NIC to find the closest GPU(s) based on PCIe address for (int nicIndex = 0; nicIndex < numNics; nicIndex++) { - if (!ibvDeviceList[nicIndex].hasActivePort || ibvDeviceList[nicIndex].busId.empty()) { - continue; - } + if (ibvDeviceList[nicIndex].busId.empty()) continue; // Find closest GPUs using LCA algorithm std::set closestGpuIdxs = GetNearestDevicesInTree(ibvDeviceList[nicIndex].busId, gpuAddressList); @@ -7476,26 +7468,20 @@ static bool IsConfiguredGid(union ibv_gid const& gid) if (closestGpuIdxs.empty()) { // Fallback: use bus ID 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]); 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); } } #endif From 9ed2c823f7f8c597c814149ad64bf39d4488f7fc Mon Sep 17 00:00:00 2001 From: paklui <5041261+paklui@users.noreply.github.com> Date: Fri, 19 Jun 2026 18:51:12 -0700 Subject: [PATCH 4/6] show all closest NICs per GPU in topology display use GetClosestNicsToGpu (vector) instead of GetClosestNicToGpu (int) in both the NIC table and GPU table so all equally-close NICs are shown as a comma-separated list (e.g. "0,1") also fix the NIC table loop: the old code called GetClosestNicsToGpu inside a per-NIC outer loop, making numNics*numGpus times of calls change to build an inverse GPU-per-NIC map upfront in numGpus times and look up the result directly in the print loop --- src/client/Topology.hpp | 30 +++++++++++++++++++++--------- 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/src/client/Topology.hpp b/src/client/Topology.hpp index 09269377..4338ef77 100644 --- a/src/client/Topology.hpp +++ b/src/client/Topology.hpp @@ -52,16 +52,20 @@ 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 (!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", @@ -192,13 +196,21 @@ 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", + std::vector nicsForGpu; + TransferBench::GetClosestNicsToGpu(nicsForGpu, i); + std::string nicStr = ""; + for (int nicIdx : nicsForGpu) { + if (nicStr != "") nicStr += ","; + nicStr += std::to_string(nicIdx); + } + if (nicStr == "") 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 From 0673279646dc629dc4764893adffcb0216bc11af Mon Sep 17 00:00:00 2001 From: paklui <5041261+paklui@users.noreply.github.com> Date: Tue, 25 Aug 2026 14:55:23 -0700 Subject: [PATCH 5/6] Address review: narrow NIC proximity fix to domain metric + ties Reworks the earlier commits on this branch in response to review, keeping the behaviour that fixes NIC-to-GPU proximity on multi-domain systems while dropping the parts that were unnecessary or actively harmful. Reverted: including inactive NICs in the PCIe proximity tree Verified on a 4-GPU / 9-NIC multi-domain rack that this was a no-op for the tree and a regression for executor selection: - No-op: an LCA depends only on the two paths being compared, so adding a NIC's nodes cannot change any other pair's result. On this hardware GPUs and NICs never share a root complex (GPU 3 is under pci0004:00, its NICs under pci0004:40), so every NIC ties at the same LCA depth and the domain metric is what actually discriminates. - Regression: it let a NIC with a down port be recorded as "closest". NicIsActive() rejects such a Transfer with ERR_FATAL rather than falling back to a usable NIC, so nic_rings (which builds a ring for every NIC returned by GetClosestNicsToGpu) would abort on any tray with a down link, and NIC_NEAREST could resolve to an unusable NIC. Reverted: the minDistance/matches.clear() reordering in GetNearestDevicesInTree, which was a no-op - matches.clear() does not read minDistance, and minDistance is assigned before the loop advances, so both orderings behave identically. Kept and tightened: - ExtractBusNumber -> ExtractDomain, GetBusIdDistance -> GetDomainDistance. The metric no longer consults bus numbers at all, so the old names were misleading. Returning a plain int also removes the unused structured bindings flagged in review. Bus numbers are deliberately not used to discriminate within a domain: doing so would break ties between NICs that are genuinely equidistant from a GPU, which is the case this fix exists to preserve. Dropped the *256 scaling, which became vestigial once same-domain returns 0; ordering is unchanged. - Record all equally-close NICs/GPUs instead of picking one via the assignedCount round-robin. - Topology display uses the vector-returning API so ties are visible, and joins the NIC list with spaces so the field stays a single column when OUTPUT_TO_CSV=1 makes the separator a comma. Verified on ctheliosr-rck-g02-k19-2 (4x gfx1250, 8 BE + 1 FE NIC, one GPU per PCIe domain). Before, all four GPUs mapped to NIC 0, the front-end NIC in domain 0000. After: GPU0->1,2 GPU1->5,6 GPU2->3,4 GPU3->7 (ionic_8's port is down), and the front-end NIC maps to no GPU. CSV rows hold a constant 11 fields; build is warning-free. Co-Authored-By: Claude --- src/client/Topology.hpp | 9 ++-- src/header/TransferBench.hpp | 91 ++++++++++++++++-------------------- 2 files changed, 45 insertions(+), 55 deletions(-) diff --git a/src/client/Topology.hpp b/src/client/Topology.hpp index 9c2a07b6..4c4441f3 100644 --- a/src/client/Topology.hpp +++ b/src/client/Topology.hpp @@ -59,6 +59,7 @@ static void PrintNicToGPUTopo(bool outputToCsv) 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); } @@ -195,14 +196,16 @@ void DisplaySingleRankTopology(bool outputToCsv) char pciBusId[20]; HIP_CALL(hipDeviceGetPCIBusId(pciBusId, 20, i)); + // 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 = ""; + std::string nicStr; for (int nicIdx : nicsForGpu) { - if (nicStr != "") nicStr += ","; + if (!nicStr.empty()) nicStr += ' '; nicStr += std::to_string(nicIdx); } - if (nicStr == "") nicStr = "-1"; + 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, diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index af9a3e87..29084f4f 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -3176,15 +3176,7 @@ const auto& AmdSmiFabricInfoV1(const T& info) // Add NICs to the tree auto const& ibvDeviceList = GetIbvDeviceList(); for (IbvDevice const& ibvDevice : ibvDeviceList) { - // Include all NICs in the PCIe proximity tree regardless of link state. - // PCIe proximity is a hardware property determined by physical wiring through - // root complexes and PCIe switches -- it does not depend on whether a port is - // currently active. Excluding inactive NICs would cause GPUs that share the - // same root complex with them to find no LCA match, falling back to a coarser - // distance heuristic and potentially selecting a more distant NIC instead. - // hasActivePort is enforced separately: it is stored in topo.nicIsActive and - // checked via NicIsActive() during transfer validation, before any QP is set up. - if (ibvDevice.busId == "") continue; + if (!ibvDevice.hasActivePort || ibvDevice.busId == "") continue; InsertPCIePathToTree(ibvDevice.busId, ibvDevice.name, pcieRoot); } @@ -3250,8 +3242,9 @@ const auto& AmdSmiFabricInfoV1(const T& info) return -1; } - // Function to extract the domain and bus number from a PCIe address (domain:bus:device.function) - static std::pair ExtractDomainAndBus(std::string const& pcieAddress) + // Function to extract the domain number from a PCIe address (domain:bus:device.function) + // The full address is parsed (not just the domain) so that a malformed address is rejected + static int ExtractDomain(std::string const& pcieAddress) { int domain, bus, device, function; char delimiter; @@ -3262,36 +3255,33 @@ const auto& AmdSmiFabricInfoV1(const T& info) #ifdef VERBS_DEBUG System::Get().Log("Invalid PCIe address format: %s\n", pcieAddress.c_str()); #endif - return {-1, -1}; + return -1; } - return {domain, bus}; + return domain; } - // Compute a proximity distance between two PCIe addresses, used as a secondary - // tiebreaker when candidates share the same LCA depth in the PCIe tree. + // 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 (returns 0): within one PCIe domain all devices share a root - // complex, so the LCA tree already captures their true structural proximity. - // Bus numbers within a domain are firmware-assigned and do not reliably reflect - // physical closeness, so they are intentionally not used here (bus is extracted - // by ExtractDomainAndBus but unused in this function). + // 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 (returns |delta_domain| * 256): a PCIe domain holds at most 256 - // bus numbers (0x00-0xFF), so scaling by 256 guarantees any cross-domain - // distance exceeds the maximum possible same-domain bus difference of 255. - // This creates a hard separation: a same-domain device is always ranked closer - // than any cross-domain device, regardless of individual bus numbers. - // - // On platforms where all devices share domain 0000 (standard x86), all - // distances collapse to 0 and the LCA tree is the sole proximity discriminator. - static int GetBusIdDistance(std::string const& pcieAddress1, - std::string const& pcieAddress2) - { - auto [domain1, bus1] = ExtractDomainAndBus(pcieAddress1); - auto [domain2, bus2] = ExtractDomainAndBus(pcieAddress2); + // 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; - if (domain1 == domain2) return 0; - return std::abs(domain1 - domain2) * 256; + return std::abs(domain1 - domain2); } // Given a target busID and a set of candidate devices, returns a set of indices @@ -3311,18 +3301,16 @@ 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); + // When more than one LCA match is found, choose the one with smallest domain difference + // NOTE: currDistance could be -1, which signals problem with parsing, however still + // remains a valid "closest" candidate, so is included if (depth > maxDepth || (depth == maxDepth && depth >= 0 && currDistance < minDistance)) { maxDepth = depth; - // minDistance must be updated before matches.clear() so that any subsequent - // candidate at the same depth and distance correctly passes the == minDistance - // tie-check below. With the old order (clear then update), a candidate - // arriving immediately after a depth-improving entry would compare against the - // stale minDistance from the previous depth level and be incorrectly excluded. - minDistance = currDistance; matches.clear(); matches.insert(i); + minDistance = currDistance; } else if (depth == maxDepth && depth >= 0 && currDistance == minDistance) { matches.insert(i); } @@ -7507,11 +7495,8 @@ const auto& AmdSmiFabricInfoV1(const T& info) std::vector ibvAddressList; auto const& ibvDeviceList = GetIbvDeviceList(); if (IsIbvSymbolsReady()) { - // Include all NICs regardless of link state -- PCIe proximity is a hardware - // property that does not depend on whether a port is currently active. - // hasActivePort is enforced separately via topo.nicIsActive / NicIsActive(). for (auto const& ibvDevice : ibvDeviceList) - ibvAddressList.push_back(ibvDevice.busId); + ibvAddressList.push_back(ibvDevice.hasActivePort ? ibvDevice.busId : ""); // Loop over each GPU to find the closest NIC(s) based on PCIe address for (int gpuIndex = 0; gpuIndex < numGpus; gpuIndex++) { @@ -7521,16 +7506,16 @@ const auto& AmdSmiFabricInfoV1(const T& info) // Find closest NICs std::set closestNicIdxs = GetNearestDevicesInTree(hipPciBusId, ibvAddressList); - // 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 (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); + int distance = GetDomainDistance(hipPciBusId, ibvDeviceList[nicIndex].busId); if (distance >= 0 && distance < minDistance) { minDistance = distance; closestNicIdxs.clear(); @@ -7548,17 +7533,19 @@ const auto& AmdSmiFabricInfoV1(const T& info) // Compute the reverse mapping: closest GPU(s) for each NIC // Loop over each NIC to find the closest GPU(s) based on PCIe address for (int nicIndex = 0; nicIndex < numNics; nicIndex++) { - if (ibvDeviceList[nicIndex].busId.empty()) continue; + if (!ibvDeviceList[nicIndex].hasActivePort || ibvDeviceList[nicIndex].busId.empty()) { + continue; + } // Find closest GPUs using LCA algorithm 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(); 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; closestGpuIdxs.clear(); From 8f118e04e469bdc78ce7395a896f279c7af97d6d Mon Sep 17 00:00:00 2001 From: paklui <5041261+paklui@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:30:49 -0700 Subject: [PATCH 6/6] Address Copilot review on 0673279 Three fixes, one per review comment. 1. Fallback path could map a GPU to a NIC with no active port (TransferBench.hpp:7519). The fallback guarded on ibvDeviceList[nicIndex].busId, the raw device list, which holds a valid BDF even when the port is down, while the PCIe-tree path above uses ibvAddressList, which is blanked out for inactive NICs. A GPU could therefore be mapped to a down NIC through the fallback, and NicIsActive() would later abort the Transfer with ERR_FATAL. Both paths now read ibvAddressList. This is pre-existing in develop, but recording every tie rather than a single NIC widens it, so it is fixed here. 2. An unparseable address won the distance tiebreak (GetNearestDevicesInTree). GetDomainDistance returns -1 on parse failure, and -1 compares smaller than every valid distance, so one malformed candidate outranked valid same-depth candidates. The develop NOTE said such a candidate "remains a valid closest candidate, so is included", but the code made it win rather than merely be included. Unknown distances are now clamped to INT_MAX: still eligible when nothing else matches at that depth, still tying with other unparseable candidates, never outranking a known distance. NOTE: this changes develop behaviour in the unparseable-address path. 3. ExtractDomain's comment overclaimed. It said the full address is parsed "so that a malformed address is rejected", but the stream parse consumes separators without checking them, so 0001-01-00-0 parses successfully. Reworded to describe what the parse actually guarantees. The parsing itself is unchanged from develop; every BDF here originates from sysfs or hipDeviceGetPCIBusId. Not changed: the CSV separator comment on Topology.hpp:209 was re-issued from the previous review and no longer applies. That line joins with a space, not a comma. Verified with OUTPUT_TO_CSV=1: every GPU row holds a constant 11 fields and a multi-NIC value stays a single field. Re-verified on ctheliosr-rck-g02-k19-2, warning-free build. A link flapped between runs (ionic_1 went down, ionic_8 came up), which independently exercised both behaviours: GPU0 correctly dropped to NIC 2 alone, and GPU3 correctly reported the 7,8 tie. Co-Authored-By: Claude --- src/header/TransferBench.hpp | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) diff --git a/src/header/TransferBench.hpp b/src/header/TransferBench.hpp index 29084f4f..1736d016 100644 --- a/src/header/TransferBench.hpp +++ b/src/header/TransferBench.hpp @@ -3243,7 +3243,9 @@ const auto& AmdSmiFabricInfoV1(const T& info) } // Function to extract the domain number from a PCIe address (domain:bus:device.function) - // The full address is parsed (not just the domain) so that a malformed address is rejected + // 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; @@ -3303,9 +3305,13 @@ const auto& AmdSmiFabricInfoV1(const T& info) int depth = GetLcaDepth(lca->address, GetPCIeTreeRoot()); 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 domain difference - // NOTE: currDistance could be -1, which signals problem with parsing, however still - // remains a valid "closest" candidate, so is included if (depth > maxDepth || (depth == maxDepth && depth >= 0 && currDistance < minDistance)) { maxDepth = depth; matches.clear(); @@ -7514,8 +7520,11 @@ const auto& AmdSmiFabricInfoV1(const T& info) #endif int minDistance = std::numeric_limits::max(); for (int nicIndex = 0; nicIndex < numNics; nicIndex++) { - if (ibvDeviceList[nicIndex].busId != "") { - int distance = GetDomainDistance(hipPciBusId, ibvDeviceList[nicIndex].busId); + // 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; closestNicIdxs.clear();