Fix NIC to GPU proximity detection on systems with multiple PCIe domains by paklui · Pull Request #352 · ROCm/TransferBench

paklui · 2026-08-25T20:36:16Z

Motivation

On systems where each GPU sits in its own PCIe domain, TransferBench picked the wrong NIC as closest, thus every GPU mapped to a single front-end NIC that is not wired to any GPU, while the back-end NICs actually adjacent to each GPU mapped to nothing. The mapping decides which NIC a GPU uses for RDMA transfers (NIC_NEAREST executor, nic_rings preset). With incorrect mapping it would mean benchmarking ising a NIC that is not attached to that GPU's root complex, producing misleading numbers. The causes are:

The proximity metric compared raw bus numbers. Bus numbers repeat across domains, so devices in different domains looked "close" purely because their bus numbers were numerically near.
When several NICs were equally close, only one was recorded, chosen by a round-robin counter, so a GPU with two adjacent NICs only ever reported one.

This PR fixes the topology detection so it works correctly on systems with multiple PCIe domains and multiple NICs per GPU (verified on a 4-GPU, 8 BE + 1 FE, or 2 BE NICs per GPU).

Technical Details

Use PCIe domain, not bus number, for the distance metric
Two functions were renamed to match what they now do. ExtractBusNumber became ExtractDomain, and GetBusIdDistance became GetDomainDistance.

The distance between two devices is now computed as follows:

If both devices sit in the same PCIe domain, the distance is 0. Within a single domain, the PCIe tree search already tells us how the two devices are related, so no extra tie-break is needed.
If the two devices sit in different domains, the distance is the difference between their domain numbers. That value is always at least 1, so a device in a different domain always ranks behind every device in the same domain.
Bus numbers are no longer used to compare devices within a domain. Firmware assigns them, and they do not reliably indicate which device is physically closer. Using them would also separate NICs that are genuinely the same distance from a GPU, and reporting those ties is the point of this fix.

Record all equally-close NICs/GPUs
Replaces the assignedCount round-robin, which picked one NIC per GPU even when several were tied. Both directions now keep the full tie set, matching how GetClosestNicsToGpu() was already shaped.

Trade-off: the removed round-robin was intended to spread multi-port NICs (ports sharing one busId) across GPUs. With ties recorded, nic_rings now uses all tied ports, better, but a bare NIC_NEAREST resolves slot 0 for every GPU rather than spreading. Use the explicit N<gpu>.<slot> form to select a specific port.

Update the CLI topology display
The display called the scalar GetClosestNicToGpu(), so it would have shown one NIC even after (2). Switched to the vector API. The NIC list is space-separated so the field stays a single column under OUTPUT_TO_CSV=1, where the separator is a comma.

ionic_8's port is down, so GPU 3 reports only NIC 7. NICs that cannot execute a Transfer are excluded from the mapping: NicIsActive() rejects such a Transfer with ERR_FATAL rather than falling back, so one would make nic_rings abort on any tray with a down link.
OUTPUT_TO_CSV=1 rows hold a constant 11 fields regardless of NIC count.

Test Plan

Compare both unmodified develop (44dec25) and this branch merged with develop on the same hardware, and compared the topology output.

System with 4 GPUs, 8 BE NICs + 1 FE NIC. For example, 2 NICs are wired via PCIe per GPU, each GPU in its own PCIe domain.
latest therock or ROCm 7.15.0

Test Result

Current develop HEAD:

 NIC | Device Name | Active | PCIe Bus ID  | NUMA | Closest GPU(s) | GID Index | GID Descriptor
-----+-------------+--------+--------------+------+----------------+-----------+-------------------
 0   | ionic_0     | Yes    | 0000:04:00.0 | 0    | 0,1,2,3        | 1         | RoCEv2 IPv4-mapped IPv6
 1   | ionic_1     | Yes    | 0001:44:00.0 | -1   |                | 1         | RoCEv2 IPv4-mapped IPv6
 2   | ionic_2     | Yes    | 0001:48:00.0 | -1   |                | 1         | RoCEv2 IPv4-mapped IPv6
 3   | ionic_3     | Yes    | 0003:44:00.0 | -1   |                | 1         | RoCEv2 IPv4-mapped IPv6
 4   | ionic_4     | Yes    | 0003:48:00.0 | -1   |                | 1         | RoCEv2 IPv4-mapped IPv6
 5   | ionic_5     | Yes    | 0002:44:00.0 | -1   |                | 1         | RoCEv2 IPv4-mapped IPv6
 6   | ionic_6     | Yes    | 0002:48:00.0 | -1   |                | 1         | RoCEv2 IPv4-mapped IPv6
 7   | ionic_7     | Yes    | 0004:44:00.0 | -1   |                | 1         | RoCEv2 IPv4-mapped IPv6
 8   | ionic_8     | No     | 0004:48:00.0 | -1   |                | N/A       | N/A

        | gfx1250 | gfx1250 | gfx1250 | gfx1250 |
        | GPU 00 | GPU 01 | GPU 02 | GPU 03 | PCIe Bus ID  | #CUs | NUMA | #DMA | #XCC | NIC
--------+--------+--------+--------+--------+--------------+------+------+------+------+------
 GPU 00 |    N/A | XGMI-1 | XGMI-1 | XGMI-1 | 0001:01:00.0 | 256  | 0    | 16   | 8    | 0
 GPU 01 | XGMI-1 |    N/A | XGMI-1 | XGMI-1 | 0002:01:00.0 | 256  | 0    | 16   | 8    | 0
 GPU 02 | XGMI-1 | XGMI-1 |    N/A | XGMI-1 | 0003:01:00.0 | 256  | 0    | 16   | 8    | 0
 GPU 03 | XGMI-1 | XGMI-1 | XGMI-1 |    N/A | 0004:01:00.0 | 256  | 0    | 16   | 8    | 0

Currently every one of the 4 GPUs is mapped to NIC 0 which is on PCIe domain 0000, a completely different domain from any GPU (0001-0004). NIC 0 is a Front End NIC that is not physically wired to any GPU. Meanwhile the 8 Back End NICs that are actually adjacent to a GPU (ionic_1-ionic_8) show up as closest to blank. This is the domain-blind bus-number comparison from change (2) picking the numerically-smallest bus number regardless of domain, combined with (3)'s single-NIC-only recording.

actual output from ./TransferBench after (this branch merged with develop):

 NIC | Device Name | Active | PCIe Bus ID  | NUMA | Closest GPU(s) | GID Index | GID Descriptor
-----+-------------+--------+--------------+------+----------------+-----------+-------------------
 0   | ionic_0     | Yes    | 0000:04:00.0 | 0    |                | 1         | RoCEv2 IPv4-mapped IPv6   ### NOTE: FE NIC, not GPU-facing
 1   | ionic_1     | Yes    | 0001:44:00.0 | -1   | 0              | 1         | RoCEv2 IPv4-mapped IPv6
 2   | ionic_2     | Yes    | 0001:48:00.0 | -1   | 0              | 1         | RoCEv2 IPv4-mapped IPv6
 3   | ionic_3     | Yes    | 0003:44:00.0 | -1   | 2              | 1         | RoCEv2 IPv4-mapped IPv6
 4   | ionic_4     | Yes    | 0003:48:00.0 | -1   | 2              | 1         | RoCEv2 IPv4-mapped IPv6
 5   | ionic_5     | Yes    | 0002:44:00.0 | -1   | 1              | 1         | RoCEv2 IPv4-mapped IPv6
 6   | ionic_6     | Yes    | 0002:48:00.0 | -1   | 1              | 1         | RoCEv2 IPv4-mapped IPv6
 7   | ionic_7     | Yes    | 0004:44:00.0 | -1   | 3              | 1         | RoCEv2 IPv4-mapped IPv6
 8   | ionic_8     | No     | 0004:48:00.0 | -1   | 3              | N/A       | N/A            

        | gfx1250 | gfx1250 | gfx1250 | gfx1250 |
        | GPU 00 | GPU 01 | GPU 02 | GPU 03 | PCIe Bus ID  | #CUs | NUMA | #DMA | #XCC | NIC
--------+--------+--------+--------+--------+--------------+------+------+------+------+------
 GPU 00 |    N/A | XGMI-1 | XGMI-1 | XGMI-1 | 0001:01:00.0 | 256  | 0    | 16   | 8    | 1,2
 GPU 01 | XGMI-1 |    N/A | XGMI-1 | XGMI-1 | 0002:01:00.0 | 256  | 0    | 16   | 8    | 5,6
 GPU 02 | XGMI-1 | XGMI-1 |    N/A | XGMI-1 | 0003:01:00.0 | 256  | 0    | 16   | 8    | 3,4
 GPU 03 | XGMI-1 | XGMI-1 | XGMI-1 |    N/A | 0004:01:00.0 | 256  | 0    | 16   | 8    | 7,8

Each GPU is now correctly mapped to the NIC(s) in its own PCIe domain:

