UCT/RC: Preserve pending order for multi-credit SGL puts

[michal-shalev]

What?

Enforce FIFO ordering for RC pending operations on all send paths (PUT/GET/AM/atomic/flush/keepalive).
Previously, a new send could be posted while the EP already had a pending request:

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

Why?

CHECK_RES did not check the pending queue.
For example, SGL PUT may return NO_RESOURCE while some credits are still available because it needs count WQEs. A later 1-credit AM could still pass the resource check and be posted before the pending SGL.

How?

Block new sends when the EP has a queued pending request. The EP's pending callback is allowed to retry its operation.
pending_add also respects reserved credits so multi-credit operations can be queued correctly.

[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]

The iface already tracks dispatch state via iface->tx.in_pending, set around the very same callback in uct_rc_iface_invoke_pending_cb(). The new per-ep ep->in_pending (set/cleared in uct_rc_ep_process_pending around that same call) is redundant with it — pending dispatch is one-request-at-a-time per iface. Suggestion: can we reuse iface->tx.in_pending here instead of adding a per-ep byte? Today it's #if UCS_ENABLE_ASSERT only, so it'd need to be compiled unconditionally and UCT_RC_EP_MUST_BLOCK_SEND would read it via the iface. This avoids growing every rc_ep. This is a soft suggestion; if there's a reason per-ep granularity is needed (e.g., nested dispatch across ifaces), it's fine to keep as-is.

[michal-shalev]

iface->tx.in_pending is iface-wide and not refcounted, so it would allow other eps on the same iface to skip their pending queue during this callback (and break under nested dispatch).

[svc-nvidia-pr-review]

The new per-ep ep->in_pending set here is redundant with iface->tx.in_pending, which is set around the same callback in uct_rc_iface_invoke_pending_cb(). Consider reusing the iface-level state instead of a per-ep field.

[michal-shalev]

See #11841 (comment).

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

[iyastreb]

can it be just a new flag?

[michal-shalev]

ep->flags is already a full uint8_t (bits 0–4 local, 5–7 FC AM header). Adding this would need a 16-bit flags field and would mix dispatch state with bits that go on the wire, so a separate in_pending is cleaner.

[brminich]

should we check that we have max_put_sgl credits on the qp for all operations?

[brminich]

can we avoid direct call of arbiter dispatch but trigger it with some message instead?

[michal-shalev]

A user send is blocked while the FC grant is queued (that was the old put_short hack), dispatching the arbiter after enable_entity is the same pattern as test_rc_keepalive.

[iyastreb]

it seems this is redundant include

[michal-shalev]

Done

[michal-shalev]

should we check that we have max_put_sgl credits on the qp for all operations?

Each op should only check the credits it needs, requiring max_put_sgl on every send would keep half the QP idle, and once an SGL is pending other sends are already blocked.

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

[brminich]

should we check that we have max_put_sgl credits on the qp for all operations?

Each op should only check the credits it needs, requiring max_put_sgl on every send would keep half the QP idle, and once an SGL is pending other sends are already blocked.

can we require max_put_sgl credits to be available for each op, but every op will take just the credits it needs. This way just max_put_sgl may be idle at max which is not that big.

[michal-shalev]

can we require max_put_sgl credits to be available for each op, but every op will take just the credits it needs. This way just max_put_sgl may be idle at max which is not that big.

max_put_sgl is tx_qp_len/2, so requiring it on every op would keep half the QP unused.

[svc-nvidia-pr-review]

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

[svc-nvidia-pr-review]

🤖 Review complete — no issues found.