Skip to content

UCT/CUDA: Moved mem_alloc/mem_free to cuda/base. - #12204

Open
rakhmets wants to merge 2 commits into
openucx:masterfrom
rakhmets:topic/uct-cuda-mem
Open

rakhmets wants to merge 2 commits into
openucx:masterfrom
rakhmets:topic/uct-cuda-mem

Conversation

@rakhmets

Copy link
Copy Markdown
Contributor

What?

Moved uct_cuda_mem_alloc/uct_cuda_mem_free to cuda/base, addressing #11795 (comment).

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/cuda/base/cuda_mem.c Resolved
Comment thread src/uct/cuda/base/cuda_mem.c Resolved
Comment thread src/uct/cuda/base/cuda_mem.c Resolved
Comment thread src/uct/cuda/Makefile.am Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Severity: all of the above are non-blocking (COMMENT mode), with the release-path behavior change being the one that warrants an answer before merge.

Test coverage: no tests are added, which is acceptable for a move-only refactor, but the behavior change in the fabric release path is only exercised by fabric/MNNVL-capable hardware (HAVE_CUDA_FABRIC), which typically is not covered by the default gtest/CI runners — worth confirming a fabric-capable CI job or manual run.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/cuda/base/cuda_mem.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/cuda/Makefile.am Resolved
Comment thread src/uct/cuda/base/cuda_mem.h Resolved
Comment thread src/uct/cuda/base/cuda_mem.c Resolved
Comment thread src/uct/ib/mlx5/gdaki/gdaki.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap: no tests added, which is fine for a move-only refactor: the cuda_copy alloc/free path is exercised by test/gtest/uct/cuda/ (e.g. test_switch_cuda_device.cc), and the gda path by the GDA jobs. The HAVE_CUDA_FABRIC branches in uct_cuda_mem_alloc_fabric/uct_cuda_mem_release_fabric still need fabric/MNNVL-capable hardware to be executed at all.

@rakhmets
rakhmets marked this pull request as ready for review October 30, 2026 19:05
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/cuda/Makefile.am Resolved
Comment thread src/uct/ib/mlx5/gdaki/gdaki.c Resolved
@svc-ucx

svc-ucx commented Oct 30, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Tests new on worker 3) · commit 751991a8

TL;DR: tcp/test_ucp_rma.put_nonblocking/2 <tcp,cuda_copy,rocm_copy/user_memh> aborted on the rcache invariant ucs_assert(!(region->flags & UCS_RCACHE_REGION_FLAG_PGTABLE)) (rcache.c:396) while ucp_mem_unmap() released the parent region — i.e. the region's refcount fell to 0 (page-table reference consumed) while it was still in the page table, a reference-count imbalance in the CUDA user-memh registration path introduced by this PR's new shared CUDA alloc/free code.

Full analysis

Summary: gtest aborted (core dumped, make: *** [Makefile:4971: test] Aborted) in the first case of tcp/test_ucp_rma with the user_memh variant on CUDA memory; the failure is an rcache assertion, not a data/timeout failure.

Root cause: From the backtrace: ucp_mem_unmap() → ucp_memh_cleanup() (ucp_mm.c:470) → ucp_memh_put_rcache() (ucp_mm.c:441) → ucs_rcache_region_put_unsafe() (rcache.inl:128) → ucs_mem_region_destroy_internal() (rcache.c:396). A cached region is created with refcount = 2 (page-table + user, rcache.c:1028) and may only be destroyed once it has been removed from the page table (UCS_RCACHE_REGION_FLAG_PGTABLE cleared by ucs_rcache_region_invalidate_internal(), rcache.c:492-501). The assert shows refcount reached 0 while the flag was still set, so the user memh's parent region was put one time too many (one ucs_rcache_get() reference released twice, or released without ever being taken). The only component changed on this branch is the CUDA memory path: HEAD commit 751991a8 moves CUDA mem_alloc/mem_free into the new src/uct/cuda/base/cuda_mem.c, where the VMM/fabric path rounds the allocation up to the VMM granularity (mem->length = ucs_align_up(length, *granularity), cuda_mem.c:45) and uct_cuda_copy_mem_alloc() reports that rounded length back to UCP (*length_p = cuda_mem->length, cuda_copy_md.c:322). Changed allocation ranges/alignment change which UCM free events and rcache region merges/invalidations the CUDA buffers produce, which is what exposes the extra put of the parent region. The pre-existing code is also fragile here: region->refcount = 2 at rcache.c:1028 is an assignment, not an increment, so any reference taken on the region between page-table insertion and that line is silently lost and later leads to exactly this assertion.

