Skip to content

UCT/CUDA/CUDA_IPC: Release unused cache entry at rkey release. - #12108

Merged
rakhmets merged 9 commits into
openucx:masterfrom
rakhmets:topic/cuda-ipc-cache-release-unused
Oct 29, 2026
Merged

rakhmets merged 9 commits into
openucx:masterfrom
rakhmets:topic/cuda-ipc-cache-release-unused

Conversation

@rakhmets

Copy link
Copy Markdown
Contributor

What?

Release unused cache entry at rkey release.

Why?

An alternative to #11870

Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.c Resolved
@rakhmets
rakhmets force-pushed the topic/cuda-ipc-cache-release-unused branch 2 times, most recently from a54ca4c to a797450 Compare October 19, 2026 12:33
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 19, 2026
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 19, 2026
@rakhmets
rakhmets force-pushed the topic/cuda-ipc-cache-release-unused branch from a797450 to 8ca1782 Compare October 19, 2026 12:42
@rakhmets
rakhmets marked this pull request as ready for review October 19, 2026 12:44
@rakhmets rakhmets added the API label Oct 19, 2026
@rakhmets
rakhmets force-pushed the topic/cuda-ipc-cache-release-unused branch from 8ca1782 to 5726508 Compare October 19, 2026 12:52
@openucx openucx deleted a comment from svc-nvidia-pr-review Oct 19, 2026
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

@rakhmets
rakhmets force-pushed the topic/cuda-ipc-cache-release-unused branch from 5726508 to ecb137c Compare October 19, 2026 13:13
@rakhmets
rakhmets force-pushed the topic/cuda-ipc-cache-release-unused branch from ecb137c to 5ee5af2 Compare October 19, 2026 13:16
Comment thread src/ucp/core/ucp_rkey.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.c Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap: no gtest exercises the new UCT flag or the new cache-release function; validation depends on the CUDA MPI job only.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/uct/api/v2/uct_v2.h Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
Comment thread contrib/test_jenkins.sh Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
shasson5
shasson5 previously approved these changes Oct 22, 2026
@rakhmets

Copy link
Copy Markdown
Contributor Author

/azp run UCX PR

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

tomerg-nvidia
tomerg-nvidia previously approved these changes Oct 22, 2026
@svc-ucx

svc-ucx commented Oct 22, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests gpu prebuild) · commit 5ee5af27

TL;DR: The "Build for tests" step died with exit 243 because Open MPI's rsh launcher could not find an ssh/rsh binary inside the doca-3.1.0-gpunetio GPU container, so the very first mpirun in run_ucx_perftest 1 aborted — unrelated to the PR's code; fix by installing openssh-client in the image or forcing a local launcher (-mca plm isolated / -mca plm_rsh_agent "") in MPIRUN_COMMON.

Full analysis

Summary: UCX PR (Tests gpu prebuild) build 140268 failed in the "Build for tests" step: mpirun aborted at launch with plm_rsh_agent: ssh : rsh "path could not be found", FORCE-TERMINATE AT Not found:-13 - error plm_rsh_component.c(335), and Bash exited with code 243.

Root cause: Environment/infrastructure, not the PR. The build, make install and make installcheck all succeeded (examples compiled cleanly at 16:45:28–29). Immediately afterward run_mpi_tests loaded the hpcx-ga-gcc module (resolving to the custom HPC-X 2.24 tree under /hpc/local/benchmarks/custom_by_michael/hpcx-v2.24-gcc-doca_ofed-ubuntu24.04-cuda13-x86_64), and run_ucx_perftest 1 issued the first real process launch:
mpirun --allow-run-as-root --bind-to none ... -np 2 ... ucx_perftest ...
ORTE's plm_rsh component validates its launch agent at startup; its default value is ssh : rsh, and neither binary exists on the PATH inside the ubuntu2404_doca31_gpunetio container (rdmz-harbor.rdmz.labs.mlnx/hpcx/x86_64/ubuntu24.04/builder:doca-3.1.0-gpunetio), so it force-terminates with Not found:-13 before any UCX code runs. mpirun --version and which mpirun succeeded earlier precisely because they never instantiate the PLM. There is no timeout/hang here — the failure is instantaneous (same-second timestamps). The MPIRUN flags in the test script set pml, btl, osc, coll but never pin a launcher, so this is sensitive to whether the image ships openssh-client.

