Repository navigation
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
minor: |
|
🤖 Starting review — findings will be posted here when done. |
Done. The header has no preprocessor-testable version macro before API 1.19, so ze.m4 now runs AC_CHECK_DECLS for zeCommandQueueGetOrdinal and zeCommandListGetOrdinal, and the test skips when they are missing. Debian 12 ships API 1.4 headers and Ubuntu 24.04 ships 1.9, so the guard is needed.
Done, num_queues_p may be NULL and ze_copy passes NULL. |
|
Both new tests need real Intel GPUs, and
Residual behavior change worth the author's attention but not comment-worthy on its own: element 0 is now created on a copy-engine ordinal instead of the default group, so |
|
🤖 CI Triage Agent — TL;DR: The ASAN gtest job failed because one test, Full analysisSummary: Root cause: The test runs Implicated commit: unknown — not the PR commit a9227fb (ZE copy engine, not exercised by File: Suggested fix: Re-run this job (and/or run the same test on current master) to confirm it is not PR-related — expected outcome is that it reproduces on master too, in which case the PR can proceed and a separate bug should be opened. For the real defect, instrument/inspect the forced-close purge path for RNDV GET-zcopy: when Related: PR #12149 (this PR,
|
a9227fb to
418987e
Compare
|
🤖 Starting review — findings will be posted here when done. |
418987e to
a9227fb
Compare
|
PR size exceeds the 500-line hard limit (~670 added lines) and mixes two independent changes. Please split: one PR moving Residual coverage gaps: both new multi-device tests still require two real Intel GPUs and skip otherwise, and |
a9227fb to
f61680d
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
The compile-time declaration check (ze.m4) does not imply runtime support: it only proves the installed header declares Residual coverage gaps: both multi-device tests still require two resolvable ZE devices and skip otherwise; |
This PR is stacked on #12090, as the first line of the description says: the per-device queue array and its tests are that PR, open for review. The change here is the last two commits, 217 added lines in 6 files. Once #12090 merges this branch is rebased onto master and the diff shrinks to those two commits. |
Agreed, the declaration check covers the headers, not the loader. The next revision skips the test when the ordinal query returns |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN roce job did not fail on an assertion — the gtest watchdog killed it after a 900-second hang inside Full analysisSummary: gtest aborted (SIGABRT via watchdog Root cause: Timestamp analysis shows a single, exactly-900-second gap in the log: the last application output is Implicated commit: Not this PR's commit ( File: Hang detected at Suggested fix: Re-trigger the roce ASAN job — this failure is not caused by the ZE change and should not block PR #12149. In parallel, open a separate bug for the hang: reproduce with Related: PR #12149 (this build). No existing issue found for
|
|
🤖 Starting review — findings will be posted here when done. |
|
PR size is still ~670 added lines and mixes the Coverage: both multi-device tests need two resolvable ZE devices and skip otherwise, |
|
🤖 CI Triage Agent — TL;DR: The gpu-worker-3 gtest run failed on a single unrelated test, Full analysisSummary: Root cause: Not a hang or timeout — the log shows continuous activity to the end (largest gap ~17 s inside the legitimately slow The failing test posts a Note: the Azure console log is truncated to the last ~500 lines, so the test's own assertion text was not retrievable — the above is derived from the test source and the fact that only Relevance to this PR: the branch Implicated commit: none — the PR commit File: Suggested fix:
Related: PR #12149 (#12149) — the PR under test; related ZE work in #12090. No existing issue found tracking |
4f533d2 to
db39492
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gap (unchanged, no new comment): there is still no ZE job in |
db39492 to
73a1e85
Compare
|
🤖 Starting review — findings will be posted here when done. |
| ze_device_handle_t device = NULL; | ||
| ze_result_t ret; | ||
|
|
||
| ret = zeMemGetAllocProperties(ze_md->ze_context, address, &props, &device); |
There was a problem hiding this comment.
this adds 1-2 zeMemGetAllocProperties() calls per operation on the data path, also for put_short/get_short. can we cache the address→queue result instead of querying the driver per copy (uct_cuda_copy_get_mem_type() goes through ucs_memtype_cache_lookup() first, and ze_copy_md already updates that cache)?
|
🤖 CI Triage Agent — TL;DR: The ASAN RoCE gtest run failed on a single test, Full analysisSummary: Root cause: Evidence from the log:
Caveat: the fetched log is truncated to the last ~500 lines, so the actual assertion text for Implicated commit: unknown — no commit in the PR ([REDACTED:Hex High Entropy String], branch File: Suggested fix: Re-trigger the "AddressSanitizer roce on worker 1" job — this is a single flaky HW-atomics stress test unrelated to the ZE change, and should not block #12149. If it reproduces, get the full (untruncated) job log to capture the assertion text, which distinguishes the two failure modes: a Related: PR #12149 (the PR under test, #12149); no existing issue found tracking
|
ze_copy created its command queues and lists on queue group 0. On the tested devices that is the compute group, so ze_copy copies ran on the compute engine, next to the application's kernels. Copies to a peer or to host should run on the copy engine, as the driver recommends. Move the queue group lookup from ze_ipc to ze_base and use it wherever ze_copy creates a queue, so the queues of other devices, created on first use, get a dedicated copy group too when the device has one, or else any copy-capable group. Add a test which checks every queue of the iface, including one created for another device. Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
Routing all ze_copy transfers through copy engines also moved same-device device-to-device copies off the compute path, slowing some cache-resident transfers. Select a queue group with both COMPUTE and COPY flags when both buffers are device memory of the same device. Keep host, shared, and cross-device copies on the copy-engine path. On the tested devices this restores the earlier engine choice, with the trade-off that same-device copies can contend with application kernels. Key queue entries by device and engine, keeping entry 0 on the MD device's copy engine. Remove the single-device shortcut so buffer properties are checked even when only one device is visible. Share queue-group discovery between the compute and copy paths while preserving the dedicated-copy preference. Log queue-group selection without '=' and skip ordinal checks in tests when the Level Zero driver does not implement the query. Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
Use UCT_ZE_FUNC for both queue-group queries instead of duplicating error checking and logging. Remove the test-only sub-device count getter; the test now enumerates sub-devices until the lookup returns NULL. Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
…once Signed-off-by: Yaser Afshar <yaser.afshar@intel.com>
73a1e85 to
906b7c2
Compare
|
🤖 Starting review — findings will be posted here when done. |
| * locally, or else on the owner of the destination. A queue on an | ||
| * unrelated device has to reach both buffers over PCIe. */ | ||
| device = uct_ze_copy_iface_get_mem_device(iface, src); | ||
| device = uct_ze_copy_iface_get_mem_device(iface, src, &src_is_device); |
There was a problem hiding this comment.
With the single-device fast path gone, every put/get now does up to two zeMemGetAllocProperties() calls, including on single-GPU nodes where get_queue() used to return immediately with zero driver calls. The dst lookup is also needed for the same-device/compute-engine decision, so it can't be avoided by reordering. Can we resolve this more cheaply, e.g. cache the last resolved pointer -> (device, is_device) in the iface, similar to how uct_cuda_copy_get_mem_type() tries ucs_memtype_cache_lookup() before falling back to a driver query (uct_cuda_copy_md_mem_query())?
What?
Create the ze_copy command queues and command lists for copies to host memory and to another device on a dedicated copy engine queue group when the device has one, or else on any copy-capable group, instead of the default group 0. Copies within one device run on a queue group with both the compute and the copy flag, where the driver also places them. Move the queue group lookup used by ze_ipc into
ze_base, so both transports share it.Why?
ze_copy creates its queues and lists with a zeroed descriptor, so they use queue group 0. On the tested devices, group 0 is the compute group, so every ze_copy submission runs on the compute engine, alongside the application's kernels. Copies to a peer or to host should run on the copy engine, as the driver recommends. ze_ipc already does this. Copies within one device are different: the driver keeps its own on the compute engine, which can be faster for cache-resident same-device copies, so ze_copy keeps them there as well.
How?
uct_ze_base_get_copy_ordinal()is the formeruct_ze_ipc_find_copy_ordinal(), moved toze_base. It returns a dedicated copy group (COPY without COMPUTE) when the device has one, otherwise any group that can copy. Compared to the ze_ipc version it queries the group count first and allocates that many entries instead of a fixed array of 16, accepts a NULL queue-count pointer, and takes the log level for its failure messages: error fromze_ipcand fromze_copyiface init, debug from the lazy per-device queue creation.uct_ze_base_get_compute_ordinal()returns the first group with both the COMPUTE and the COPY flag, since the queues on it run memory copies. Both lookups share one group query; the copy lookup keeps its dedicated-copy preference.uct_ze_copy_iface_get_queue()queries the allocation properties of both buffers. When source and destination are device memory of the same device, the copy runs on that device's compute queue, created on first use like the per-device copy queues of UCT/ZE/COPY: Run copies on the device which owns the buffer #12090; shared memory does not count, as it may reside on the host. Every other copy keeps UCT/ZE/COPY: Run copies on the device which owns the buffer #12090's rule: the copy queue of the device owning the source, else of the one owning the destination. Queue entries are keyed by device and engine; entry 0 stays the copy queue of the MD device. The single-device shortcut of UCT/ZE/COPY: Run copies on the device which owns the buffer #12090 goes away, as the same-device check needs the allocation properties anyway.uct_ze_copy_iface_queue_create(), the single place where UCT/ZE/COPY: Run copies on the device which owns the buffer #12090 creates queues, and setcq_desc.ordinalandcl_desc.commandQueueGroupOrdinalfor the engine of the entry, so each list is submitted to the group of its queue. When a lookup or the queue creation fails on the lazy path, the copy keeps using the MD device queue, as in UCT/ZE/COPY: Run copies on the device which owns the buffer #12090. At iface init such a failure fails the iface; a device without a copy-capable group was not known to work on group 0 either.test_ze_copy_rma.queue_on_copy_enginefirst triggers the creation of a queue for a second device (when there is one), then for every queue of the iface reads the queue and list ordinals withzeCommandQueueGetOrdinal/zeCommandListGetOrdinaland checks that they match and that the group is the one the entry's engine asks for: a copy group, dedicated when the device has one, or a group with compute and copy. Newsame_device_copy_on_compute_enginechecks that copies to and from host stay on the copy queue, that a copy between two device buffers selects a compute queue of that device in both directions, moves data through it with a put and with a get, and checks its group;shared_memory_copy_on_copy_enginechecks that shared memory stays on the copy queue.queue_follows_buffer_devicealso moves data between two buffers of the second device through its compute queue and checks that queue's group. The ordinal queries are Level Zero API 1.9: configure checks for them (AC_CHECK_DECLSinze.m4), and the tests skip when the headers do not declare them or the driver does not implement them. When the MD context does not resolve the other context's memory, the tests check the MD device queue alone.Trade-off
Copies to a peer or to host are equal or faster on the copy engine and no longer compete with the application's kernels on the compute engine. Same-device copies keep sharing the compute engine with the kernels, as before this PR. On the tested devices the group selected for them is the group ze_copy used before, so their performance is unchanged; moving them to the copy engine would slow cache-resident same-device copies of a few MB. On one of the tested devices, small same-device copies have lower latency on the copy engine; choosing the engine per size would need device-specific thresholds and is not done here.
Testing
UCX_LOG_LEVEL=error:*ze_copy*:*ze_cpy*:*ze_ipc*gtests pass (67/67; 65/65 with the copy-engine commits alone, 64/64 on UCT/ZE/COPY: Run copies on the device which owns the buffer #12090 alone). Without that variable the same run has one failure,ze_cpy/test_md.sys_device/0, which fails identically on master:ze_copywarns when it limits two visible devices to one for the memtype endpoint, and gtest fails any test that logs a warning. The warning is unrelated to this change. The new tests report, on both cards, the same-device queue on a group with compute and copy and the copy queues on a dedicated copy group. Built against UCT/ZE/COPY: Run copies on the device which owns the buffer #12090 without this change,queue_on_copy_enginefails: the queues are on group 0 (COPY|COMPUTE).PrintDeviceAndEngineIdOnSubmission): with UCT/ZE/COPY: Run copies on the device which owns the buffer #12090 alone, every ze_copy submission goes to the compute engine of the device running the copy; with this change, copies to host and between devices go to that device's copy engine and copies within one device to its compute engine.--without-gda;*cuda_copy*:*cuda_ipc*and the UCP memtype/proto gtests give the same results with and without this change.