Repository navigation
Conversation
The ZE and ROCm allocators ignored the device id given by -a <type:dev-id> and always derived the GPU from the group index, so only CUDA honored the option. Extract the shared selection into ucx_perf_get_gpu_index() and use it from all three allocators, so ZE and ROCm also validate the requested id. ROCm additionally gains a device count check, replacing an unguarded modulo by the device count. The id selects the device for the allocator context and for UCT allocations. Under UCP the buffers are allocated by the memory domain (ze_cpy uses UCX_ZE_COPY_DEVICE_ORDINAL, and rocm_cpy takes rocm memory from the last HSA device pool), so only CUDA placement follows the id there. Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap worth noting: there is no test or CI job exercising |
|
🤖 CI Triage Agent — TL;DR: The gpu worker's gtest run failed on a single unrelated protocol‑selection test, Full analysisSummary: Root cause: The log shows a legitimate assertion failure (not a timeout/hang — the run finished tear-down normally and all other tests passed). The failing test runs with The PR under test (#11993, "TOOLS/PERF: Honor configured device id in ZE and ROCm allocators") touches only Implicated commit: 0311ed7 — jeffnvidia, "UCP/PROTO/RNDV: Isolate remote system device namespace (#11685)" (added the File: test/gtest/ucp/test_ucp_proto.cc:805-813 (test body) and test/gtest/ucp/test_ucp_proto.cc:438 ( Suggested fix:
Related: #11685, #11858, #11861, #11864, #11990 (topology assignment on DPU-equipped nodes), PR under test: #11993
|
| ucs_error("Illegal cuda device %d number of devices %d", gpu_index, | ||
| num_gpus); | ||
| return UCS_ERR_NO_DEVICE; | ||
| status = ucx_perf_get_gpu_index(perf, group_index, num_gpus, "cuda", |
There was a problem hiding this comment.
-a currently also switches the run into device api mode: it adds UCP_FEATURE_DEVICE (libperf.c:958), moves thread_count into device_thread_count (perftest_params.c:866), and routes ucp dispatch through ucx_perf_mem_type_device_dispatchers[] (ucp_tests.cc:1209), which is registered for cuda only. So using -a to pick a ze/rocm device also pulls in device mode semantics that those allocators do not support.
Can we add a separate knob to enable device mode, and keep -a for device type and id only, as its help text already says? Then cuda could also select a device in a regular run, which is not possible today.
There was a problem hiding this comment.
Agreed, -a currently mixes device selection with device-API mode, and this is pre-existing: on master, ucx_perftest -t tag_bw -m cuda -a cuda:1 also fails with Invalid test case, so CUDA can't select a device in a regular UCP run either. Adding a separate knob changes CLI semantics that CI relies on (test_jenkins.sh uses -a cuda:0 to enable device mode), so I'd like to keep this PR limited to honoring the id in the allocators and do the knob in a follow-up PR. With this PR alone, the ZE/ROCm device id takes effect for UCT tests.
There was a problem hiding this comment.
-a today is only usable for the device feature: it implies UCP_FEATURE_DEVICE (libperf.c:958), moves thread_count into device_thread_count (perftest_params.c:866) and routes ucp dispatch through ucx_perf_mem_type_device_dispatchers[] (ucp_tests.cc:1209), which is registered for cuda only. So it would make ze/rocm honor a device id only in a mode those allocators do not support, and no allocator supports placement outside that mode today.
Can we instead move the device id to -m, with the same <type>:<dev-id> syntax, and keep -a without argument to only enable the device feature? -m already selects the allocator that places the buffers, and it avoids the strange -m ze-device -a ze-device:0.
There was a problem hiding this comment.
actually started #12003 that should cover this PR, if you could confirm?
There was a problem hiding this comment.
Yes, #12003 subsumes this PR: the shared device-index helper, ZE/ROCm honoring an explicit id, the range checks and the ROCm zero-device guard, and it also separates device mode from device selection, which answers the open -a threads and the type-mismatch question here. Since it targets master independently and moves the id from -a <type:id> to -m <allocator:id>, I'll treat it as a replacement and close this PR once #12003 is merged. I left a few notes on #12003; the main one is preserving the GPU-0 pinning in the CUDA device CI job.
Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap: no CI job exercises |
Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| /* group index 0 is the receiver side */ | ||
| gpu_index = (group_index == 0) ? perf->params.recv_device.device_id : |
There was a problem hiding this comment.
minor: should we also check that the -a device type matches the allocator? Only device_id is read here (without looking at send_device.mem_type/recv_device.mem_type), so a device id given for one accelerator type is applied to whichever allocator happens to run init() — e.g. -m rocm -a cuda:1 silently selects rocm device 1 instead of reporting a mismatch. Low impact (mostly a mistyped-argument case) and partly pre-existing for cuda, but this PR is the point where the code becomes shared across three backends. This overlaps the still-open thread about -a semantics for rocm/ze, so it may be simplest to answer it there.
What?
Extract the accelerator device-selection logic shared by the CUDA, ZE and ROCm perftest allocators into
ucx_perf_get_gpu_index()inlibperf_int.h, and use it from all three. The ZE and ROCm allocators now honor the device id given by-a <type:dev-id>, and an out-of-range id is rejected at allocator init.Why?
An explicit device id was silently ignored by the ZE and ROCm allocators: only the CUDA allocator read
params.{send,recv}_device.device_id, so a run requested on a specific card was placed bygroup_index % num_gpusinstead, with no diagnostic. ROCm also computed that modulo without checking the device count, which divides by zero whenhipGetDeviceCount()reports none. The same selection logic was duplicated in three allocators.How?
A single helper, used by all three allocators.
-adoes not select GPU memory on its own;-mselects the allocator, and the id then applies to it. For UCT with-m ze-*/-m rocm*, the perftest allocator allocates the buffers (.uct_alloc), so the id selects the device they are placed on.Scope: with this PR the ZE/ROCm device id takes effect for UCT tests. For UCP, these allocators define no
.mem_alloc, so buffers are allocated by the memory domain (UCP_MEM_MAP_ALLOCATE), not placed by the selected id. Moreover,-aswitches the run into device-API mode, whose dispatcher is registered for CUDA memory types only and implements only PUT tests. A regular UCP test such astag_bwwith-atherefore fails withInvalid test caseon both master and this branch, including with CUDA. Separating device selection from device-API mode will be done in a follow-up PR.Testing (master
8cfe4b803vs this branch, same configure flags, same commands):CUDA_VISIBLE_DEVICES=1,2: same device-count and upper-bound checks, no device-selection behavior change. The device chosen for-a cuda:0,-a cuda:1,-a cuda:0,cuda:1and no-ais identical between master and this branch, and UCPtag_bw -m cudabandwidth matches. The only difference is the error text for an out-of-range id:Illegal cuda device 2 number of devices 2becomesillegal cuda device 2, number of devices 2.ZE_AFFINITY_MASK=1,2, with-x rc_verbs -d mlx5_0:1 -t am_lat -D zcopy -m ze-device:-a ze-device:2,-a ze-device:9) is rejected on both processes (illegal ze device N, number of devices 2); master accepts it silently.-avalue (placement by group index only). On this branch they follow the requested id, including split send/recv ids, and are unchanged without-a. Placement was inferred from the device-specific address ranges, not queried from the driver.ibv_reg_mr()fails withBad addressfor ZE device pointers, because this UCT registration path supplies no DMA-BUF fd andib_md.ctherefore takes the VA path. The failure is identical on master and without-a, so it is pre-existing and unrelated to this change.tag_bw -m ze-devicewithout-acompletes with matching bandwidth on master and this branch.Pre-existing limitation found while testing, not addressed here:
UCX_PERF_CMD_PUTwith-azeroesrequired_flags/max_size, so that UCT path cannot pass capability checking.