Repository navigation
UCS/SYS/TOPO: Allow multiple sibling devices per memory device - #12126
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The BlueField gtest job failed on a single unrelated test, Full analysisSummary: Root cause: Evidence from the log:
On a BlueField ARM target the progress thread dispatches expired timers in batch ( Note: the fetched log is the job tail; the failing test ran early in the run so its individual assertion message is outside the retrievable window. The attribution above is based on the test's source-level timing assumptions, not on the quoted assertion text. Implicated commit: none — PR commit [REDACTED:Hex High Entropy String] (Tomer Gilad, topo-only change) is not implicated; the flaky test logic predates it (last functional touches: a7b71bc "UCS/ASYNC: Fix crash when async thread is not running", 2faf305 "UCS/ASYNC: Filter stale missed fd events") File: test/gtest/ucs/test_async.cc:830 ( Suggested fix:
Related: PR #12126 (the PR under test); prior flake report for the same test: #2023
|
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap worth noting: the new tests only assert |
|
🤖 CI Triage Agent — TL;DR: The gpu gtest run aborted on a UCT assertion — Full analysisSummary: Root cause: The backtrace is unambiguous: The reason this surfaces on this branch: UCP failover assumes each lane has independent UCT send-queue state, but for DC the txwq belongs to a DCI shared across endpoints/lanes. PR #12126 / commit Implicated commit: File: Suggested fix:
Related: PR #12126 (this change); context on failover lane rebuild: #11562, #11563, #11988, #12085; recent txwq FAILED/purge work: #11829, #12058.
|
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The Coverity Full analysisSummary: Root cause: Size mismatch between the new sibling bitmap and the device table introduced by this PR:
At Implicated commit: File: Suggested fix: Guard the loop body with the same validity check used elsewhere in the file: static void ucs_topo_sys_device_clear_siblings(ucs_sys_device_t sys_dev)
{
ucs_topo_sys_device_info_t *device = &ucs_topo_global_ctx.devices[sys_dev];
unsigned peer;
UCS_STATIC_BITMAP_FOR_EACH_BIT(peer, &device->siblings) {
if (peer >= ucs_topo_global_ctx.num_devices) {
/* Bits past the last registered device are never set; the bitmap
* capacity is rounded up to a whole word, so bound the index. */
break;
}
UCS_STATIC_BITMAP_RESET(&ucs_topo_global_ctx.devices[peer].siblings,
sys_dev);
}
UCS_STATIC_BITMAP_RESET_ALL(&device->siblings);
}Alternatively (more structural, and it also makes Related: PR #12126 (the PR under test, "UCS/SYS/TOPO: Allow multiple sibling devices per memory device"); Coverity report published as build artifact |
The siblings bitmap capacity is rounded up to a whole word, so iterating it can yield indices past the devices table. Stop at num_devices, which also fixes the Coverity OVERRUN report.
|
🤖 Starting review — findings will be posted here when done. |
| */ | ||
| (ucs_topo_global_ctx.devices[sys_dev_mem].sibling_sys_dev == | ||
| sys_dev); | ||
| UCS_STATIC_BITMAP_GET( |
There was a problem hiding this comment.
the siblings bitmap is symmetric now, so is_sibling(mem, dev) also returns true and then trips the assert below (and silently returns 1 in release builds). pls make the role part of the condition instead of only asserting it:
(devices[sys_dev].sibling_role == UCS_TOPO_SIBLING_ROLE_DEV) && UCS_STATIC_BITMAP_GET(...)
Swapped calls do exist in the tree (test_topo.cc sibling test calls ucs_topo_is_sibling(gpu_dev, hca_devs[0])), and the old scalar only exposed this for the one representative MEM, so the bitmap widens the exposure to every alias and plane.
|
|
||
| UCS_STATIC_BITMAP_FOR_EACH_BIT(peer, &device->siblings) { | ||
| if (peer >= ucs_topo_global_ctx.num_devices) { | ||
| break; |
There was a problem hiding this comment.
minor: bits are only set for indices < num_devices (every registration does RESET_ALL, and num_devices only grows except via extract_state, whose callers bound-check sys_dev), so this branch is unreachable. A silent break would also leave the remaining peers stale while device->siblings is cleared below, i.e. asymmetric state. Suggestion:
UCS_STATIC_BITMAP_FOR_EACH_BIT(peer, &device->siblings) {
ucs_assertv(peer < ucs_topo_global_ctx.num_devices,
"sys_dev=%u peer=%u num_devices=%u", sys_dev, peer,
ucs_topo_global_ctx.num_devices);
UCS_STATIC_BITMAP_RESET(&ucs_topo_global_ctx.devices[peer].siblings,
sys_dev);
}
| UCS_TOPO_SIBLING_ROLE_MEM; | ||
|
|
||
| /* All devices sharing the auxiliary device match the memory device */ | ||
| for (dev = 0; dev < ucs_topo_global_ctx.num_devices; dev++) { |
There was a problem hiding this comment.
minor: is_pci_bridge() does 2 realpath() + 3 PATH_MAX allocations per candidate under the global spinlock, so now every memory device pays it for all DEVs. With MPS aliases this becomes num_mems * num_devs path resolutions at init. maybe we can resolve the sysfs path of sys_dev_aux once per device instead of per pair?
|
Residual coverage gap: still no test asserting the MEM-side bit is dropped from a specific plane (only Residual coverage gap: the new tests remain hardware dependent: |
|
🤖 CI Triage Agent — TL;DR: The Coverity Full analysisSummary: Root cause: Implicated commit: File: Suggested fix: Make the bound a compile-time constant so the array index is provably in range, e.g.: UCS_STATIC_BITMAP_FOR_EACH_BIT(peer, &device->siblings) {
if ((peer >= UCS_SYS_DEVICE_ID_COUNT) ||
(peer >= ucs_topo_global_ctx.num_devices)) {
break;
}
UCS_STATIC_BITMAP_RESET(&ucs_topo_global_ctx.devices[peer].siblings,
sys_dev);
}(plus |
The siblings bitmap is rounded up to a whole word, so its iterator can yield an index past the devices table. num_devices is a runtime value Coverity cannot bound, so also check against UCS_SYS_DEVICE_ID_COUNT.
|
🤖 Starting review — findings will be posted here when done. |
| * the MEM-side sibling is the canonical relationship. | ||
| * The MEM side holds every matched DEV, including all planes | ||
| * of a multiplane NIC. | ||
| */ |
There was a problem hiding this comment.
The bitmap is symmetric, so swapped arguments also return true and trip the assert. Please gate on the role, like ucs_topo_is_reachable() does.
| */ | |
| (ucs_topo_global_ctx.devices[sys_dev].sibling_role == | |
| UCS_TOPO_SIBLING_ROLE_DEV) && | |
| /* | |
| * The MEM side holds every matched DEV, including all planes | |
| * of a multiplane NIC. | |
| */ | |
| UCS_STATIC_BITMAP_GET( | |
| ucs_topo_global_ctx.devices[sys_dev_mem].siblings, | |
| sys_dev); |
The siblings bitmap is symmetric, so with swapped arguments the DEV side bit matched as well and tripped the role assertion.
|
🤖 Starting review — findings will be posted here when done. |
| unsigned peer; | ||
|
|
||
| UCS_STATIC_BITMAP_FOR_EACH_BIT(peer, &device->siblings) { | ||
| if ((peer >= UCS_SYS_DEVICE_ID_COUNT) || |
There was a problem hiding this comment.
minor: a sibling bit is only ever set for a registered device (both ids in sibling_match() are < num_devices), so this can never trigger; pls make it an assert instead of silently skipping the rest of the peers.
| if ((peer >= UCS_SYS_DEVICE_ID_COUNT) || | |
| ucs_assertv(peer < ucs_topo_global_ctx.num_devices, | |
| "peer=%u num_devices=%u", peer, | |
| ucs_topo_global_ctx.num_devices); |
| (sys_dev_mem != UCS_SYS_DEVICE_ID_UNKNOWN) && | ||
| (sys_dev_mem < ucs_topo_global_ctx.num_devices) && | ||
| (ucs_topo_global_ctx.devices[sys_dev].sibling_role == | ||
| UCS_TOPO_SIBLING_ROLE_DEV) && |
There was a problem hiding this comment.
minor: with this check in the condition, role_dev == UCS_TOPO_SIBLING_ROLE_DEV in the assert below is always true - pls assert only role_dev_mem.
|
|
||
| // Setting the same auxiliary device again keeps the pairing | ||
| ASSERT_UCS_OK(ucs_topo_sys_device_set_sys_dev_aux(plane, dma_dev)); | ||
| EXPECT_TRUE(ucs_topo_is_sibling(plane, gpu_dev)); |
There was a problem hiding this comment.
can we also call ucs_topo_sys_device_enable_aux_path(gpu_dev) twice here? The new MEM early-return in enable_aux_path() is not covered, while the set_sys_dev_aux() one is covered by this check.
|
Residual gaps (not posted, already raised in earlier rounds): the suite still depends on the host having |
|
🤖 CI Triage Agent — Summary: Unable to fetch logs — cannot diagnose without them.
|
What?
Topology sibling tracking now keeps a bitmap of all matched sibling devices. Before, it kept a single sibling_sys_dev per device.
Why?
On a multi-plane directNIC setup, each ConnectX-8 shows up as several plane RDMA devices that all report the same Data Direct device. The old code kept only one match, so just one plane was reachable from the GPU with Direct NIC.
How?