[michal-shalev]

What?

Reserve the TX credits a multi-credit send needs, so that later single-credit sends on the same EP cannot consume them.

Why?

put_sgl_zcopy needs count WQEs, so it returns NO_RESOURCE while credits are still available and is queued in pending. A later 1-credit AM then passes the resource check and is posted ahead of it:

rc_iface.h:620 Assertion `iface->tx.in_pending || ucs_arbiter_group_is_empty(arb_group)' failed

How?

Resource checks compare the available credits against max(requirement, EP reservation), and a failing send records its requirement in txqp_reserve / cq_reserve.
The reservation is released by uct_rc_ep_schedule_pending() once nothing is queued on the EP, and by uct_rc_ep_pending_purge().

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

Behavioral scope beyond put_sgl_zcopy: Folding UCT_RC_CHECK_PENDING_RET into UCT_RC_CHECK_CQE_RET makes all RC send paths that go through UCT_RC_CHECK_RES/UCT_RC_CHECK_TX_CQ_RES (put/get/am/atomic/flush NOP/keepalive) block when the arbiter group is non-empty or reserves are set — previously that was only an ucs_assert in UCT_RC_CHECK_RES_AND_FC. This looks intentional but is a much broader change than 'add a pending check to put_sgl_zcopy'. If the PR description only mentions the sgl path, please update it to describe this transport-wide ordering change so reviewers/regression owners are aware.

Test coverage note: The new put_am_pending test is a reasonable regression test for the ordering fix, but it is a UCP-level, non-deterministic reproducer; there is no direct assertion that ordering was preserved (it relies on data verification via the AM callback returning UCS_OK and put completion), and it runs only on all TLs, so the RC-specific fix is exercised only when an RC/RCX lane is selected.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

src/uct/ib/rc/base/rc_ep.h

minor: in_pending could be a UCT_RC_EP_FLAG_* bit in the existing flags field instead of a new byte, unless the read-modify-write on flags on the pending dispatch path is a concern. can we reuse flags?

test/gtest/ucp/test_ucp_rma.cc

The new put_am_pending test shares one sgl_ctx/local/remote/param across all worker threads and issues on the same ep under MULTI_THREAD_WORKER. That is safe (read-only SGL contents, MT worker mode), but since there is no data verification and no direct ordering assertion, the test only catches crashes/asserts/errors — it will not detect silent ordering violations.

[michal-shalev]

src/uct/ib/rc/base/rc_ep.h

minor: in_pending could be a UCT_RC_EP_FLAG_* bit in the existing flags field instead of a new byte, unless the read-modify-write on flags on the pending dispatch path is a concern. can we reuse flags?

test/gtest/ucp/test_ucp_rma.cc

flags is already a full uint8_t (bits 0-4 ep state, 5-7 FC AM header). No spare bit without widening, which does not save space vs a dedicated uint8_t.

The new put_am_pending test shares one sgl_ctx/local/remote/param across all worker threads and issues on the same ep under MULTI_THREAD_WORKER. That is safe (read-only SGL contents, MT worker mode), but since there is no data verification and no direct ordering assertion, the test only catches crashes/asserts/errors — it will not detect silent ordering violations.

Correct, it is a debug-assert reproducer, not a payload/FIFO checker. The invariant is UCT QP order. Checking PUT data before AM would close the race the test is meant to hit.

[svc-nvidia-pr-review]

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

[svc-ucx]

🤖 CI Triage Agent — UCX PR (AddressSanitizer gpu on worker 0) · commit dc05f89d

[incomplete: stop_reason=max_tokens]

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.

[svc-ucx]

🤖 CI Triage Agent — UCX PR (Tests BlueField on worker 0) · commit b8b39641

TL;DR: test_rc_flow_control.pending_grant fails deterministically on both rc_verbs and rc_mlx5 because this PR's commit b8b3964 inserted UCT_RC_CHECK_PENDING_RET() at the top of the generic UCT_RC_CHECK_CQE_RET() macro, so every RC send (including uct_ep_put_short) now returns UCS_ERR_NO_RESOURCE whenever the EP's arbiter group is non-empty — even when the only queued item is an internal FC pure-grant. Restrict the pending check to the multi-credit SGL path (where it is already applied explicitly) instead of the shared CQE macro.

Full analysis

Summary: gtest step failed with 2 failures — rc_verbs/test_rc_flow_control.pending_grant/0 and rc_mlx5/test_rc_flow_control.pending_grant/0 (8628 of 8630 passed, make: *** [Makefile:4713: test] Error 1).

Root cause: test_pending_grant() deliberately parks an FC pure-grant request in m_e2's arbiter group (via disable_entity()), then re-enables the iface and issues uct_ep_put_short(m_e2->ep(0), ...) to force pending dispatch, asserting UCS_OK (test/gtest/uct/ib/test_rc.cc:959). Both uct_rc_verbs_ep_put_short() (rc_verbs_ep.c:166) and uct_rc_mlx5_base_ep_put_short_inline() (rc_mlx5_ep.c:60) go through UCT_RC_CHECK_RES → UCT_RC_CHECK_TX_CQ_RES → UCT_RC_CHECK_CQE_RET, which now begins with UCT_RC_CHECK_PENDING_RET(_ep, _ret) (src/uct/ib/rc/base/rc_ep.h:109). Since UCT_RC_EP_MUST_BLOCK_SEND() is true (non-empty arb_group, in_pending == 0), the put_short returns UCS_ERR_NO_RESOURCE, the queued grant is never dispatched, and the subsequent validate_grant() / send_am_messages(..., UCS_OK) assertions also fail. That the pending check is duplicated — uct_rc_mlx5_base_ep_put_sgl_zcopy() still calls UCT_RC_CHECK_PENDING() explicitly at rc_mlx5_ep.c:246 before using the pending-free UCT_RC_CHECK_CQE_VALUE_RET — confirms the check inside UCT_RC_CHECK_CQE_RET is the new, overly broad hunk. Failure is deterministic on both RC transports (DC uses its own macros and passes), so this is not environmental flakiness.

Implicated commit: [REDACTED:Hex High Entropy String] — "UCT/RC: Block sends only when pending is queued" (Michal Shalev); builds on f44378a "UCT/RC: Preserve pending order for multi-credit SGL puts"

File: src/uct/ib/rc/base/rc_ep.h:108-115 (pending check added at line 109); test assertion at test/gtest/uct/ib/test_rc.cc:959

Suggested fix: Drop UCT_RC_CHECK_PENDING_RET(_ep, _ret) from UCT_RC_CHECK_CQE_RET and keep the pending check only where ordering against multi-credit SGL puts is actually needed (the explicit UCT_RC_CHECK_PENDING(&ep->super) already present in uct_rc_mlx5_base_ep_put_sgl_zcopy, rc_mlx5_ep.c:246), so single-WQE ops such as put_short keep their previous "send is allowed, pending dispatch is driven by progress" semantics. If blocking all sends on a non-empty arbiter group is genuinely intended, then the FC-grant case must be excluded (internal pending requests) and test_rc_flow_control::test_pending_grant() must be reworked to trigger dispatch via progress()/retry loop instead of asserting UCS_OK from a single uct_ep_put_short.

Related: PR #11841 (this PR), #11677 (put_sgl_zcopy implementation), #11361 (per-EP txqp_reserve resource checks)

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id d103b2ee-5c84-47ed-9415-b77b17bb3176 in the triage console for the audit trail.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

Note (no action required): The UCT_RC_CHECK_PENDING_RET folding into UCT_RC_CHECK_CQE_RET widens the runtime block to all RC send paths (previously only an assert). This was already raised in a prior review round; confirming it's intentional and correct.

Note (no action required): test_pending_grant correctly switches from put_short (now blocked by the new check while the FC grant is queued) to a direct uct_rc_iface_arbiter_dispatch, and the comment is updated accordingly.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

Residual coverage gap worth noting: the new tests only run where put_sgl_zcopy is supported (rc_mlx5/gga on real mlx5 HW); rc_verbs and the ibmock CI path will skip them.

[svc-nvidia-pr-review]

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

[svc-ucx]

🤖 CI Triage Agent — UCX PR (AddressSanitizer roce on worker 3) · commit 030c504c

TL;DR: The gtest shard died in shm_gga/test_ucp_rma_mt.put_get because the GGA/UMR mkey registration on mlx5_1 never completed (30 s ibv_poll_cq(UMR CQ, registration) timeout on the md's self-loopback UMR QP), making ucp_rkey_pack return UCS_ERR_IO_ERROR; the test then returned without ucp_mem_unmap, so ucp_cleanup hit the fatal region->refcount == 0 rcache assertion and core-dumped. This is in the IB/GGA memory-registration control path, not in the RC TX-credit code this PR changes — retrigger, and harden the UMR timeout + test cleanup paths.

Full analysis

Summary: shm_gga/test_ucp_rma_mt.put_get/0 <shm,gga> failed at test_ucp_rma_mt.cc:64 (ucp_rkey_pack → Input/output error) after a 30 s UMR CQ timeout, then aborted with rcache.c:394 Assertion 'region->refcount == 0' failed during ucp_cleanup.

Root cause:

• First (real) error, 23:21:19 → 23:21:49 (exactly the 30 s internal poll limit, not a wall-clock kill):
  mlx5_1: ibv_poll_cq(UMR CQ, registration, UMR mkey ... index 0x526) timed out. The IBV_WR_BIND_MW WQE posted to md->umr.qp (an RC QP looped back to itself via port_attr->lid / default GID index on first_port, created with max_send_wr = 1) was never completed by the HCA. Because the SQ depth is 1 and the stuck WQE is still outstanding, the follow-up invalidation post failed with returned 12 (ENOMEM = SQ full; the %m text "Bad address" is a stale errno), leaving the md's UMR QP permanently unusable.
• Consequence: uct_md_mkey_pack → ucp_rkey_pack returned UCS_ERR_IO_ERROR; ASSERT_UCS_OK(st) at test_ucp_rma_mt.cc:64 returns from the test body before ucp_mem_unmap(memh), so the registered stack region [0x7ffc0ccc4f60..0x7ffc0ccc4f80] still had ref 1 (md[2]=mlx5_0 md[3]=mlx5_1) at ucp_mem_rcache_cleanup, which is a fatal assert → Aborted (core dumped) → Bash exited with code '2'.
• Nothing in the PR's diff (UCT/RC: Reserve TX credits for multi-credit sends, commits b2aa6c30/256ff2fc in src/uct/ib/rc/base/rc_ep.c) touches md->umr.*, mlx5dv mkey creation, or BIND_MW; src/uct/ib/mlx5/dv/ib_mlx5dv_md.c was last changed in September ([REDACTED:Hex High Entropy String]). So the trigger is environmental/HW-level on swx-rain03's mlx5_1 (RoCE loopback UMR QP), not the PR logic.

Implicated commit: unknown for the UMR timeout (environment / pre-existing GGA UMR path); PR commits under test are b2aa6c30 and 256ff2fc (Michal Shalev) and are not in the failing code path.

File: src/uct/ib/mlx5/dv/ib_mlx5dv_md.c:1447-1454 (UMR sync poll timeout) and :1548-1552; test abort site test/gtest/ucp/test_ucp_rma_mt.cc:64

Suggested fix:

1. Retrigger the job; if shm_gga UMR timeouts recur on swx-rain03/mlx5_1, treat it as a lab issue and check the port state / GID table (UCT_IB_DEVICE_DEFAULT_GID_INDEX on ibdev->first_port) used by uct_ib_mlx5_devx_umr_modify_qp, since the UMR QP is connected to itself with that GID.
2. Harden uct_ib_mlx5_devx_umr_post_sync: on timeout do not post another WR on the same 1-deep SQ — move the UMR QP to RESET/ERR (or tear down md->umr.qp/cq and re-init lazily) and skip the invalidation, so one stuck WQE doesn't make every later registration on that md pay a 30 s timeout and emit misleading ENOMEM errors.
3. Make the gtest fail cleanly instead of core-dumping: wrap the ucp_mem_map in test_ucp_rma_mt::put_get in a scope guard (or use ASSERT_UCS_OK only after registering a cleanup that calls ucp_mem_unmap) so an early ASSERT_UCS_OK return doesn't leave an in-use rcache region for ucp_cleanup.

Related: PR #11841 (build under test, UCT/RC: Reserve TX credits for multi-credit sends); no existing issue found for the UMR CQ ... timed out signature — worth opening one against the GGA/UMR registration path.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 4f4dd59a-f6f6-4edd-80b9-68736a70ae8f in the triage console for the audit trail.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.