UCT/CUDA_IPC: Publish the chunk layout of multi-allocation VMM ranges - #11991
tomerg-nvidia wants to merge 1 commit into
Conversation
|
🤖 Starting review — findings will be posted here when done. |
797a36c to
74f4788
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
Coverage gaps: the new gtest only covers the exporter side on a fabric-capable GPU (skipped otherwise); there is no coverage for the discover/export failure paths (non-fabric multi-chunk memory) or for the importer, which does not exist yet. |
|
🤖 CI Triage Agent — TL;DR: The Coverity Full analysisSummary: Azure Pipelines job "Coverity coverity devel on coverity_rh7" (build 137279) ended with Root cause: Coverity's CHECKED_RETURN checker flagged the newly added cleanup path in the multi-allocation VMM test helper: Coverity's supporting evidence cites 164: void cleanup()
165: {
166: for (size_t i = 0; i < m_num_mapped; ++i) {
167: cuMemUnmap(m_ptr + (i * m_chunk_size), m_chunk_size);
168: }
169:
170: for (auto alloc_handle : m_alloc_handles) {
171: cuMemRelease(alloc_handle); /* <-- flagged */
172: }This is a real (if benign) code-quality finding introduced by this PR, not infrastructure flakiness — no timeouts, SIGTERM or log gaps appear anywhere in the run. Implicated commit: [REDACTED:Hex High Entropy String] — "UCT/CUDA_IPC: Publish the chunk layout of multi-allocation VMM ranges", Tomer Gilad (the only commit touching this file in this PR; it introduced the multi-handle File: test/gtest/uct/cuda/cuda_vmm_mem_buffer.h:171 (loop starting at :170; also see unchecked Suggested fix: Capture and check the driver return value in for (auto alloc_handle : m_alloc_handles) {
CUresult ret = cuMemRelease(alloc_handle);
if (ret != CUDA_SUCCESS) {
UCS_TEST_MESSAGE << "cuMemRelease() failed: " << ret;
}
}Alternatively wrap the call with the existing Related: PR #11991 (openucx/ucx), branch
|
74f4788 to
82eac5d
Compare
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| /* Peers may unpack any published metadata until memh deregistration. */ | ||
| ucs_list_add_tail(&key->vmm_multi_list, &meta->link); |
There was a problem hiding this comment.
Unbounded metadata accumulation: every range not covered by an existing record adds another metadata buffer of at least the allocation granularity (2MB on most GPUs) that lives until dereg, so repeated packs of growing ranges on the same memh keep consuming device memory. Is there a bound, or can the record be published once for the whole registered region?
|
Residual coverage gap (unchanged from the earlier note): the new gtest exercises only the exporter on a fabric-capable GPU (skipped otherwise); the discover/export failure paths (multi-chunk range with a non-fabric chunk) and the importer are untested, and |
82eac5d to
52b8183
Compare
|
🤖 Starting review — findings will be posted here when done. |
| } | ||
|
|
||
| /* Peers may unpack any published metadata until memh deregistration. */ | ||
| ucs_list_add_tail(&key->vmm_multi_list, &meta->link); |
There was a problem hiding this comment.
Each published record pins a granularity-sized (>=2MB) device allocation until mem_dereg(), and since a newer range never supersedes an older one, a growing sequence of ranges on the same key accumulates several of them. Can we reuse one buffer sized for the largest range so far, or is the assumption at most one range per key?
A VMM range backed by several physical allocations cannot be described by the one allocation handle a key carries today. Add the exporter side of a multi-chunk key: discover the allocations behind a range, write their descriptors to a GPU buffer shared by fabric handle, and keep that buffer on the local key until deregistration. Nothing packs such a key yet; the pack path is wired up separately.
52b8183 to
4b7579e
Compare
|
🤖 Starting review — findings will be posted here when done. |
| if (!(allowed_handle_types & CU_MEM_HANDLE_TYPE_FABRIC)) { | ||
| ucs_debug("VMM chunk 0x%llx does not allow fabric handles", | ||
| chunk_base); | ||
| status = UCS_ERR_UNSUPPORTED; |
There was a problem hiding this comment.
This makes "chunk is not fabric-exportable" indistinguishable from the "single allocation" meaning documented for uct_cuda_ipc_mkey_pack_vmm_multi_chunk(), so a caller falling back to the plain pack would publish a key covering only the first chunk. Can we return a different status here (or document both cases)?
| * @return UCS_OK on success, UCS_ERR_UNSUPPORTED for a single allocation, or | ||
| * another error status on failure | ||
| */ | ||
| ucs_status_t uct_cuda_ipc_mkey_pack_vmm_multi_chunk( |
There was a problem hiding this comment.
minor: pls rename to uct_cuda_ipc_vmm_multi_mkey_pack() for consistency with the other uct_cuda_ipc_vmm_multi_* functions in this file.
What?
A VMM range backed by several physical allocations cannot be described by the one allocation handle a key carries today. Add the exporter side of a multi-chunk key: discover the allocations behind a range, write their descriptors to a GPU buffer shared by fabric handle, and keep that buffer on the local key until deregistration.
Nothing packs such a key yet, the pack path is wired up separately.
Why?
A step in fixing multi-allocation VMM transfers with cuda_ipc. Currently they are not working.
How?
During pack, if a multi-allocation VMM is detected (currently not wired but will be in a followup PR):
Note: This PR contains only the pack-side functions, and does not wire them in yet. A new handle type will be introduced for it when it is wired up.