Repository navigation
Conversation
An element of an SGL put or get may be of zero length. Such an element transfers no data, while the other elements of the SGL are transferred. Check it on the transports that allow zero-length zcopy operations. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
cuMemcpyBatchAsync fails with an invalid argument error for a copy of zero length, so an SGL put or get with a zero-length element failed with UCS_ERR_IO_ERROR. Leave such elements out of the batch, and take the CUDA context and the stream from the first element with data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Starting review — findings will be posted here when done. |
| void test_sgl_zero_length_element() | ||
| { | ||
| static const size_t count = 3; | ||
| static const size_t empty_index = 1; |
There was a problem hiding this comment.
pls cover a zero length in the first element too (e.g. loop the index over 0..count-1) - the new first selection in cuda_ipc only matters when lengths[0] == 0, and with empty_index = 1 it is never exercised.
There was a problem hiding this comment.
Added zero_length_first_last_elements with {0, 2} in eefee38: the first element with data is element 1, and the last element is empty too. I did not loop over the index, to keep one named case per scenario; with {1} and {0, 1, 2}, each position is empty in some test. All elements in the test have valid buffers and rkeys on the same GPU, so the test runs the first > 0 path and checks the data, but it would also pass if the context and the stream were taken from element 0.
🤖 Generated with Claude Code
| mapping = ucs_malloc(sizeof(*mapping) + | ||
| count * (sizeof(*mapping->entries) + | ||
| sizeof(CUdeviceptr)), | ||
| (2 * sizeof(CUdeviceptr)) + sizeof(size_t)), |
There was a problem hiding this comment.
maybe keep the common case cheap: when no length is zero we could pass buffers/lengths directly as before, and skip allocating and filling the two extra arrays per element.
There was a problem hiding this comment.
Done in b661636: the sum loop also counts the elements with data. If none is empty, buffers and lengths are passed to cuMemcpyBatchAsync as on master, and the allocation has the same size as on master. Only if some element is empty, the allocation also has room for the compacted local addresses and lengths, which a separate loop fills.
An SGL put or get with all elements of zero length transfers no data. Check that it keeps the target buffers unchanged, and that the completion is not called when the operation completes in place. Pass a completion to the test of a single zero-length element as well. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Starting review — findings will be posted here when done. |
| total_length = 0; | ||
| first = count; | ||
| for (i = 0; i < count; i++) { | ||
| if ((first == count) && (lengths[i] != 0)) { |
There was a problem hiding this comment.
minor: maybe find the first non-empty element right after the total_length == 0 check (for (first = 0; lengths[first] == 0; ++first) {})? The sum loop stays branchless and the first = count sentinel is not needed.
There was a problem hiding this comment.
Done in b661636: the first element with data is found right after the total_length == 0 check, the sum loop has no branch, and the first = count sentinel is gone. The search is a while loop rather than a for with an empty body.
An SGL put or get may start or end with an element of zero length. With an empty first element, cuda_ipc takes the CUDA context and the stream from the first element with data, a path that the other tests do not reach. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
If no element of an SGL put or get is of zero length, pass the local buffers and the lengths of the caller to cuMemcpyBatchAsync as before, without allocating and filling compacted copies of them. Find the first element with data after the check for zero total length, and trace the number of copies in the batch instead of the number of elements. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each length of an SGL put or get must not be less than the minimal zcopy length of the interface. A length can be 0 only if that minimum is 0, and such an element transfers no data. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Starting review — findings will be posted here when done. |
| elem_size += sizeof(*local_addrs) + sizeof(*copy_lengths); | ||
| } | ||
|
|
||
| mapping = ucs_malloc(sizeof(*mapping) + (count * elem_size), |
There was a problem hiding this comment.
minor: only num_nonempty elements are ever stored in these arrays, so maybe allocate num_nonempty * elem_size and offset mapped_addrs/local_addrs/copy_lengths by num_nonempty instead of count? The fast path is unchanged since there num_nonempty == count.
There was a problem hiding this comment.
Done in bf15f67: the mapping is allocated for num_nonempty elements, and mapped_addrs, local_addrs and copy_lengths are offset by num_nonempty.
| UCS_TEST_SKIP_R("not enough sgl elements"); | ||
| } | ||
|
|
||
| if (elem_size(0) > 0) { |
There was a problem hiding this comment.
minor: elem_size(0) > 0 as a min_zcopy check is hard to read; can we add a small min_zcopy() helper and use it here and in elem_size()?
There was a problem hiding this comment.
Done in b118b78: added a min_zcopy() helper, used here and in elem_size().
|
Residual coverage gaps (not posted as inline comments): the new tests only exercise zero-length elements with valid buffers/memhs/rkeys — the doc does not state whether those may be invalid for a zero-length element, and |
The rule tied each SGL length to min_zcopy, which uct_iface_attr defines for the total length of the iov of a zcopy operation. Skipping the empty elements in cuda_ipc does not need such a new contract. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🤖 Starting review — findings will be posted here when done. |
Only the elements with data are stored in the arrays of an SGL mapping, so allocate and lay out the arrays for that number of elements instead of for all the elements.
Skip the zero-length tests by the minimal zcopy length itself instead of by the size of an empty element, and use the helper in elem_size() too.
| elem_size += sizeof(*local_addrs) + sizeof(*copy_lengths); | ||
| } | ||
|
|
||
| mapping = ucs_malloc(sizeof(*mapping) + (count * elem_size), |
There was a problem hiding this comment.
minor: can we use num_nonempty instead of count here and in the offsets below? the tail of every sub-array is unused when some elements are empty.
There was a problem hiding this comment.
Done in bf15f67: the mapping is allocated for num_nonempty elements, and the arrays are offset by num_nonempty.
| /* Same elements and order as in the mapping loop below */ | ||
| local_addrs = mapped_addrs + count; | ||
| copy_lengths = (size_t *)(local_addrs + count); | ||
| for (i = 0, j = 0; i < count; i++) { |
There was a problem hiding this comment.
maybe fill local_addrs/copy_lengths inside the mapping loop below instead of here, so the two loops don't have to be kept in the same order? this path already does a ucs_malloc plus a memhandle cache lookup per element, so one extra store per element is not significant (docs/OptimizationStyle.md).
There was a problem hiding this comment.
Done in c3082be: local_addrs and copy_lengths are filled in the mapping loop together with mapped_addrs, so there is one loop over the elements.
|
🤖 Starting review — findings will be posted here when done. |
| /* If some elements are empty, the local addresses and lengths of the | ||
| * non-empty ones are compacted into the same allocation, otherwise the | ||
| * caller arrays are passed to cuMemcpyBatchAsync as is */ | ||
| elem_size = sizeof(*mapping->entries) + sizeof(*mapped_addrs); |
There was a problem hiding this comment.
maybe we could always fill local_addrs/copy_lengths inside the mapping loop at index mapping->count? That would drop the elem_size branch, the extra pre-pass and j, and the "same elements and order as in the mapping loop below" coupling, at the cost of 16 bytes and two stores per element, which look negligible next to the per-element memhandle mapping.
There was a problem hiding this comment.
Done in c3082be: local_addrs and copy_lengths are always allocated with the mapping and filled in the mapping loop at index mapping->count, so the elem_size branch, the separate loop and j are gone. The caller arrays are no longer passed to cuMemcpyBatchAsync as is, which costs two stores and 16 bytes of the mapping per element.
|
The
Pre-existing and out of scope: when |
Store the local address and the length of each element with data in the mapping loop, next to its remote address, instead of compacting them in a separate loop only if some element is empty. This keeps one loop over the elements, at the cost of two stores and 16 bytes of the mapping per element, which is small next to the memory handle lookup per element.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The gtest Full analysisSummary: Root cause: Two layered problems, both visible in the log:
This is unrelated to PR #12219 ( Implicated commit: unknown for the lost completion; most plausible recent candidate is File: Suggested fix:
Related: #11841 (and its v1.23.x backport #12222); fence-path changes #11721, #10658; purge-outstanding series #11866/#11874/#12133. |
Resolve the conflict in cuda_ipc_ep.c with the renames on master: the array of the local addresses of the remote memory is now local_addrs, so the array of the buffer addresses of the elements with data is renamed to buffer_addrs. The comment and the sizeof() terms of the mapping allocation, which merged without a conflict, are updated the same way.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 Review complete — no issues found. |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Azure job "Cuda Test Cuda Docker centos7_cuda_11_4" (build 142791) failed in the Root cause: The build itself completed cleanly (all Implicated commit: unknown — not a code regression; the failure is environmental (GPU agent / MPS state). Test app File: Suggested fix:
Related: PR #12219 (#12219) — failure is unrelated to its change; no existing issue found for CUDA error 807 in CI. |
Count the empty elements before the first element with data in the loop that sums the lengths, instead of searching for that element after the check for zero total length, so that the search does not rely on the total length to stay in bounds.
|
🤖 Starting review — findings will be posted here when done. |
|
/azp run |
|
Azure Pipelines successfully started running 4 pipeline(s). |
UCT/CUDA_IPC: Skip zero-length elements in SGL zcopy
What?
A cuda_ipc
put_sgl_zcopy/get_sgl_zcopywith a zero-length element among non-empty ones fails withUCS_ERR_IO_ERROR. This PR leaves zero-length elements out of the copy batch, so such an SGL transfers the other elements and completes successfully, as on rc_mlx5.Why?
uct_cuda_ipc_post_cuda_sgl_async_copy()skips only an SGL whose total length is zero. Otherwise it passes all the elements tocuMemcpyBatchAsync(), which rejects a copy of zero length:The SGL zcopy API does not forbid zero-length elements, and rc_mlx5 allows them for put.
How?
local_addrs,buffer_addrsandcopy_lengths) in arrays allocated together with the SGL mapping, somapping->countis the number of copies in the batch. The CUDA context and the stream are taken from the first element with data instead of element 0, which may be empty.test_uct_sgl.zero_length_element,zero_length_first_last_elementsandzero_length_all_elements: a 3-element SGL put/get with an empty middle element, with empty first and last elements, and with all elements empty. They check that the other elements are transferred, that the targets of the empty ones keep their data, and that the completion is not called when the operation completes in place. They are skipped where zero-length zcopy is not allowed (min_zcopy > 0, e.g. get on rc_mlx5).Tests:
cuda_ipc/test_uct_sgl.zero_length_elementfails for put and get (Input/output error);rc_mlx5put passes.cuda_ipc/test_uct_sgl.*andrc_mlx5/test_uct_sgl.*passed UCPall/test_ucp_rma_sgl.*passed.cuda_ipc/*UCT tests, with the first version of the fix: all pass excepttest_uct_loopback_cuda.envelope("Destination is unreachable" in the perf endpoint setup), which fails the same way without the fix on this machine.Found while testing #12205 on cuda_ipc.