Skip to content

TEST/UCP: Allow non-contiguous mlx5 devices - #11900

Open
william-gallagher-nv wants to merge 5 commits into
openucx:masterfrom
william-gallagher-nv:allow_sparse_device
Open

TEST/UCP: Allow non-contiguous mlx5 devices#11900
william-gallagher-nv wants to merge 5 commits into
openucx:masterfrom
william-gallagher-nv:allow_sparse_device

Conversation

@william-gallagher-nv

Copy link
Copy Markdown
Contributor

Removes assumption that mlx5 devices always start at mlx5_0 and go up to mlx5_n with a valid device in each slot. This allows for testing setups with sparse configurations

What?

Removes assumption that mlx5 devices always start at mlx5_0 and go up to mlx5_n with a valid device in each slot. This allows for testing setups with sparse configurations.

Why?

Blossom CI setup configures a single virtualized device per pod typically not mlx5_0. For example, mlx5_18.

Removes assumption that mlx5 devices always start at mlx5_0 and go up to
mlx5_n with a valid device in each slot. This allows for testing setups
with sparse configurations
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_context.cc
@guy-ealey-morag

guy-ealey-morag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

This change is not enough to make tests pass because the tests use devices from mlx5_0 up to mlx5_<count - 1> (this is why this assumption is being asserted).
Many tests require 2 or 3 devices so they would skip on a setup with 1 device, but would fail if there are 2 or more.

@william-gallagher-nv

Copy link
Copy Markdown
Contributor Author

This change is not enough to make tests pass because the tests use devices from mlx5_0 up to mlx5_<count - 1>. Many tests require 2 or 3 devices so they would skip on a setup with 1 device, but would fail if there are 2 or more.

Yes, that makes sense. The scenario that I encountered is a single mlx5 device, something like mlx5_10, so the tests pass with my change. A scenario with mlx5_0, mlx5_2 and mlx5_4 would break the tests. What if change the check in get_mlx5_device_count to if (count > 1)?

@guy-ealey-morag

Copy link
Copy Markdown
Contributor

This change is not enough to make tests pass because the tests use devices from mlx5_0 up to mlx5_<count - 1>. Many tests require 2 or 3 devices so they would skip on a setup with 1 device, but would fail if there are 2 or more.

Yes, that makes sense. The scenario that I encountered is a single mlx5 device, something like mlx5_10, so the tests pass with my change. A scenario with mlx5_0, mlx5_2 and mlx5_4 would break the tests. What if change the check in get_mlx5_device_count to if (count > 1)?

I think that if (count > 1) may be a bit fragile.
I suggest keeping the check that devices are between 0 to max_id, but if they are not then call UCS_TEST_SKIP_R.
Then add a comment explaining that this condition is satisfied on real setups, and virtualized devices may be of any idx.

@svc-ucx

svc-ucx commented Sep 4, 2026

Copy link
Copy Markdown

🤖 CI Triage AgentUCX PR (AddressSanitizer roce on worker 2) · commit 44c4828b

TL;DR: The roce on worker 2 ASAN job wasn't slow — it hung: dcx/test_ucp_am_nbx_reply_always.multi_zcopy/0 <dc_x> produced its [ RUN ] line at 15:21:40 and then emitted nothing for ~12 minutes until the agent was torn down, consistent with the UCP EP-close/flush never completing and the test spinning in the unbounded progress loop in entity::close_all_eps().

Full analysis

Summary: Build 134366 was killed after a 735-second silence while running dcx/test_ucp_am_nbx_reply_always.multi_zcopy/0 <dc_x> (multi-fragment AM zcopy with UCP_AM_SEND_FLAG_REPLY, ZCOPY_THRESH=1, RNDV_THRESH=inf, MAX_EAGER_LANES=2).

Root cause: Evidence from the log: last application output is 2026-09-04T15:21:40.2504427Z [ RUN ] dcx/test_ucp_am_nbx_reply_always.multi_zcopy/0 <dc_x>; next line is 2026-09-04T15:33:55.5723478Z ##[error]The agent has received a shutdown signal — a single ~12 min gap that is essentially the whole tail of the job, i.e. a hang, not slowness (the gtest watchdog is 900 s, so it hadn't fired yet either). All waits inside the test body are deadline-bounded (wait_for_cond/wait_for_value = 10 s, request_process uses ucs::get_deadline()), and none of their failure messages (request %p did not complete on time, EXPECT_EQ output) were printed. The only unbounded wait reachable in this path is the teardown loop while (!is_request_completed(req)) { test.progress(worker_idx); } in ucp_test_base::entity::close_all_eps(), which spins forever if a UCP EP-close (and its implicit flush) never completes — i.e. an outstanding multi-fragment AM zcopy operation on the dc_mlx5 lane never completed. The PR under test (#11900, "TEST/UCP: Allow non-contiguous mlx5 devices") is a test-harness change to IB device enumeration, so it changes which/how many mlx5 devices the UCP entities use; with MAX_EAGER_LANES=2 set by test_ucp_am_base::init() the AM fragments are now striped over an additional device, which is the plausible trigger for the never-completing send on this RoCE worker.

Implicated commit: unknown for the stuck flush itself. Candidates to check first: 73a6a5c9 "UCT/IB/MLX5: Handle relaxed-only fence ordering (#11721)" (Roie Danino, 2026-09-02 — two days before this build, touches the dc_mlx5 fence/flush path) and the AM multi-fragment work ef265cdc (#11160) / 2bafbb02 (#11452). The PR's own test-only device-enumeration change is the trigger, not the defect.

File: test/gtest/ucp/ucp_test.cc:917-921 (unbounded close wait); hanging test at test/gtest/ucp/test_ucp_am.cc:1124-1132 (test_ucp_am_nbx_reply_always.multi_zcopy)

Suggested fix:

  1. Make the failure diagnosable instead of CI-fatal: respect the deadline in the inner loop of close_all_eps(), e.g. while (!is_request_completed(req) && (ucs_get_time() < deadline)) { test.progress(worker_idx); }, then let the existing EXPECT_TRUE(m_close_ep_reqs.empty()) fail with "endpoints were not closed". This converts the hang into a normal test failure with logs.
  2. Reproduce locally on the two-device RoCE setup with the PR's device selection: UCX_TLS=dc_x UCX_MAX_EAGER_LANES=2 UCX_ZCOPY_THRESH=1 UCX_RNDV_THRESH=inf ./gtest --gtest_filter='dcx/test_ucp_am_nbx_reply_always.multi_zcopy*' and dump ucp_ep/lane state plus dc_mlx5 outstanding counters at the hang to identify the lane whose flush never completes.
  3. If the second (newly enabled) mlx5 device is the one stalling, gate the harness change so tests only add extra IB devices that pass a reachability/loopback check, rather than adding all non-contiguous mlx5 devices unconditionally.

Related: PR #11900 (the change under test, #11900); note test_ucp_am_nbx_send_copy_header.all_protos in the same file is already permanently skipped with the comment "FIXME: Disabled due to unresolved failure - CI Hang" (test/gtest/ucp/test_ucp_am.cc:1286-1289), suggesting a pre-existing hang in the AM pending/multi-fragment path.

Skip mlx5 range checks when devices are not continguous from mlx5_0 to
mlx5_{max_idx1}. This condition can appear in virtualized environments.
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_context.cc Outdated
Comment thread test/gtest/ucp/test_ucp_context.cc
Fix comment alignment and Whitespace.
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-nvidia-pr-review

Copy link
Copy Markdown

🤖 Review complete — no issues found.

Comment thread test/gtest/ucp/test_ucp_context.cc Outdated
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_context.cc
Comment thread test/gtest/ucp/test_ucp_context.cc
Comment thread test/gtest/ucp/test_ucp_context.cc Outdated
/* Assuming we have all devices from 0 to max_idx */
EXPECT_EQ(max_idx + 1, count) << "Expected " << (max_idx + 1)
<< " mlx5 devices, found: " << count;
/*

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/*
/* Range tests assume devices mlx5_0 .. mlx5_<max_idx>. This assumption
* can break in virtualized environments with devices that may use any
* idx. */

minor: keep one comment style; other block comments in this file don't open with a bare /*.

@svc-nvidia-pr-review

Copy link
Copy Markdown

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

Comment thread test/gtest/ucp/test_ucp_context.cc
Comment thread test/gtest/ucp/test_ucp_context.cc
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