Repository navigation
UCT/CUDA: Prepare GDRCopy support for cudaMallocAsync - #12092
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The only failure in this 8792-test run was the timing-sensitive unit test Full analysisSummary: Root cause: For the Implicated commit: Not the PR commit. The flaky assertion dates to File: Suggested fix:
Related: PR #12092 (openucx/ucx) — "UCT/CUDA: Prepare GDRCopy support for cudaMallocAsync" (the PR under test, not the cause); assertion introduced in PR #8918 "UCS/PROFILE: added ucx env to profiling output". |
|
🤖 Starting review — findings will be posted here when done. |
a0f3244 to
bf81ba5
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN gtest shard aborted because Full analysisSummary: Root cause: The test started at 11:57:24.6 and the fatal fired at 11:57:56.2 — a 31.6 s silent interval that exactly matches the UD default
Under AddressSanitizer the gtest binary is heavily slowed (and this is shard 3 of 4 sharing the same RoCE port), which makes a 30 s peer timeout easy to exceed. Nothing in the PR under test (commit a0f3244, which only touches Implicated commit: unknown — not the PR commit a0f3244 (gdr_copy only). If this recurs on File: Suggested fix:
Related: PR #12092 (the PR under test, unrelated change); #11920 / #11948 "UCT/GTEST: Hardening of UD stale private EP tests" (precedent for hardening UD tests against timeouts); #11845 (UD/IB AH cache) |
|
Residual coverage gaps: |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Root cause: The
PR #12092 (GDRCopy / Implicated commit: unknown — no code commit implicated; failure is environmental (faulty File: Suggested fix:
Related: PR #12092 (the PR under test, not the cause); no existing issue found for the |
|
🤖 Starting review — findings will be posted here when done. |
|
Residual coverage gaps: The two |
|
🤖 CI Triage Agent — TL;DR: The gtest run aborted because Full analysisSummary: Root cause: Evidence chain from the log:
The real failure is #1 (UMR QP stall on the RoCE device under ASan); #3/#4 are a test-harness defect that converts a recoverable transport error into a fatal crash of the whole gtest binary. Nothing in this path touches the CUDA/GDRCopy code changed by PR #12092, so the PR looks like an innocent bystander — but the crash is fully reproducible as a job failure whenever the UMR bind times out. Implicated commit: unknown — no recent commit in File: Suggested fix:
Related: PR #12092 (build under test, unrelated change); PR #11958 (rcache region refcount/merging work) — no existing issue found for the
|
Prepare guarded runtime/backend/driver checks for async GDRCopy and honor quiet registration error requests. Do not define async memory/copy capability flags, advertise support, or change transport selection.
Reuse shared error-log capture and qualification constants, and group GDRCopy test flags with the existing CUDA settings.
Handle a missing dlerror message before formatting the debug diagnostic, and keep the registration-error test declarations separate from assignment.
a3349d7 to
80c4036
Compare
|
🤖 Starting review — findings will be posted here when done. |
|
Inconsistent with the existing optional-API pattern: the whole Residual coverage gaps: the new |
Log failed CUDA driver-version queries at error level and diagnose optional GDRCopy backend-query lookup failures without changing support decisions. Keep registration flags consistent with the internal helper and restrict the v2-pin declaration guard to tests that depend on it. Signed-off-by: Nathan Bellalou <nbellalou@nvidia.com>
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The ASAN GPU job failed because exactly one of 10,398 gtests failed — Full analysisSummary: Root cause: From the log ( That test ( Crucially, nothing in this path is touched by the PR. The four commits on Caveat: the Azure log retained only the last ~500 lines, so the per-test assertion text for Implicated commit: none — not caused by File: Suggested fix: Do not change PR code — re-run the "AddressSanitizer gpu on worker 3" job; a single unrelated MEMIC test failure out of 10,398 is an infra flake. If it reproduces, grab the full (untruncated) log to get the assertion text, and harden the test the same way its history suggests: widen the retry condition in Related: PR #12092 (this PR, "UCT/CUDA: Prepare GDRCopy support for cudaMallocAsync"); prior hardening of the same test:
|
Declare test variables near use and construct the cleanup handle after a successful allocation assertion. Apply the formatting changes reported by CI.
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The Full analysisSummary: Step "Build for tests" failed with exit code 243 at the very first Root cause: Build and using
Implicated commit: unknown — no UCX source commit is implicated. The breakage is in the CI container image / File: contrib/test_jenkins.sh:969-981 ( Suggested fix:
Related: PR #12092 (the PR under test, "UCT/CUDA: Prepare GDRCopy support for cudaMallocAsync" — not the cause). No existing issue found for the |
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The "gpu prebuild" job died because Full analysisSummary: Stage "Build for tests" of job Tests gpu prebuild failed with Root cause: Not a code/PR problem. The build and Open MPI's rsh/ssh PLM component (used even for single-node launches) searched its default agent list Because PR #11896 ( Implicated commit: 390a30e — Alexey Rivkin, "AZP/TESTS: Build once per GPU test row and share the tree with the workers (#11896)" (exposes the issue; the actual defect is the container image / File: Suggested fix: All CI MPIRUN_COMMON="mpirun \
--allow-run-as-root \
--bind-to none \
-mca plm isolated \
-x UCX_ERROR_SIGNALS \
-x UCX_HANDLE_ERRORS"Additionally/alternatively: (a) install Related: PR #11896 (introduced the shared GPU prebuild job); PR under test: #12092 |
|
🤖 Starting review — findings will be posted here when done. |
| dlsym(plugin.get(), "uct_gdr_copy_cuda_async_detect")); | ||
| ASSERT_NE(nullptr, detect); | ||
|
|
||
| #if HAVE_DECL_GDR_GET_ATTRIBUTE && HAVE_DECL_GDR_ATTR_USING_DMA_BUF_MMAP |
There was a problem hiding this comment.
In cuda_async_qualification, the #if HAVE_DECL_GDR_GET_ATTRIBUTE && HAVE_DECL_GDR_ATTR_USING_DMA_BUF_MMAP block recomputes exactly the same version/attribute/driver checks as uct_gdr_copy_cuda_async_detect() (major/minor >= 2.6, gdr_get_attribute present, cuDriverGetVersion >= UCT_GDR_COPY_CUDA_ASYNC_MIN_DRIVER_VERSION, GDR_ATTR_USING_DMA_BUF_MMAP) and asserts the two agree. The test therefore passes for any logic error in the qualification (wrong version threshold, inverted condition, wrong attribute) and only detects a link/dlsym break — regression tests should fail without the fix, and this duplicates the implementation. Can we assert on observable MD behavior instead of comparing the implementation to a copy of itself?
|
Residual coverage gap: the Residual coverage gap: the two |
|
🤖 CI Triage Agent — TL;DR: The "gpu prebuild" stage died in Full analysisSummary: Step "Build for tests" of job gpu prebuild (build 140355) failed with Root cause: ORTE's rsh PLM is the only launcher available and neither Implicated commit: Not a code regression in the PR. Environment/image side (no File: Suggested fix:
Related: PR #11896 ( |
| void *address; | ||
| ucs::handle<void*> buffer; | ||
|
|
||
| const int ret = posix_memalign(&address, REG_SIZE, REG_SIZE); | ||
| if (ret == 0) { | ||
| buffer.reset(address, free); | ||
| } | ||
|
|
||
| ASSERT_EQ(0, ret); |
There was a problem hiding this comment.
Maybe coverity will accept something like this:
| void *address; | |
| ucs::handle<void*> buffer; | |
| const int ret = posix_memalign(&address, REG_SIZE, REG_SIZE); | |
| if (ret == 0) { | |
| buffer.reset(address, free); | |
| } | |
| ASSERT_EQ(0, ret); | |
| void *address = nullptr; | |
| const int ret = posix_memalign(&address, REG_SIZE, REG_SIZE); | |
| ucs::handle<void*> buffer(address, free); | |
| ASSERT_EQ(0, ret); |
|
🤖 Starting review — findings will be posted here when done. |
| #endif | ||
| } | ||
|
|
||
| int uct_gdr_copy_cuda_async_detect(gdr_t gdr_ctx) |
There was a problem hiding this comment.
uct_gdr_copy_cuda_async_detect() has no caller in the library — only the gtest reaches it via dlsym. Nothing in uct_gdr_copy_md_query(), mem_reg, or the iface uses it, so the PR adds an exported detection helper plus its whole dladdr/dlopen/dlsym support path with no behavior change (dead code in a shipped module). Can we land it together with the code that consumes the result?
| dlsym(plugin.get(), "uct_gdr_copy_cuda_async_detect")); | ||
| ASSERT_NE(nullptr, detect); | ||
|
|
||
| #if HAVE_DECL_GDR_GET_ATTRIBUTE && HAVE_DECL_GDR_ATTR_USING_DMA_BUF_MMAP |
There was a problem hiding this comment.
cuda_async_qualification recomputes expected with the same runtime-version, gdr_get_attribute, and driver-version checks as the function under test, so the test would still pass if the threshold (e.g. 13050), the comparison, or the ordering were wrong — it only checks the function does not crash. Can we assert a value that does not mirror the implementation?
| uct_gdr_copy_mem_reg(uct_md_h uct_md, void *address, size_t length, | ||
| const uct_md_mem_reg_params_t *params, uct_mem_h *memh_p) | ||
| { | ||
| unsigned flags = UCT_MD_MEM_REG_FIELD_VALUE(params, flags, FIELD_FLAGS, 0); |
There was a problem hiding this comment.
| unsigned flags = UCT_MD_MEM_REG_FIELD_VALUE(params, flags, FIELD_FLAGS, 0); | |
| uint64_t flags = UCT_MD_MEM_REG_FIELD_VALUE(params, flags, FIELD_FLAGS, 0); |
minor: params->flags is uint64_t, and uct_gdr_copy_mem_rcache_reg() in this file already uses uint64_t.
| ASSERT_UCS_OK(register_mem()); | ||
| } | ||
|
|
||
| #endif /* HAVE_DECL_GDR_PIN_BUFFER_V2 */ |
There was a problem hiding this comment.
minor: the two new tests are inserted between two #if HAVE_DECL_GDR_PIN_BUFFER_V2 blocks, creating a second identical guard block in the same fixture; pls move the two new tests below the existing ones so one guard block is enough.
|
Residual coverage gaps: CI loads |
What?
First of three PRs to enable the
gdr_copytransport for buffers allocated withcudaMallocAsync.This PR adds prerequisite detection and quiet registration-error handling. It does not enable async GDRCopy or change protocol/transport selection. The following PRs will add the dormant short-copy path, then enable it only on supported configurations.
Why?
internal issue
The feature needs to distinguish a supported GDRCopy runtime/backend/driver combination from one that cannot CPU-map stream-ordered CUDA allocations. It also needs registration attempts to honor the existing request to suppress error-level logging, before adding a registration-time fallback.
Keeping this groundwork separate lets it be reviewed without introducing new buffer classification or routing decisions.
How?
gdr_get_attributesymbol from the library that suppliedgdr_open, rather than assuming that newer build headers imply a newer runtime. Missing declarations, symbols, or failed queries return unsupported.UCT_MD_MEM_FLAG_HIDE_ERRORS; ordinary failures retain error-level logs. Return codes and cleanup are unchanged.Scope and performance
The qualification helper has no production caller in this PR: normal initialization and copying do not invoke the new probes. There are no UCP changes, async allocation markers, new advertised capabilities, configuration knobs, protocol-cost changes, or changes to copy operations.
This is preparatory for selection and the copy fast path, not feature activation. It is not a complete runtime no-op: honoring the existing quiet-error flag is an intentional logging fix. No latency/throughput benchmarks or measured zero-overhead claim are included.
Validation
cuda_async_qualificationandhide_registration_error, both executed and passed.master.