Implicated commit: unknown (not a code regression; the head commit 5ee5af27, Raul Akhmetshin, "TEST/MPI: Use remote mem list in test_ucp_rkey_destroy." touches contrib/test_jenkins.sh only at lines 1007–1020, which are never reached). The trigger is the GPU container image / HPC-X module environment on node swx-rdmz-ucx-gpu-02.

File: contrib/test_jenkins.sh:969 (MPIRUN_COMMON definition) — failing call site contrib/test_jenkins.sh:668; container definition buildlib/pr/main.yml:66-68.

Suggested fix: Two complementary options:

  1. Make the CI MPI launches self-contained (preferred, robust across images). All UCX CI mpirun usage is single-node, so disable the rsh launcher explicitly:
    MPIRUN_COMMON="mpirun \
            --allow-run-as-root \
            --bind-to none \
            -mca plm isolated \
            -x UCX_ERROR_SIGNALS \
            -x UCX_HANDLE_ERRORS"
    (or -mca plm_rsh_agent "" if plm isolated is unavailable in this HPC-X build).
  2. Add openssh-client to the hpcx/x86_64/ubuntu24.04/builder:doca-3.1.0-gpunetio image and bump the tag in buildlib/pr/main.yml:67, so any OMPI version works unchanged.

Also worth confirming whether the hpcx-ga-gcc module pointing into /hpc/local/benchmarks/custom_by_michael/... is intentional for CI, or a stale module definition on the GPU hosts. Re-run the PR after applying either fix; no UCX source change is needed.

Related: none found (searches for plm_rsh_agent and the gpunetio image returned no matching issues/PRs; only unrelated #12092).

Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/ucp/core/ucp_rkey.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.h Resolved
Comment thread src/uct/api/v2/uct_v2.h Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap: contrib/test_jenkins.sh explicitly does not run test_ucp_rkey_destroy/test_rma_cuda ("they fail on the CI GPU hosts"), so the end-to-end path (UCP rkey destroy → UCT flag → cache close → peer memory actually freed) has no automated coverage; the new gtests cover uct_cuda_ipc_cache_release_unused() in isolation only.

tomerg-nvidia
tomerg-nvidia previously approved these changes Oct 27, 2026
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.h Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/ucp/core/ucp_rkey.c Resolved
@rakhmets
rakhmets force-pushed the topic/cuda-ipc-cache-release-unused branch from 2bd45e8 to 9303c0d Compare October 27, 2026 16:26
@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_cache.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/ucp/core/ucp_rkey.h Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap: the flag path is exercised only by the CUDA MPI tests, and the no-mapping case from finding 1 (rkey unpacked but never mapped, then destroyed) is not covered by any test - a gtest on uct_cuda_ipc_unmap_memhandle() with mapped_addr == NULL and an empty cache would pin it down.

@svc-ucx

svc-ucx commented Oct 27, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests gpu on worker 2) · commit 9303c0d2

TL;DR: cuda_ipc/test_ucp_device.create_remote_fail/1 segfaulted in ucp_rkey_destroy → uct_cuda_ipc_unmap_memhandle because uct_cuda_ipc_cache_destroy_region() dereferences the result of ucs_pgtable_lookup() without checking for NULL — reachable on the new "free unused mapping" release path where the rkey was never mapped (mapped_addr == NULL). Add a NULL check on pgt_region (and skip the cache walk entirely when nothing was mapped).

Full analysis

Summary: gtest cuda_ipc/test_ucp_device.create_remote_fail/1 <rc,cuda_ipc,cuda_copy,rocm_copy/cuda> crashed with SIGSEGV (address not mapped to object at address 0xf8), aborting make test with "Segmentation fault (core dumped)".

Root cause: Backtrace: ucp_rkey_destroy → uct_cuda_ipc_rkey_release → uct_cuda_ipc_unmap_memhandle+0x240 (inlined uct_cuda_ipc_cache_destroy_region). The fault address 0xf8 (=248) is exactly the byte offset of uct_cuda_ipc_cache_region_t::refcount (pgt super 16 + list 16 + lru_list 16 + uct_cuda_ipc_rkey_t + mapped_addr), i.e. region was NULL.