Implicated commit: 751991a — "UCT/CUDA: Moved mem_alloc/mem_free to cuda/base." (Raul Akhmetshin), PR #12204 (branch topic/uct-cuda-mem)

File: src/ucs/memory/rcache.c:396 (assertion); triggered from src/ucp/core/ucp_mm.c:441 (ucp_memh_put_rcache) / src/ucp/core/ucp_mm.c:470; suspect new code: src/uct/cuda/base/cuda_mem.c:45 and src/uct/cuda/cuda_copy/cuda_copy_md.c:294-322

Suggested fix:

  1. In the moved allocation path, keep the reported allocation range consistent with what UCM/the memtype cache and rcache will see: either do not enlarge the request silently (align the request before cuMemCreate and report the same base/length), or make sure uct_cuda_copy_mem_alloc() reports base address and rounded length consistently for both the fabric (VMM) and cuMemAlloc paths, and that uct_cuda_mem_free() releases exactly the same range that was announced.
  2. Reproduce locally with UCX_TLS=tcp,cuda_copy ./gtest --gtest_filter='tcp/test_ucp_rma.put_nonblocking/2' and UCX_LOG_LEVEL=trace (rcache "created/hold/put/destroy" traces identify which region is over-released); a quick A/B with UCX_CUDA_COPY_ENABLE_FABRIC=n will confirm whether the new VMM/fabric allocation path is the trigger.
  3. Independently harden rcache: change region->refcount = 2; (rcache.c:1028) to an atomic increment of the existing value so a concurrently acquired reference is not clobbered, and consider upgrading the bare ucs_assert at rcache.c:396 to ucs_assertv printing region/rcache name and flags for faster triage.

Related: PR #12204 (#12204); prior work in the same area: #11795 (GDAKI VMM fabric allocation), #12186 (don't widen VMM range crossing a handle boundary). No existing issue found for this assertion signature.

@tvegas1 tvegas1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pls add something like below in the AGENT file under cuda base.

- `cuda_mem.[ch]` — device memory alloc/free (`uct_cuda_mem_alloc()`/
  `uct_cuda_mem_free()`), including fabric VMM allocation.

@rakhmets

rakhmets commented Nov 2, 2026

Copy link
Copy Markdown
Contributor Author

LGTM, pls add something like below in the AGENT file under cuda base.

- `cuda_mem.[ch]` — device memory alloc/free (`uct_cuda_mem_alloc()`/
  `uct_cuda_mem_free()`), including fabric VMM allocation.

I have some doubts about the usefulness of listing the files. I'm more inclined to delete the existing list of files and leave only the description of the directories. Moreover, this file is quite compact, the name corresponds to what this file describes, and it does not do anything beyond that, and the functions have a detailed comment.

@tvegas1
tvegas1 enabled auto-merge (squash) November 2, 2026 09:10
@rakhmets
rakhmets requested a review from a team as a code owner November 4, 2026 13:37
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/uct/cuda/Makefile.am Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

src/uct/cuda/base/cuda_mem.c

minor: mem->length is size_t but printed with %ld (lines 88 and 96). It is moved code, but since it lands in a new file it is a cheap cleanup — use %zu:

                "allocated memory at %p of size %zu does not have fabric "
                "property",

Same in the ucs_trace() below.

@svc-ucx

svc-ucx commented Nov 5, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Namespace_Tests gpu_namespace on worker) · commit 7ae50789

TL;DR: The gpu_namespace job failed because ucx_perftest could not initialize CUDA at all — cudaGetDeviceCount() returned CUDA error 807 ("MPS server is not ready to accept new MPS client requests") on the GPU worker swx-rdmz-ucx-gpu-02, an environment/MPS-daemon problem on that node rather than anything in this PR; fix by clearing/disabling CUDA MPS on the worker (or bypassing MPS for the unshared namespace test) and adding a GPU pre-flight check.

Full analysis

Summary: "Run namespace test" step exited with code 255 — both the perftest server and client died during CUDA init with cudaGetDeviceCount() failed: 807 (MPS server is not ready to accept new MPS client requests).

Root cause: The test launches perftest inside a fresh PID namespace:
sudo unshare --mount-proc --pid --fork sudo -u swx-azure-svc UCX_TLS=cuda_ipc,cuda_copy,sysv CUDA_VISIBLE_DEVICES=0 .../ucx_perftest -t ucp_put_bw -m cuda -p 10500.
On the worker node, a CUDA MPS control daemon is active, so the CUDA runtime tries to attach to an MPS server. CUDA error 807 (CUDA_ERROR_MPS_SERVER_NOT_READY) is returned before any device is enumerated — the MPS server for that user is not up/healthy, and a client in a new PID namespace with a remounted /proc cannot be registered by the MPS control daemon. The failure happens in the very first CUDA call of perftest's init path (src/tools/perf/cuda/cuda_alloc.c:32), i.e. before any UCX/UCT CUDA memory code from this branch is reached, and it reproduced identically for both the server and client process. There is no compile/link error and nothing in the build portion of the log failed, so commit 7ae5078 (topic/uct-cuda-mem) is not implicated. Note there were also no large time gaps — the whole step ran ~3 s to failure, so this is a hard environment error, not a hang or timeout.

Implicated commit: none — environment/infrastructure failure (no UCX commit implicated; build of [REDACTED:Hex High Entropy String] completed and installed cleanly)

File: src/tools/perf/cuda/cuda_alloc.c:32 (error reporting site only, not the defect); failing CI step: "Run namespace test" in the gpu_namespace on worker job

Suggested fix:

  1. Infra: on swx-rdmz-ucx-gpu-02, reset MPS before GPU jobs — e.g. echo quit | nvidia-cuda-mps-control (and restart it only if intentionally required), then verify with nvidia-smi / a trivial cudaGetDeviceCount probe. A stale/half-started MPS control daemon from a previous job is the likely leftover.
  2. Make the namespace test MPS-agnostic: in the unshare environment, neutralize MPS for the child, e.g. pass CUDA_MPS_PIPE_DIRECTORY=/nonexistent (or unset CUDA_MPS_PIPE_DIRECTORY CUDA_MPS_LOG_DIRECTORY) alongside CUDA_VISIBLE_DEVICES=0, since MPS clients are not expected to work across a fresh PID namespace with --mount-proc.
  3. Add a pre-flight GPU/CUDA health check at the start of the namespace test that fails fast with an explicit "GPU/MPS unavailable on this node" message (or skips the test) instead of surfacing a bare Bash exited with code '255'.
  4. Re-run the PR pipeline after the node is cleaned to confirm the PR itself is green.

Related: #12200 (UCT/CUDA_IPC: Pass POSIX FD handles across PID namespaces) and #12095 touch the same PID-namespace CUDA IPC test path; #11896 ("Build once per GPU test row and share the tree with the workers") introduced the shared-tree/worker layout this job uses. No existing issue tracks the MPS error 807 in CI — worth opening one.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 199f8f78-46ff-4ed7-ae5a-3e2b72b455e1 in the triage console for the audit trail.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants