Repository navigation
rocrtst: fix async-copy topology discovery under hwloc 2.x - #11137
Conversation
✅ All Policy Checks Passed
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🚫 Please fix the failed policies before requesting reviews. The following policy checks failed:
The |
There was a problem hiding this comment.
🟡 Changes recommended
The fallback hwloc header include path is currently added after the main rocrtst target is created, which can prevent the bundled header from being applied and break fallback builds.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates rocrtst’s async-copy performance tests and build logic to correctly discover PCI/IO devices when linking against hwloc 2.x, avoiding failures caused by hwloc header/library mismatches and ensuring topology discovery succeeds on modern systems.
Changes:
- Version-guard hwloc IO-device enablement in
MemoryAsyncCopy::FindTopology()(flags for hwloc < 2.0, IO type filter for hwloc >= 2.0). - Stop placing the bundled hwloc 1.x headers on the global include path; only use them in the bundled-hwloc fallback branch.
File summaries
| File | Description |
|---|---|
| projects/rocr-runtime/rocrtst/suites/test_common/CMakeLists.txt | Prevents bundled hwloc 1.x headers from shadowing system/TheRock hwloc 2.x headers; scopes fallback header exposure. |
| projects/rocr-runtime/rocrtst/suites/performance/memory_async_copy.cc | Enables PCI/IO discovery under hwloc 2.x via IO type filter while retaining hwloc 1.x behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
cfreeamd
left a comment
cfreeamd
left a comment
There was a problem hiding this comment.
🤖 AI-assisted pre-review (for @cfreeamd)
The hwloc 1.x/2.x migration in memory_async_copy.cc looks correct — gating HWLOC_TOPOLOGY_FLAG_IO_DEVICES to < 2.0 and using hwloc_topology_set_io_types_filter(KEEP_ALL) for 2.x is the right replacement, and the CMake change to stop the bundled 1.x header from shadowing a system 2.x lib (with the explanatory comment) is a good catch. Two non-blocking notes inline. Note: the missing-rocrtst-YAML check does not apply here — no new TEST()/TEST_P() entries were added, this only fixes existing discovery logic, so the config YAMLs need no update.
ec8e6af to
5279dff
Compare
There was a problem hiding this comment.
🟢 Approval recommended
The changes are narrowly scoped to test topology discovery, align with hwloc 1.x/2.x behavior, and the build-system adjustment correctly prevents header/library mismatches without impacting runtime code.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Lite
|
@cfreeamd @dayatsin-amd - Please help review this with priority. I have resolved earlier review comments. Thank you. |
The Memory_Async_Copy performance tests compile against a bundled hwloc 1.x header while linking a system hwloc 2.x library. Under hwloc 2.x the legacy HWLOC_TOPOLOGY_FLAG_IO_DEVICES flag has no effect; PCI/IO devices are instead gated by a type filter that defaults to keeping none. As a result the loaded topology contained no PCI devices, the busid lookup returned null for every GPU, and topology discovery aborted with an error before any bandwidth measurement ran. Enable IO devices in a version-correct way: keep the legacy flag for hwloc 1.x, and set the IO type filter to keep-all for hwloc 2.x. Also stop forcing the bundled 1.x header onto the global include path so a system hwloc 2.x build compiles against the matching 2.x header; the bundled header is now used only in the bundled-library fallback.
…opagation Check the return values of the hwloc IO-type-filter and topology-load calls in the async-copy topology discovery path so a failure surfaces explicitly instead of silently producing a topology with zero PCI devices. Attach the bundled fallback hwloc header to the imported target's usage requirements instead of a global include directory, so it propagates through the link dependency independent of target/directory ordering.
5279dff to
664bfc0
Compare
|
Will merge given code owner approval.
|
The Memory_Async_Copy performance tests compile against a bundled hwloc 1.x header while linking a system hwloc 2.x library. Under hwloc 2.x the legacy HWLOC_TOPOLOGY_FLAG_IO_DEVICES flag has no effect; PCI/IO devices are instead gated by a type filter that defaults to keeping none. As a result the loaded topology contained no PCI devices, the busid lookup returned null for every GPU, and topology discovery aborted with an error before any bandwidth measurement ran. Enable IO devices in a version-correct way: keep the legacy flag for hwloc 1.x, and set the IO type filter to keep-all for hwloc 2.x. Also stop forcing the bundled 1.x header onto the global include path so a system hwloc 2.x build compiles against the matching 2.x header; the bundled header is now used only in the bundled-library fallback. [rocm-systems] ROCm/rocm-systems#11137 (commit aa24ea1)
header propagation Check the return values of the hwloc IO-type-filter and topology-load calls in the async-copy topology discovery path so a failure surfaces explicitly instead of silently producing a topology with zero PCI devices. Attach the bundled fallback hwloc header to the imported target's usage requirements instead of a global include directory, so it propagates through the link dependency independent of target/directory ordering. [rocm-systems] ROCm/rocm-systems#11137 (commit aa24ea1)
TheRock Submodule Bump ActivityNewest first
|
Motivation
The
rocrtstPerf.Memory_Async_CopyandMemory_Async_Copy_On_Engineperformancetests fail during topology discovery on systems that link a modern (hwloc 2.x)
NUMA library. The tests abort before running any bandwidth measurement and print
"**** No GPU found in same NUMA node as a CPU ****", which is misleading because
the real failure is that no PCI/IO devices were discovered at all.
Technical Details
Root cause is an hwloc header/library version mismatch. The tests compile against a
bundled hwloc 1.x header (
thirdparty/include) but link a system hwloc 2.x library.HWLOC_TOPOLOGY_FLAG_IO_DEVICES.instead controlled by a type filter that defaults to
KEEP_NONE. The old flagbit value additionally maps to a different (unrelated) flag in 2.x.
Consequently
FindTopology()loads a topology with zero PCI devices,hwloc_get_pcidev_by_busid()returns null for every GPU, and discovery abortswith
HSA_STATUS_ERRORbefore the NUMA comparison ever runs.Fix (test-only, no runtime changes):
memory_async_copy.ccFindTopology(): version-guard IO-device enablement.Keep
HWLOC_TOPOLOGY_FLAG_IO_DEVICESfor hwloc < 2.0; for hwloc >= 2.0 callhwloc_topology_set_io_types_filter(topology_, HWLOC_TYPE_FILTER_KEEP_ALL)before
hwloc_topology_load().test_common/CMakeLists.txt: stop unconditionally putting the bundledthirdparty/include(hwloc 1.x header) on the global include path, so asystem/TheRock hwloc 2.x build compiles against the matching 2.x header and the
HWLOC_API_VERSIONguards resolve correctly. The bundled header is added onlyinside the bundled-hwloc fallback branch.
Issue Tracking
JIRA ID : ROCM-29094
Test Plan
Built rocrtst against a system hwloc 2.7.0 library on a gfx942 (MI300A) host with
4 CPU NUMA nodes + 4 GPUs, then ran the affected filters:
Also confirmed via a standalone hwloc probe that with the fix the loaded topology
contains PCI objects (137) and every GPU busid resolves, versus zero PCI objects
and null lookups without it.
Test Result
HSA_STATUS_ERROR(topology has 0 PCI devices).Memory_Async_CopyPASSED andMemory_Async_Copy_On_EnginePASSED(full bandwidth run, ~163s each). 2/2 PASSED, no regressions.
Submission Checklist