In uct_cuda_ipc_cache_destroy_region() (src/uct/cuda/cuda_ipc/cuda_ipc_cache.c:644-668), ucs_pgtable_lookup() is only asserted non-NULL when mapped_addr != NULL; afterwards region->refcount is dereferenced unconditionally. That NULL path is newly reachable: uct_cuda_ipc_rkey_release_unmap_memhandle() (cuda_ipc_md.c:646-659) no longer returns early when rkey_handle->mapped_addr == NULL if free_unused_mapping is set (UCT_RKEY_UNPACK_FLAG_FREE_UNUSED_MAPPING). In this test the rkey is unpacked for a peer whose accessibility is already cached as UCS_YES, so uct_cuda_ipc_is_peer_accessible() never calls uct_cuda_ipc_map_memhandle() and mapped_addr stays NULL — but a per-peer cache for {pid, pid_ns, cu_dev} does exist (created by an earlier mapping on the other device), so the code proceeds to a page-table lookup for a d_bptr that was never inserted, gets NULL, and crashes. Note the local-rkey shortcut at cuda_ipc_cache.c:681 also fails to trigger because it compares d_bptr == (uintptr_t)mapped_addr, which is false when mapped_addr is NULL.

Implicated commit: a1fb590 "UCT/API/V2: Added flag to free unused mappings at rkey release" (Raul Akhmetshin), carried by 81170eb / 9303c0d on this branch

File: src/uct/cuda/cuda_ipc/cuda_ipc_cache.c:649-660 (crash site region->refcount, line 656/660); trigger at src/uct/cuda/cuda_ipc/cuda_ipc_md.c:646-659

Suggested fix:

  1. Harden uct_cuda_ipc_cache_destroy_region():
pgt_region = UCS_PROFILE_CALL(ucs_pgtable_lookup, &cache->pgtable, d_bptr);
if (pgt_region == NULL) {
    /* nothing was mapped for this rkey (e.g. peer accessibility was
     * already cached) - nothing to release */
    ucs_assert(mapped_addr == NULL);
    pthread_rwlock_unlock(&cache->lock);
    return;
}
region = ucs_derived_of(pgt_region, uct_cuda_ipc_cache_region_t);
  1. Also fix the local-rkey shortcut in uct_cuda_ipc_unmap_memhandle() so it covers the unmapped case:
    if (((mapped_addr == NULL) || (d_bptr == (uintptr_t)mapped_addr)) && uct_cuda_ipc_is_rkey_local(pid, pid_ns)) return;
  2. Optionally keep uct_cuda_ipc_rkey_release_unmap_memhandle() returning early when mapped_addr == NULL, since with no mapping there is nothing for free_unused_mapping to free.

Related: PR #12108 (this PR); prior work on this cache: #11744, #11501, #11364, #11730

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/core/ucp_rkey.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/uct/cuda/cuda_ipc/cuda_ipc_md.c Resolved
Comment thread src/uct/api/v2/uct_v2.h Resolved
Comment thread src/ucp/core/ucp_rkey.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

No blockers found: the refcount invariant (mapped address implies a held reference, in-flight zcopy ops and rkey_ptr/mem_elem_pack mappings all hold one) makes the new refcount == 0 && free_unused_mapping destroy path safe from use-after-close.

The do { progress; create } while (UCS_ERR_NOT_CONNECTED) loop added in test_ucp_rkey_destroy.c matches the existing idiom in test/gtest/ucp/test_ucp_device.cc, so I did not comment on it.

The already-reported pgt_region == NULL / ucs_assert(mapped_addr == NULL) path and the coverage gap were not repeated.

Residual coverage gap: contrib/test_jenkins.sh still does not run test_ucp_rkey_destroy/test_rma_cuda, and this revision dropped the earlier gtest additions, so the new UCT flag (uct_rkey_unpack_v2 → rkey_release → cache close) has no automated coverage at all; the ordering case in finding 2 and the multi-device case in finding 3 are untested.

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Starting review — findings will be posted here when done.

Comment thread src/ucp/core/ucp_rkey.c Resolved
Comment thread src/ucp/core/ucp_rkey.h Resolved
Comment thread test/mpi/test_ucp_rkey_destroy.c Resolved
@svc-ucx

svc-ucx commented Oct 28, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests new on worker 2) · commit cf4689ff

TL;DR: The build failed on exactly one gtest — shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0 — a pure protocol‑selection test whose expectations are unrelated to this PR's rkey‑release/cuda_ipc changes; it hard‑expects am/rndv to be selected for CUDA memory while RNDV_SCHEME=get_zcopy is forced, which only holds if a network MD on that worker can register CUDA memory, so the test needs a require_cuda_net_md() / skip guard.

Full analysis

Summary: make test in the gtest stage exited 1 with 1 FAILED TEST: shm_ib/test_ucp_proto_am_rndv.am_explicit_scheme_disables_force_provenance/0, where GetParam() = shm,ib,cuda_copy,rocm_copy (10622 passed, 20 skipped, no crash, no timeout — the suite ran to completion in 4,784,357 ms and then make: *** [Makefile:4971: test] Error 1).

Root cause: The failing test (test/gtest/ucp/test_ucp_proto.cc:829-837) runs with RNDV_PIPELINE_SHM_CUDA_STAGING_FORCE=y + RNDV_SCHEME=get_zcopy and calls check_am_rndv_remote_proto_config(0, 0). That helper's first hard expectation is EXPECT_STREQ("am/rndv", thresh->proto_config.proto->name) for a 1 MB UCP_OP_ID_AM_SEND with mem_info.type = UCS_MEMORY_TYPE_CUDA (test_ucp_proto.cc:409-442). Forcing RNDV_SCHEME=get_zcopy makes am/rndv initialize only if the nested remote lookup in ucp_proto_rndv_ctrl_select_remote_proto() finds a rndv/get/zcopy protocol for CUDA memory (src/ucp/rndv/proto_rndv.c:257-276, returns UCS_ERR_UNSUPPORTED otherwise). That depends on whether an IB/network memory domain on the agent can register CUDA memory — the very condition the fixture has a helper for (has_cuda_net_md() / require_cuda_net_md(), test_ucp_proto.cc:72-104), but this test only calls the weaker require_cuda_rndv_support(). The PR's own changes (UCT v2 "free unused mappings at rkey release" flag, ucp_rkey.c user‑rkey release, cuda_ipc_cache.c refactor) are data‑path/rkey‑destroy changes and cannot influence this selection: the AM path never sets UCP_PROTO_SELECT_OP_FLAG_TAG_RNDV, so ucp_proto_rndv_shm_pipeline_force_scope() returns 0 and no UCP_RKEY_CONFIG_FLAG_PROTO_ESTIMATION config is ever created (src/ucp/rndv/proto_rndv.inl:30-85), making the other two assertions config‑independent (and the two sibling AM tests passed). Caveat: the retrievable Azure log is only the tail of the job and does not contain the gtest assertion text, so the exact failing EXPECT line is inferred from the source, not read from the log.

Implicated commit: b60d87b (jeffnvidia, 2026-09-30, "UCP/RKEY: Keep configurations at stable addresses (#11676)") — most recent commit touching test/gtest/ucp/test_ucp_proto.cc, which introduced the test_ucp_proto_am_rndv fixture and count_rkey_configs_with_flag assertions. Not the PR under test (cf4689f / a1fb590 / 63867df / 81170eb / 9303c0d / 834f54f by Raul Akhmetshin).

File: test/gtest/ucp/test_ucp_proto.cc:829 (test body), test/gtest/ucp/test_ucp_proto.cc:439 (EXPECT_STREQ("am/rndv", ...)), src/ucp/rndv/proto_rndv.c:257

Suggested fix:

  1. Re-run the job to confirm it is not reproducible against this PR (the change set cannot affect AM rndv protocol selection).
  2. Harden the test instead of the PR: in am_explicit_scheme_disables_force_provenance (and rma_explicit_scheme_disables_force_provenance) replace require_cuda_rndv_support() with require_cuda_net_md(), and/or make select_am_rndv_remote_proto_config() UCS_TEST_SKIP_R("am/rndv is not selected") instead of EXPECT_STREQ-failing when RNDV_SCHEME is explicitly restricted — mirroring the existing "stub protocol" skip in rndv_force_cuda_send_child_uses_tag_provenance (test_ucp_proto.cc:724-726).
  3. To confirm the exact assertion, pull the full gtest log for build 141168 (the Azure tail truncates it) and grep for am_explicit_scheme_disables_force_provenance / Failure.

Related: PR #12108 (this build), PR #11676 (added these assertions); no existing issue found for this test name.

@rakhmets
rakhmets merged commit db905d9 into openucx:master Oct 29, 2026
167 checks passed
william-gallagher-nv pushed a commit to william-gallagher-nv/ucx that referenced this pull request Nov 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants