Skip to content

UCM: Don't call ucs_status_string() from UCM modules - #12228

Open
ankith117 wants to merge 2 commits into
openucx:masterfrom
ankith117:topic/ucm-no-ucs-status-string
Open

ankith117 wants to merge 2 commits into
openucx:masterfrom
ankith117:topic/ucm-no-ucs-status-string

Conversation

@ankith117

@ankith117 ankith117 commented Nov 3, 2026 •

Copy link
Copy Markdown
Contributor

What

  • Print the numeric status code instead of ucs_status_string() in src/ucm/rocm/rocmmem.c and
    src/ucm/cuda/cudamem.c.
  • Add check_ucm_no_ucs_symbols to buildlib/az-helpers.sh and run it after the build in
    buildlib/pr/rocm/test_malloc_hook.sh and buildlib/pr/cuda/test_malloc_hook.sh: it fails if a
    UCM module (libucm_*.so) has an undefined ucs_* symbol.
  • Document the rule in the ucm entry of src/AGENTS.md.

Why

UCM modules link only libucm. When UCX is loaded with RTLD_LOCAL (for example as an application
plugin), libucs is not in their scope, so logging through ucs_status_string() kills the process:

libucm_rocm.so.0: undefined symbol: ucs_status_string

Since #11887 this happens on every start when the bistro hook for the HSA allocator can't be
installed and UCX falls back to reloc hooks, because that path logs a warning. One such case is the
HSA runtime bundled with the PyTorch ROCm wheel. libucm_cuda has the same call on a ucm_diag
path (#8092).

The check covers only the modules: libucm.so itself references ucs_load_modules and
ucs_sys_is_dynamic_lib, which is fine because it is loaded together with libucs.

Testing

  • ROCm, MI355X, PyTorch ROCm wheel, UCX loaded through NIXL: the process no longer exits at startup;
    the warning is printed and the reloc hooks are installed.
  • ROCm build with contrib/configure-devel as in the CI script: check_ucm_no_ucs_symbols passes;
    with the two UCM files reverted to master it fails on ucs_status_string.

libucm_rocm doesn't link libucs, so when UCX is loaded with RTLD_LOCAL
the hook-install warning crashes with an undefined symbol. Print the
status code instead.
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread src/ucm/rocm/rocmmem.c Resolved
Comment thread src/ucm/rocm/rocmmem.c Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

If there was a concrete build or load-order problem motivating the switch from ucs_status_string() to raw %d status codes, please mention it in the PR description — it is not visible from the diff. As it stands this looks like a readability regression that also makes the ROCm install path inconsistent with the CUDA one.

edgargabriel
edgargabriel previously approved these changes Nov 3, 2026

@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.

can we also add the rule to the ucm entry in src/AGENTS.md so agents don't reintroduce this, like below?

- `ucm`: ... UCM libraries must not link or call into `libucs`; use UCS
  headers only for macros and inline functions.

Comment thread src/ucm/rocm/rocmmem.c Resolved
Comment thread src/ucm/rocm/rocmmem.c Resolved
Comment thread src/ucm/rocm/rocmmem.c Resolved

@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.

need to fix also cudamem.c

libucm_rocm and libucm_cuda don't link libucs, so when UCX is loaded with
RTLD_LOCAL the hook-install log messages crash with an undefined symbol.
Print the status code instead, add a CI check for undefined ucs_ symbols
in UCM modules, and document the rule in src/AGENTS.md.
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@ankith117

Copy link
Copy Markdown
Contributor Author

@tvegas1 @edgargabriel PTAL

@ankith117 ankith117 changed the title UCM/ROCM: Don't call ucs_status_string() from libucm_rocm UCM: Don't call ucs_status_string() from UCM modules Nov 3, 2026
Comment thread buildlib/az-helpers.sh Resolved
Comment thread buildlib/az-helpers.sh Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

Residual coverage gap worth noting: there is no test that reproduces the original failure (UCX loaded via dlopen(..., RTLD_LOCAL) and then triggering ucm_cudamem_install()), so the static nm check is the only protection; it will not catch a libucs call that is reached through a new UCS header inline function that is not itself inline.

@edgargabriel

Copy link
Copy Markdown
Contributor

we need to backport this to v1.23.x probably as well

@tvegas1
tvegas1 enabled auto-merge (squash) November 4, 2026 07:59
@svc-ucx

svc-ucx commented Nov 4, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer BlueField on worker 2) · commit 3f999051

TL;DR: The ASAN BlueField gtest shard hung (not timed out from slowness) in rcx/test_ucp_atomic32.fetch/1 <rc_x,...,/device> — an rc_x atomic never completed and the subsequent ep flush/close spun forever; the PR's own change is UCM logging only, and the hang points at the new sticky RC TX-credit reservation (ep->txqp_reserve/cq_reserve) from commit 22cff74, which is only cleared on a TX CQE for that ep.

Full analysis

Summary: Azure job "AddressSanitizer BlueField on worker 2" (build 142538) failed in make -C test/gtest test (shard 3/4): the very first test rcx/test_ucp_atomic32.fetch/1 <rc_x,cuda_copy,rocm_copy/device> never made progress, gtest's watchdog aborted the run with Connection timed out - abort testing and SIGABRT inside ucp_test::progress() from close_all_eps().

Root cause: A hang, not a wall-clock shortage. Log timestamps show the test started at 13:55:30 and the next line is at 14:06:46 (~11 min of total silence), then another ~11 min gap to 14:18:02, then the global abort at 14:21:46 — i.e. continuous no-progress, with ucp_test.cc:310 UCX ERROR request 0xffff4a20a420 did not complete on time followed by a second stalled host->host ADD, and finally the backtrace stuck in ucp_test_base::entity::close_all_eps() → ucp_test::progress() (flush/ep-close never completing). So UCT never posted/completed the atomic on the rc_x ep, and the ep stayed permanently "without TX resources".

The code path that can produce exactly this state is the TX credit reservation added by 22cff74b ("UCT/RC: Reserve TX credits for multi-credit sends", #11841, merged 2026-11-02, one day before this branch):

  • uct_rc_ep_reserve_txqp() / uct_rc_ep_reserve_cqe() latch ep->txqp_reserve / ep->cq_reserve when a send finds insufficient credits (src/uct/ib/rc/base/rc_ep.h:393-418).
  • uct_rc_ep_has_tx_resources() / uct_rc_ep_have_tx_cqe_avail() then require available > reserve forever (rc_ep.h:420-435).
  • uct_rc_ep_process_pending() deliberately keeps the reserve after a successful dispatch (rc_ep.c:426-430), and the only resets are uct_rc_ep_pending_purge() (rc_ep.c:516) and uct_rc_ep_schedule_pending() — which is invoked only from uct_rc_mlx5_iface_poll_tx() on a TX CQE for that ep (src/uct/ib/mlx5/rc/rc_mlx5.inl:2112).

Consequently, if the ep has no outstanding signaled WQE left (TX moderation / everything already completed), no CQE will ever arrive, the reserve is never cleared, every subsequent atomic/flush returns UCS_ERR_NO_RESOURCE, and UCP's request plus the ep-close flush hang indefinitely — matching the log precisely.

The PR under test (topic/ucm-no-ucs-status-string, 3f99905 "UCM: Don't call ucs_status_string() from UCM modules") touches only UCM logging (src/ucm/util/log.h is pure log plumbing, no data path), so it is not a plausible cause of an rc_x data-path stall.

Implicated commit: 22cff74 — "UCT/RC: Reserve TX credits for multi-credit sends (#11841)", Michal Shalev (PR HEAD 3f99905 by Sai Ankith Averineni is log-only and not implicated)

File: src/uct/ib/rc/base/rc_ep.h:375-435 (reserve set/test, uct_rc_ep_reset_reserve), src/uct/ib/rc/base/rc_ep.c:426-430 (reserve kept on successful pending dispatch), src/uct/ib/mlx5/rc/rc_mlx5.inl:2112 (only CQE-driven reset)

Suggested fix:

  1. Make the reserve reset independent of a TX CQE: call uct_rc_ep_reset_reserve(ep) whenever the ep's arbiter group becomes empty — e.g. in uct_rc_ep_process_pending() when returning REMOVE_ELEM and ucs_arbiter_group_is_empty(group), and/or in uct_rc_mlx5_ep_update_tx_qp_res()/uct_rc_iface_arbiter_dispatch() paths — so an ep with nothing outstanding can never stay latched at "no resources".
  2. Add an assertion/VFS counter for txqp_reserve/cq_reserve being non-zero while txqp.outstanding is empty, which would have caught this state immediately.
  3. Short term: re-run this job to confirm it is not PR-related; if it reproduces on master, consider reverting UCT/RC: Reserve TX credits for multi-credit sends #11841 (a 1.23.x backport, UCT/RC: Reserve TX credits for multi-credit sends - v1.23.x #12222, also exists and would need the same fix).

Related: PR #11841 (UCT/RC: Reserve TX credits for multi-credit sends), backport PR #12222, PR #12228 (the branch under test — log-only UCM change)

@edgargabriel

Copy link
Copy Markdown
Contributor

@tvegas1 I think the CI is stuck on this P, its running now for more than a day just in the last two checks

@tvegas1

tvegas1 commented Nov 6, 2026

Copy link
Copy Markdown
Contributor

After checking it's still in the queue as there is load currently, suggest to keep waiting for now.

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.

5 participants