Repository navigation
Conversation
|
🤖 Starting review — findings will be posted here when done. |
|
🤖 CI Triage Agent — TL;DR: The build (not the tests) failed while linking Full analysisSummary: Root cause: Pure link-time environment breakage, not a code defect:
Implicated commit: unknown — not caused by commit [REDACTED:Hex High Entropy String] ( File: Suggested fix:
Related: none found — no existing issue/PR in this repo matches
|
| UCS_MEM_FLAG_GDR_CAPABLE = UCS_BIT(2), | ||
|
|
||
| /** Memory originates from a CUDA stream-ordered allocation. */ | ||
| UCS_MEM_FLAG_CUDA_ASYNC = UCS_BIT(3) |
There was a problem hiding this comment.
nothing in the tree sets this flag, and no MD sets UCT_MD_FLAG_CUDA_ASYNC_MEMTYPE_COPY either, so the whole new route is unreachable except in the gtest that patches md_attr directly. are the producers (cuda_copy mem-flag detection and gdr_copy md_query using the existing uct_gdr_copy_cuda_async_detect()) planned for a follow-up PR?
| ucs_status_t status; | ||
|
|
||
| if ((mem_info == NULL) && | ||
| (ucp_worker_cuda_async_ep_for_short(worker, &detected, operation, |
There was a problem hiding this comment.
this probe (the mem_info == NULL probe) passes only the fabricated UCS_MEM_FLAG_CUDA_ASYNC, so an MD whose required_mem_flags also contain e.g. UCS_MEM_FLAG_REGISTRABLE never passes ucs_test_all_flags() here, and detection (hence the async route) is silently skipped for it. why not probe with all mem flags set and let the second call do the real flag matching?
| } | ||
|
|
||
| async_ep = ((mem_info == NULL) || | ||
| !(mem_info->flags & UCS_MEM_FLAG_CUDA_ASYNC)) ? NULL : |
There was a problem hiding this comment.
minor: ucp_worker_cuda_async_ep_for_short() already filters by mem type and by UCS_MEM_FLAG_CUDA_ASYNC, so the extra flag test here is redundant - (mem_info == NULL) ? NULL : ucp_worker_cuda_async_ep_for_short(...) is enough.
| ucs_align_ptr_range(®_address, ®_length, | ||
| ucp_ep_md_attr(async_ep, lane)->reg_alignment); | ||
| status = ucp_mem_type_reg_buffers( | ||
| worker, reg_address, reg_length, UCS_MEMORY_TYPE_CUDA, |
There was a problem hiding this comment.
why register as UCS_MEMORY_TYPE_CUDA when the buffer may be CUDA_MANAGED? if this md is in context->cache_md_map[], ucp_mem_type_reg_buffers() goes through ucp_memh_get(), and ucp_memh_find_slow() asserts the cached region mem_type equals the requested one - a managed buffer already cached as CUDA_MANAGED would trip that assert.
|
|
||
| if (worker->mem_type_ep[local_mem_type] != NULL) { | ||
| ep_config = ucp_ep_config(worker->mem_type_ep[local_mem_type]); | ||
| copy_mem_info = local_mem_info; |
There was a problem hiding this comment.
copy_mem_info follows whichever side happens to have a mem_type ep, so for the mtype "local copy" (cuda frag vs cuda user buffer) the frag flags win and the user buffer's async flag is dropped: the estimate keeps rma_lanes[0] while the runtime copy takes the async lane. should the async check consider both mem_infos?
| m_counts{0, 0} | ||
| { | ||
| ucs_assert(m_active == nullptr); | ||
| ucs_assert(m_ifaces[0]->internal_ops != m_ifaces[1]->internal_ops); |
There was a problem hiding this comment.
ucs_assert() is compiled out in release builds, and then m_orig[1] gets the hook itself if both ifaces share internal_ops, so estimate_perf() recurses forever. pls make this a real check in the test (skip/fail) rather than an assert, and consider copying internal_ops and swapping the pointer instead of writing into the per-TL table shared by all ifaces of that transport.
| auto saved = ucs_global_opts.handle_errors; | ||
| ucs_global_opts.handle_errors = 0; | ||
| if (operation == UCT_EP_OP_GET_SHORT) { | ||
| EXPECT_DEATH(ucp_mem_type_pack(worker(), buffer, buffer, 64, |
There was a problem hiding this comment.
this is the first death test in the gtest suite; EXPECT_DEATH forks a child that already has an initialized CUDA context and open UCT ifaces, which is not supported and tends to hang or die with an unrelated message. can we assert the "no retry" behavior without killing the process (e.g. check lane/ep selection and registration), or drop these two cases?
|
Residual coverage gap: No test exercises the async route end to end with a real stream-ordered allocation, because no MD advertises the new capability; the new tests only validate the selection helper and the perf model with hand-patched Residual coverage gap: Note (not flagged): |
What?
Second of three PRs to enable
gdr_copyfor buffers allocated withcudaMallocAsync, following merged #12092.This PR adds the complete short-copy routing and registration path, but leaves it dormant in normal use. The third PR will connect allocation detection, runtime qualification and the enable/disable setting.
Why?
Stream-ordered CUDA allocations need a targeted route to GDRCopy's DMA-BUF mapping path without making other transports newly eligible or changing the buffer's outward memory type. Preparing the complete path before activating it keeps this PR safe to merge on its own.
How?
GDR_CAPABLEon that qualified route.Scope
No production allocation detector emits the new marker, and no MD advertises the new capability in this PR. The qualification helper from #12092 still has no production caller. There is no new configuration setting, global reclassification, or change to other transports' eligibility.
The path can be exercised by tests that inject the marker and capability, but it does not enable async GDRCopy for applications. No measured zero-overhead or latency/throughput claim is made.
Validation