Skip to content

UCS/TOPO: Integrate GPU-NIC assignment in proto multi - #11984

Closed
guy-ealey-morag wants to merge 104 commits into
openucx:masterfrom
guy-ealey-morag:topo-assign-proto-direct-rma
Closed

guy-ealey-morag wants to merge 104 commits into
openucx:masterfrom
guy-ealey-morag:topo-assign-proto-direct-rma

Conversation

@guy-ealey-morag

Copy link
Copy Markdown
Contributor

What?

Describe what this PR is doing.

Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
Signed-off-by: Guy Ealey Morag <gealeymorag@nvidia.com>
@svc-nvidia-pr-review

Copy link
Copy Markdown

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

@svc-ucx

svc-ucx commented Sep 23, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Coverity coverity devel on coverity_rh7) · commit f2fd9f67

TL;DR: The Coverity (devel) job failed because cov-analyze reported 1 new DEADCODE defect in test/gtest/ucp/test_ucp_proto_mock.cc:2756 — a return; that can never be reached because the preceding ASSERT_UCS_OK(status) always throws inside the status != UCS_OK branch; drop the redundant return; (or move the assert out of the if).

Full analysis

Summary: Coverity static-analysis stage "coverity devel on coverity_rh7" failed with ##[error]Coverity found 1 issues: /__w/14/s/test/gtest/ucp/test_ucp_proto_mock.cc:2756 Type: Logically dead code (DEADCODE). The build and 745-unit compilation itself succeeded; only the defect gate failed (nerrors=1 → exit 1).

Root cause: In mock fixture::install_assignment():

status = ucp_gpu_nic_assignment_build(&groups, ..., assignment);
if (status != UCS_OK) {          /* 2753 */
    ucs_free(assignment);        /* 2754 */
    ASSERT_UCS_OK(status);       /* 2755 */
    return;                      /* 2756  <-- dead */
}

ASSERT_UCS_OK expands (via _ASSERT_UCS_STATUS in test/gtest/common/test_helpers.h:91-98) to if (!(__status == UCS_OK)) UCS_TEST_ABORT(...), and UCS_TEST_ABORT unconditionally throws a ucs::test_abort_exception. Inside the branch, status is provably != UCS_OK, so the assert always throws and control can never reach line 2756. Coverity's trace states exactly this ("cond_cannot_single … dead_error_line: Execution cannot reach this statement: return;").

Implicated commit: 6fe817d6 "TEST/UCP: Release groups on fail, check nic out of bound" (Guy Ealey Morag) — which introduced the free-then-assert-then-return cleanup path; the enclosing fixture came from f4f6ba83 "TEST/UCP: Add GPU-NIC protocol mock fixture" (same author). Both are on branch topo-assign-proto-direct-rma (head f2fd9f67).

File: test/gtest/ucp/test_ucp_proto_mock.cc:2753-2757 (defect reported at 2756); macro definition at test/gtest/common/test_helpers.h:91-98

Suggested fix: Remove the unreachable return;, and preferably restructure so the assert is outside the branch so the intent (free then fail) is explicit:

status = ucp_gpu_nic_assignment_build(
        &groups, UCP_GPU_NIC_ASSIGNMENT_POLICY_FLIP, assignment);
if (status != UCS_OK) {
    ucs_free(assignment);
}
ASSERT_UCS_OK(status);

Even better, wrap assignment in a ucs::handle<ucp_gpu_nic_assignment_t*>/std::unique_ptr with ucs_free as the deleter so the throwing assert cannot leak it, then release ownership into m_assignment on success. Re-push to clear the Coverity gate.

Related: none found (searches for prior DEADCODE/ASSERT_UCS_OK reports only returned unrelated PRs #11857, #11836)

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.

3 participants