GPU 0 -> NICs 1,2 (domain 0001)
GPU 1->NIC 5,6 (domain 0002)
GPU 2 -> NICs 3,4 (domain 0003)
GPU 3 -> NICs 7,8 (domain 0004)
NIC 0 (domain 0000, FE NIC) correctly shows no GPU mapping, since no GPU exists in its domain

Also confirmed a clean build with no new warnings, using the dynamically-loaded libibverbs path (IsIbvSymbolsReady()) that develop added since this branch's base, e.g. this fix is verified against the current develop HEAD

Submission Checklist

Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.

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

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

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

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

# Conflicts: # src/header/TransferBench.hpp

Copilot

Pull request overview

This PR fixes NIC↔GPU proximity detection on multi-PCIe-domain systems so that GPUs map to the correct physically-nearest NICs (including ties where multiple NICs are equally close), and updates the CLI topology display to reflect multi-NIC proximity.

Changes:

Include NICs in the PCIe proximity tree regardless of link state, while still enforcing link state at transfer validation time.
Make the proximity tie-breaker domain-aware to avoid “bus-number-only” false closeness across PCIe domains.
Record and display all equally-close NICs per GPU (and GPUs per NIC), updating CLI output accordingly.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
src/header/TransferBench.hpp	Fixes PCIe-domain-aware proximity distance and records all equally-close NIC/GPU ties during topology discovery.
src/client/Topology.hpp	Updates printed topology to use vector-based “closest NICs” APIs and display multiple NICs per GPU / GPUs per NIC.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

src/client/Topology.hpp:77

PrintNicToGPUTopo() passes std::to_string(...).c_str() directly to printf. That pointer refers to a temporary std::string that is destroyed before/during the call, so the %s argument is dangling (undefined behavior).

    printf(" %-3d | %-11s | %-6s | %-12s | %-4d | %-14s | %-9s | %-20s\n",
           i, ibvDeviceList[i].name.c_str(),
           ibvDeviceList[i].hasActivePort ? "Yes" : "No",
           ibvDeviceList[i].busId.c_str(),
           ibvDeviceList[i].numaNode,
           closestGpusStr.c_str(),
           ibvDeviceList[i].isRoce && ibvDeviceList[i].hasActivePort ? std::to_string(ibvDeviceList[i].gidIndex).c_str() : "N/A",
           ibvDeviceList[i].isRoce && ibvDeviceList[i].hasActivePort ? ibvDeviceList[i].gidDescriptor.c_str() : "N/A"

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 <noreply@anthropic.com>

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

paklui and others added 5 commits June 22, 2026 14:30

include inactive NICs in PCIe proximity tree

7933817

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

Merge remote-tracking branch 'origin/develop' into gfx1250-2nicpergpu

28bfcfe

# Conflicts: # src/header/TransferBench.hpp

Copilot AI lite review requested due to automatic review settings August 25, 2026 20:36

Copilot started reviewing on behalf of paklui August 25, 2026 20:37 View session

paklui self-assigned this Aug 25, 2026

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Comment thread src/header/TransferBench.hpp Outdated

Comment thread src/client/Topology.hpp

paklui changed the title ~~fix NIC-to-GPU topology/proximity detection on multi-domain systems (system with 2 or more NICs per GPU)~~ Fix NIC to GPU proximity detection on systems with multiple PCIe domains Aug 25, 2026

Copilot AI review requested due to automatic review settings August 25, 2026 22:12

Copilot started reviewing on behalf of paklui August 25, 2026 22:13 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

Comment thread src/header/TransferBench.hpp

Comment thread src/header/TransferBench.hpp

Comment thread src/header/TransferBench.hpp

Copilot AI review requested due to automatic review settings August 25, 2026 22:31

Copilot started reviewing on behalf of paklui August 25, 2026 22:32 View session

Copilot AI reviewed Aug 25, 2026

View reviewed changes

paklui marked this pull request as ready for review August 25, 2026 22:38

paklui requested a review from a team as a code owner August 25, 2026 22:38

paklui mentioned this pull request Aug 26, 2026

Fix NIC to GPU proximity detection on systems with multiple PCIe domains (candidate-1.70) #353

Open

1 task

Navigation Menu

Uh oh!

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

Fix NIC to GPU proximity detection on systems with multiple PCIe domains#352
paklui wants to merge 7 commits into
ROCm:developfrom
paklui:gfx1250-2nicpergpu

paklui commented Aug 25, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

paklui commented Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Technical Details

Test Plan

Test Result

Submission Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

paklui commented Aug 25, 2026 •

edited

Loading