Skip to content

Fix NIC to GPU proximity detection on systems with multiple PCIe domains #352

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
paklui wants to merge 7 commits into
base: develop
Choose a base branch
from
+86 −71
Open

Fix NIC to GPU proximity detection on systems with multiple PCIe domains #352

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
7933817
include inactive NICs in PCIe proximity tree
paklui Jun 20, 2026
d12537c
use PCIe domain number for NIC-GPU distance metric
paklui Jun 20, 2026
afd3e65
map all equally-close NICs per GPU in both directions
paklui Jun 20, 2026
9ed2c82
show all closest NICs per GPU in topology display
paklui Jun 20, 2026
28bfcfe
Merge remote-tracking branch 'origin/develop' into gfx1250-2nicpergpu
paklui Aug 25, 2026
0673279
Address review: narrow NIC proximity fix to domain metric + ties
paklui Aug 25, 2026
8f118e0
Address Copilot review on 0673279
paklui Aug 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 24 additions & 9 deletions src/client/Topology.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> closestGpusForNic(ibvDeviceList.size(), "");
for (int j = 0; j < numGpus; j++) {
std::vector<int> 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",
Expand Down Expand Up @@ -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<int> 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
Expand Down
124 changes: 62 additions & 62 deletions src/header/TransferBench.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment thread
paklui marked this conversation as resolved.
{
int domain, bus, device, function;
char delimiter;
Expand All @@ -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
Expand All @@ -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<int>::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)) {
Comment thread
paklui marked this conversation as resolved.
maxDepth = depth;
matches.clear();
Expand Down Expand Up @@ -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<int> 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;
Expand All @@ -7503,34 +7512,31 @@ const auto& AmdSmiFabricInfoV1(const T& info)
// Find closest NICs
std::set<int> 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<int>::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) {
Comment thread
paklui marked this conversation as resolved.
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
Expand All @@ -7544,28 +7550,22 @@ const auto& AmdSmiFabricInfoV1(const T& info)
std::set<int> closestGpuIdxs = GetNearestDevicesInTree(ibvDeviceList[nicIndex].busId, gpuAddressList);

if (closestGpuIdxs.empty()) {
// Fallback: use bus ID distance
// Fallback: use PCIe domain distance
int minDistance = std::numeric_limits<int>::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);
}
}
}
Expand Down
Loading