Skip to content

wait: with COMMUX_SPIN_US set, a wait past its budget pays an extra COMMUX_WAIT_POLL_MS after completion #12

Closed

Description

@UCzhangxi

Symptom. With COMMUX_SPIN_US set, any wait that outlasts its spin budget pays about one extra COMMUX_WAIT_POLL_MS (default 1 ms) after its request has already completed. This makes spin mode slower than spin off for exactly the waits the budget is meant to bound. With COMMUX_SPIN_US unset (the default) this path is not taken, and waits behave as before #11.

Mechanism (by reading 0994e4e; not yet measured). src/process_group_ucx.cpp, in both UCXWork::wait (:332) and ProcessGroupUCX::wait_request (:564):

while (!reap()) {                       // or: check_status(req) == UCS_INPROGRESS
  if (ucp_worker_progress(worker_)) {
    if (spin.expired()) worker_wait_timed(worker_, spin.deadline());
    continue;
  }
  worker_wait_timed(worker_, spin.deadline());
}

Trace, spin on, message arriving after the budget:

  1. The idle branch parks in poll.
  2. The event wakes it, and ucp_worker_progress completes our request.
  3. spin.expired() is true, so worker_wait_timed is called again before reap() / check_status looks.
  4. Nothing else is pending, so ucp_worker_arm returns UCS_OK, and poll(efd, wait_poll_ms()) runs to its timeout.
  5. Only then does the loop see the completion.

Separately, worker_wait_timed (:100, sleep at :111) sleeps a full wait_poll_ms() on UCS_ERR_BUSY once the deadline has passed. UCS_ERR_BUSY means events are already ready. During a burst this limits progress to about one call per ms, and in wait_request it does so while holding worker_mu_ (collectives).

Reach. Only runs that set COMMUX_SPIN_US > 0. Worst hit: a multi-tensor UCXWork whose later requests complete after the budget, and collectives through wait_request.

Test gap. The spin-budget test in tests/test_commux.cpp (:343) runs only when COMMUX_SPIN_US is set, and .github/workflows/ci.yml exports only COMMUX_MT and COMMUX_DEADLOCK. It is 2-rank and checks CPU time, not latency, so it passes with the defect above.

Expected.

  • After progress advances, check for completion before parking, and park only while the request is still pending.
  • On UCS_ERR_BUSY, return to progress at once (or yield) instead of sleeping.
  • A regression test that measures completion latency for a wait longer than the budget (red on 0994e4e, green after the fix), run in CI with COMMUX_SPIN_US set.

Found in a post-merge review of #11. GitHub Copilot's review of #11 flagged the same park-before-reap point.

Activity

  1. UCzhangxi commented on Sep 30, 2026

    @UCzhangxi
    CollaboratorAuthor

    Resolved by #14, merged as 7a192c7, which removes the opt-in COMMUX_SPIN_US spin entirely. Nothing in production set it, and with it gone the wait path is byte-identical to the one before the spin was added (cf41b46). A late-poll wait past a spin budget can no longer happen.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions