Skip to content

UCT/IB/RC/VERBS: Release the descriptor of a failed receive completion - #12060

Merged
tvegas1 merged 4 commits into
openucx:masterfrom
GuangguanWang:pr2-rc-verbs-failed-rx-desc
Oct 26, 2026
Merged

tvegas1 merged 4 commits into
openucx:masterfrom
GuangguanWang:pr2-rc-verbs-failed-rx-desc

Conversation

@GuangguanWang

@GuangguanWang GuangguanWang commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

What?

Release the receive descriptor back to the interface pool when a receive
completion reports an error, and add a regression test for it.

Why?

A receive completion consumes the receive WR either way, so the descriptor which
that WR holds must go back to the pool. The two skipped statuses,
IBV_WC_REM_ABORT_ERR and IBV_WC_WR_FLUSH_ERR, continued without releasing
it, so every failed receive leaked one descriptor. Enough of them exhaust the
pool, which stops reposting receives and stalls incoming traffic.

How?

Neither status delivered an active message, so the descriptor is released
unconditionally, as the success path already does. All other non-success
statuses are fatal, so no other path leaks one here.

Test: the error completion is produced by a mock, which replaces the
poll_cq() operation of the device context to report it stably. The test
takes one descriptor out of the pool, as if a receive WR held it, reports an
IBV_WC_REM_ABORT_ERR completion for it, and requires it back afterwards. It
then gives back the receive queue slot which that completion returned, since
the queue never held the work request.

A receive descriptor is taken from the interface pool for every receive WR
posted to the SRQ, but a completion which reports an error returns without
releasing it, while the SRQ counts the WR as available and the next post
takes another descriptor from the pool. Every failed receive therefore leaks
a descriptor.

Release the descriptor before skipping the completion.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-ucx

svc-ucx commented Oct 9, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Codestyle AUTHORS file update check) · commit 9ac3ff75

TL;DR: This isn't a code bug — the "AUTHORS file update check" failed because PR #12060's author, Guangguan Wang, is a new contributor who isn't listed in the AUTHORS file. Run ./contrib/authors_update.sh locally and commit the updated AUTHORS file to the PR branch.

Full analysis

Summary: Azure Pipelines Codestyle job "AUTHORS file check" exited with code 1 because git diff --exit-code detected uncommitted changes to AUTHORS after running contrib/authors_update.sh.

Root cause: The CI step computes the PR's commit range (8f52fbb5..9ac3ff75) and runs ./contrib/authors_update.sh on it, then asserts the working tree is clean. The script detected a commit author not present in AUTHORS:

Names:
++ Guangguan Wang <guangguan.wang@linux.alibaba.com>

and appended the entry, producing a diff:

+Guangguan Wang <guangguan.wang@linux.alibaba.com>

Confirmed in the source: AUTHORS lines 45–46 jump from Graham Lopez straight to Guy Ealey Morag, with no Guangguan Wang entry. contrib/authors_update.sh:36 matches on either a known email or an identical name — neither matched, so the name was added at line 37 and written back at line 46. The subsequent git diff --exit-code therefore returned non-zero. This is a contributor-policy gate, not a build/test regression; no compile or runtime failure occurred.

Implicated commit: [REDACTED:Hex High Entropy String] (Guangguan Wang, PR #12060 head) — the PR simply omits the required AUTHORS update, not a defect in the script.

File: AUTHORS:45-46 (missing entry); check logic at contrib/authors_update.sh:36-46

Suggested fix: On the pr2-rc-verbs-failed-rx-desc branch, regenerate and commit the AUTHORS file exactly as CI does:

git fetch origin master
./contrib/authors_update.sh origin/master..HEAD
git add AUTHORS && git commit -m "AUTHORS: add Guangguan Wang" && git push

Alternatively, add the single line Guangguan Wang <guangguan.wang@linux.alibaba.com> between Graham Lopez and Guy Ealey Morag (the file is LC_COLLATE=C sort-ordered, so insert it in that exact position to keep the script idempotent). Note the script writes in place and requires AUTHORS to be writable, so run it from the repo root. If this contributor already appears under a different email, adding that alias email to the existing line would also satisfy the grep -iqw "$email" check.

Related: PR #12060; check implemented by contrib/authors_update.sh (added 2024, NVIDIA) — no pre-existing issue found indicating a false positive in this check.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 4a31099e-de59-4e10-bca2-eac848eeccc4 in the triage console for the audit trail.

Comment thread test/gtest/uct/ib/test_rc.cc Resolved
Comment thread test/gtest/uct/ib/test_rc.cc Resolved
@GuangguanWang
GuangguanWang force-pushed the pr2-rc-verbs-failed-rx-desc branch from 9ac3ff7 to 4447c4f Compare October 9, 2026 11:54
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/uct/ib/test_rc.cc Resolved
Comment thread test/gtest/uct/ib/test_rc.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

The injected struct ibv_wc leaves vendor_err, opcode and wc_flags uninitialized. That is fine for the two statuses handled here (neither reaches UCT_IB_IFACE_VERBS_COMPLETION_FATAL nor uct_ib_log_recv_completion), but it does mean the test cannot be extended to other statuses without filling the rest of the WC.

The mock replaces poll_cq on the whole device context, so any other CQ polled while it is installed returns 0 instead of chaining to the original (ucs::mock::orig_func). Harmless here because m_e1 has its own ibv_context and the mock lives for a single progress() call.

No coverage for the equivalent continue paths in other verbs RX loops; uct_rc_iface_verbs_cleanup_rx() still carries its /* TODO flush RX buffers */, which is out of scope for this fix.

Mock the poll_cq() operation of the device context to produce a receive
completion with an error status, and check that its descriptor is released
back to the receive buffer pool. Both statuses which the transport skips are
covered.

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
@svc-ucx

svc-ucx commented Oct 9, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Codestyle AUTHORS file update check) · commit 4447c4f0

TL;DR: The "AUTHORS file check" failed because PR #12060's commit author Guangguan Wang <guangguan.wang@linux.alibaba.com> is not listed in the AUTHORS file; the fix is to run ./contrib/authors_update.sh and commit the resulting AUTHORS change to the PR.

Full analysis

Summary: Codestyle stage "AUTHORS file check" exited 1 — git diff --exit-code found an uncommitted modification to AUTHORS after running the authors-update script.

Root cause: The CI step computes the PR commit range (8f52fbb5..4447c4f0) and runs contrib/authors_update.sh, which appends any commit author not already present in AUTHORS. The script reported ++ Guangguan Wang <guangguan.wang@linux.alibaba.com> and inserted that line into AUTHORS (after Graham Lopez <lopezmg@ornl.gov>). The subsequent git diff --exit-code then failed by design, signalling that the contributor is missing from the checked-in AUTHORS file. This is not a build/test regression — it's the intended gate telling the PR author to add themselves. No hang or timeout is involved (whole job ran ~15 s with no gaps).

Implicated commit: [REDACTED:Hex High Entropy String] (PR #12060, author Guangguan Wang) — missing AUTHORS entry, not a faulty code change.

File: AUTHORS:46 (expected insertion point, after Graham Lopez <lopezmg@ornl.gov>); check logic in contrib/authors_update.sh:44-46 and the pipeline's git diff --exit-code step.

Suggested fix: In the PR branch pr2-rc-verbs-failed-rx-desc, run:

./contrib/authors_update.sh $(git merge-base HEAD origin/master)..HEAD
git add AUTHORS && git commit -m "AUTHORS: add Guangguan Wang" && git push

i.e. add the line Guangguan Wang <guangguan.wang@linux.alibaba.com> to AUTHORS in alphabetical order (LC_COLLATE=C sort) and amend/add it to the PR. If the contributor already appears under a different name/email spelling, align the commit's user.name/user.email with the existing AUTHORS entry instead, since the script matches on email (grep -iqw) or name (grep -iq).

Related: PR #12060; no existing issue found for this specific AUTHORS gate failure.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id fd0eb3e0-3c85-4dbc-bfe9-e7bffd83cefe in the triage console for the audit trail.

tvegas1
tvegas1 previously approved these changes Oct 9, 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.

need to update AUTHOR file too

Comment thread test/gtest/uct/ib/test_rc.cc Resolved
Comment thread test/gtest/uct/ib/test_rc.cc Resolved
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
Signed-off-by: Guangguan Wang <guangguan.wang@linux.alibaba.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@tvegas1

tvegas1 commented Oct 9, 2026

Copy link
Copy Markdown
Contributor

pls fix pr description according to latest code/testing.

@GuangguanWang

Copy link
Copy Markdown
Contributor Author

pls fix pr description according to latest code/testing.

Done.

@svc-ucx

svc-ucx commented Oct 9, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (AddressSanitizer roce on worker 1) · commit 247179d9

TL;DR: The ASAN gtest shard hung for exactly 900 s in its very first test, rc_mlx5/uct_amo_fand_for_test.for32, until the gtest watchdog fired SIGABRT; the hang is an atomic fetch reply that never completed on rc_mlx5 over RoCE, which is not the transport this PR touches — re-run CI and, if reproducible, harden the unbounded polling loop in validate_replies().

Full analysis

Summary: make -C test/gtest test (shard 2 of 4, AddressSanitizer / roce) aborted with "Connection timed out - abort testing" / signal 6 while running the first test of the shard, rc_mlx5/uct_amo_fand_for_test.for32/0 <rc_mlx5/mlx5_0:1>.

Root cause: A hang, not a slow test. The log has exactly one gap, and it is the entire runtime of the step:

  • 18:52:32.77 — [ RUN ] rc_mlx5/uct_amo_fand_for_test.for32/0 <rc_mlx5/mlx5_0:1>
  • (900 s of total silence — no progress, no error completion, no ASAN report)
  • 19:07:32.51 — test_helpers.cc:57: Failure / Connection timed out - abort testing

900 s is precisely watchdog_timeout (test/gtest/common/test_helpers.cc:31). The watchdog thread's pthread_cond_timedwait returned ETIMEDOUT, so it called ADD_FAILURE() and pthread_kill(watched_thread, SIGABRT) (lines 57–59) — hence "Caught signal 6 (Aborted: tkill(2))", the core dump, and make: *** [Makefile:4925: test] Aborted. The abort/core/exit-2 are all symptoms of the watchdog kill.

The backtrace frames 10–16 are gtest framework frames, i.e. the main thread was stuck inside the test body. In uct_amo_test, the main thread's only unbounded wait is validate_replies():

while (count < exp_replies.size()) {
    while (m_replies.empty()) {
        progress();   // no deadline, spins forever
    }

(test/gtest/uct/test_amo.cc:87-90)

So one or more uct_ep_atomic32_fetch (OR) completions on the rc_mlx5 RoCE endpoint never invoked atomic_reply_cb, and the main thread busy-polled silently until the watchdog killed it. Note the test volume is small here (ASAN sets test_time_multiplier()=20, so count() = 50 ops × 4 senders), which rules out "legitimately slow" — a 50-op atomic test cannot need 15 minutes.

Not caused by this PR: the failing instance is rc_mlx5, but commit 247179d9 / PR #12060 only changes src/uct/ib/rc/verbs/rc_verbs_impl.h (uct_rc_verbs_iface_poll_rx_common, lines 96–106), which is compiled into the rc_verbs transport exclusively and is not on the rc_mlx5 code path. I also reviewed that hunk: the descriptor is returned to the pool on IBV_WC_REM_ABORT_ERR / IBV_WC_WR_FLUSH_ERR before continue, and rx.srq.available += num_wcs after the loop still accounts for the skipped WCs, so RX credits are not leaked. No plausible mechanism links it to a lost rc_mlx5 atomic completion.

Implicated commit: unknown — not a065c406 / PR #12060 (rc_verbs-only). Candidates on the rc_mlx5 TX-completion path worth checking if this reproduces: 68cfcbc5 "UCT/IB/MXL5: uct comp marker (#11915)" (Zihao Zhao) and 757f9f9b "UCT/IB/MLX5: support rma get in purge outstanding (#11866)" (Zihao Zhao); f53da7f0 "UCT/IB: Add refcounted, TTL-based address handle cache (#11845)" (Thomas Vegas) is RoCE-relevant.

File: test/gtest/uct/test_amo.cc:87 (silent unbounded poll) — watchdog trip point test/gtest/common/test_helpers.cc:57

Suggested fix:

  1. Re-run the ASAN/roce job on this PR. The failure is in a transport the PR does not modify, with no sanitizer report and no error completion — treat it as a flake/infra issue on swx-rain03 until it reproduces, and do not raise the time limit (this was a hang, not slow progress).
  2. Make the hang diagnosable instead of a 15-minute blackout: bound the wait in uct_amo_test::validate_replies() with ucs::get_deadline() (test_helpers.cc:342) and UCS_TEST_ABORT with how many of exp_replies.size() replies arrived and per-worker count. The same applies to the worker-thread while (status == UCS_ERR_NO_RESOURCE) loop at test_amo.cc:176-180.
  3. If it does reproduce on rc_mlx5/RoCE, bisect the rc_mlx5 TX CQE handling (uct_rc_mlx5_txqp_process_tx_cqe → uct_rc_txqp_completion_inl_resp, src/uct/ib/mlx5/rc/rc_mlx5.inl:64-75) against the commits listed above — a dropped inline-response atomic completion matches the symptom exactly.

Related: PR #12060 (this PR), #11915, #11866, #11845; historically similar silent UCT hang: issue #3007.

@tvegas1
tvegas1 enabled auto-merge (squash) October 26, 2026 14:56
@tvegas1
tvegas1 merged commit 42c438a into openucx:master Oct 26, 2026
162 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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants