UCT/EP: Enhance uct_ep_check by evgeny-leksikov · Pull Request #11495 · openucx/ucx

evgeny-leksikov · 2026-05-27T18:05:42Z

What?

Enhance uct_ep_check to support asynchronous completion callbacks.

Why?

This change improves the flexibility and responsiveness of endpoint status checks in the UCP layer.
Required for effective recovery after failure

How?

Modified uct_ep_check to accept a completion callback, allowing for asynchronous reporting of peer status.
Updated related functions across various transport implementations to handle the new completion parameter.
Added tests to verify the correct behavior of the asynchronous completion mechanism during endpoint checks.

…acks - Modified uct_ep_check to accept a completion callback, allowing for asynchronous reporting of peer status. - Updated related functions across various transport implementations to handle the new completion parameter. - Added tests to verify the correct behavior of the asynchronous completion mechanism during endpoint checks. This change improves the flexibility and responsiveness of endpoint status checks in the UCP layer.

roiedanino · 2026-05-28T12:54:14Z

@svc-nvidia-pr-review

svc-nvidia-pr-review · 2026-05-28T12:54:20Z

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

roiedanino · 2026-05-28T13:07:23Z

@svc-nvidia-pr-review

svc-nvidia-pr-review · 2026-05-28T13:08:06Z

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

svc-nvidia-pr-review

Code Review

8 findings — 🚨 1 blocker, ⚠️ 7 minors

7 findings posted as inline comments.

General Findings

⚠️ MINOR src/uct/api/uct.h

API doc for uct_ep_check is not updated to describe the new comp semantics. Public header still implies comp must be NULL (matching the just-removed UCT_CHECK_PARAM). pls add a docstring update covering UCS_OK vs UCS_INPROGRESS returns and when comp is invoked.

svc-nvidia-pr-review · 2026-05-28T13:17:20Z

+    ucs_status_t status;

-    UCT_EP_KEEPALIVE_CHECK_PARAM(flags, comp);
+    UCT_EP_KEEPALIVE_CHECK_PARAM(flags);


⚠️ MINOR

Behavior change for legacy comp == NULL callers: in the wireup branch the function now returns UCS_INPROGRESS (or UCS_ERR_NO_RESOURCE) via uct_ud_ep_comp_skb_add(..., NULL) instead of the previous UCS_OK. The UCP worker is adjusted, but other NULL-comp callers (e.g. ucp_wireup_ep_check) need to be re-checked for tolerance of the new statuses.

svc-nvidia-pr-review · 2026-05-28T13:17:21Z

    if (status == UCS_ERR_NO_RESOURCE) {
        return 0;
-    } else if (status != UCS_OK) {
+    } else if (UCS_STATUS_IS_ERR(status)) {


⚠️ MINOR

Replacing status != UCS_OK with UCS_STATUS_IS_ERR(status) silently swallows UCS_INPROGRESS here. The call passes comp == NULL, so today no transport returns UCS_INPROGRESS, but the assertion that previously held (status == UCS_OK) is now lost. Worth an ucs_assert(status == UCS_OK) in the success arm to catch regressions.

svc-nvidia-pr-review · 2026-05-28T13:17:22Z

+     * pending req, and the first req's dispatch clears the flag before the
+     * rest run. With comp == NULL we have nothing to fire and the sibling's
+     * keepalive WQE already covers peer-liveness - skip the redundant post. */
+    if ((comp == NULL) && !(ep->flags & UCT_RC_EP_FLAG_KEEPALIVE_PENDING)) {


⚠️ MINOR

The (comp == NULL) && !KEEPALIVE_PENDING skip is only for NULL-comp pending reqs, but with comp != NULL multiple pending reqs can pile up on the same EP and each posts its own keepalive WQE. If a sibling already cleared the flag (peer-liveness was just proven) maybe we should fire the comp with UCS_OK instead of posting a redundant probe?

svc-nvidia-pr-review · 2026-05-28T13:17:23Z

+}
+
+
+UCT_INSTANTIATE_NO_SELF_TEST_CASE(test_uct_ep_check_async)


⚠️ MINOR

The new tests only exercise the UCS_OK comp path. No test exercises the error path (peer dies / EP destroyed → comp fires UCS_ERR_CANCELED / UCS_ERR_CONNECTION_RESET) for any of the new pending-purge code in tcp_ep.c, rc_ep.c, ud_ep.c.

svc-nvidia-pr-review · 2026-05-28T13:17:25Z

+        tcp_pending_req = ucs_derived_of(self, uct_tcp_ep_pending_req_t);
+        status          = (tcp_pending_req->ep->conn_state ==
+                           UCT_TCP_EP_CONN_STATE_CLOSED) ?
+                          UCS_ERR_CONNECTION_RESET : UCS_ERR_CANCELED;


⚠️ MINOR

uct_tcp_ep_pending_purge_cb chooses CONNECTION_RESET for CLOSED state, but on normal EP destroy the destructor flips conn_state to CLOSED before pending_purge runs, so user keepalive comps on destroyed EPs will always report UCS_ERR_CONNECTION_RESET rather than UCS_ERR_CANCELED. Is that the intended status?

svc-nvidia-pr-review · 2026-05-28T13:17:26Z

+    ep->flags &= ~UCT_RC_EP_FLAG_KEEPALIVE_PENDING;
    if (status == UCS_OK) {
-        ep->flags &= ~UCT_RC_EP_FLAG_KEEPALIVE_PENDING;
+        ucs_assert(comp == NULL);


⚠️ MINOR

ucs_assert(comp == NULL) here is true by construction (uct_rc_ep_check_internal returns UCS_INPROGRESS when comp != NULL), but the early if (status == UCS_ERR_NO_RESOURCE) + late if (status == UCS_INPROGRESS) + final error branch reads easier as a switch. Minor readability.

evgeny-leksikov

Need to fix commit comment UCP -> UCT

gleon99 · 2026-05-28T14:25:42Z

    }

-    return status;
+    if (status == UCS_INPROGRESS) {


When a queued ep_check posts, uct_rc_ep_check_progress() returns UCS_INPROGRESS. uct_rc_ep_process_pending() maps that to NEXT_GROUP -> the same pending request stays queued. Further progress can repost w same comp -> purge can cancel a comp that the CQE path will also invoke. Free the pending request and return UCS_OK once the async check is armed?

Posted operations should be removed from pending queue and enqueued to transport level outstanding ops. So, pending purge won't take any effect on it.

In uct_rc_ep_check after uct_rc_ep_check_internal, there's

ucs_assert((comp == NULL) ? (status == UCS_OK) : (status == UCS_INPROGRESS));

gleon99 · 2026-05-28T14:27:39Z

    /* in case if no TX resources are available then there is at least
     * one signaled operation which provides actual peer status, in this case
     * just return without any actions */
    UCT_RC_CHECK_TXQP_RET(iface, ep, UCS_OK);


Returns UCS_OK even when comp != NULL. In debug -> new assertion expects UCS_INPROGRESS; in release -> ignores the user comp. The async path should either attach comp to the relevant outstanding signaled op or keep/retry the check until it can arm a real comp.

In debug -> new assertion expects UCS_INPROGRESS

could you please point which one?

in release -> ignores the user comp

this is the common UCT/UCP design - don't invoke completion if the operation completed immediately.

uct_rc_ep_process_pending() gets UCS_INPROGRESS from the callback -> returns UCS_ARBITER_CB_RESULT_NEXT_GROUP. Then, after uct_rc_ep_check_progress() returns UCS_INPROGRESS, the same uct_pending_req_t still remains on the arbiter, while its comp was already attached to the txqp outstanding op?

evgeny-leksikov added 2 commits May 27, 2026 20:58

GTEST: Refactor completion handling in test_peer_failure

5cfa038

gleon99 requested a review from william-gallagher-nv May 28, 2026 12:56

svc-nvidia-pr-review reviewed May 28, 2026

View reviewed changes

Comment thread src/uct/ib/ud/base/ud_ep.c

svc-nvidia-pr-review reviewed May 28, 2026

View reviewed changes

roiedanino requested a review from svc-nvidia-pr-review May 28, 2026 14:20

evgeny-leksikov changed the title ~~UCP/EP: Enhance uct_ep_check~~ UCT/EP: Enhance uct_ep_check May 28, 2026

evgeny-leksikov commented May 28, 2026

View reviewed changes

gleon99 reviewed May 28, 2026

View reviewed changes

evgeny-leksikov added 2 commits June 3, 2026 11:15

Merge branch 'master' into uct_ud_ep_check_comp

2d5304f

Merge branch 'master' into uct_ud_ep_check_comp

d64a91a

		}


		UCT_INSTANTIATE_NO_SELF_TEST_CASE(test_uct_ep_check_async)

Conversation

evgeny-leksikov commented May 27, 2026

What?

Why?

How?

Uh oh!

roiedanino commented May 28, 2026

Uh oh!

svc-nvidia-pr-review commented May 28, 2026

Uh oh!

roiedanino commented May 28, 2026

Uh oh!

svc-nvidia-pr-review commented May 28, 2026

Uh oh!

svc-nvidia-pr-review left a comment

Choose a reason for hiding this comment

Code Review

General Findings

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeny-leksikov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gleon99 Jun 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gleon99 Jun 4, 2026 •

edited

Loading