Skip to content

UCT/IB: Fix RoCE LAG put bandwidth estimate - v1.23.x - #11863

Merged
shasson5 merged 1 commit into
openucx:v1.23.xfrom
shasson5:backport-pr11691-v1.23.x
Aug 31, 2026
Merged

UCT/IB: Fix RoCE LAG put bandwidth estimate - v1.23.x#11863
shasson5 merged 1 commit into
openucx:v1.23.xfrom
shasson5:backport-pr11691-v1.23.x

Conversation

@shasson5

@shasson5 shasson5 commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What?

Backport of #11691 to v1.23.x.

  • Keep RoCE LAG path bandwidth at full device bandwidth for PUT operations
  • Continue splitting path bandwidth by dev_num_paths for non-PUT RoCE LAG operations

Why?

PUT operations can use the full RoCE LAG bandwidth, so dividing the estimate by dev_num_paths makes the protocol selection logic underrate PUT relative to GET.

@svc-nvidia-pr-review

svc-nvidia-pr-review commented Aug 31, 2026

Copy link
Copy Markdown

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

svc-nvidia-pr-review
svc-nvidia-pr-review reviewed Aug 31, 2026
View reviewed changes
Comment thread test/gtest/uct/ib/test_ib.cc
@svc-nvidia-pr-review

svc-nvidia-pr-review commented Aug 31, 2026

Copy link
Copy Markdown

src/uct/ib/base/ib_iface.c

Repeated ibv_query_port_speed calls per estimate_perf: uct_ib_iface_is_multiplane_xdr_bw() calls uct_ib_iface_query_port_speed_gbps() (an ibv_query_port_speed syscall) on every invocation. In a single uct_ib_iface_estimate_perf() it can be called once directly plus once via estimate_path_bw, and estimate_bandwidth queries the speed again — so 2-3 syscalls per query. This is not a data-path function, so it is minor, but the multiplane flag and speed could be cached at iface init since neither changes at runtime.

@svc-ucx

svc-ucx commented Aug 31, 2026

Copy link
Copy Markdown

🤖 CI Triage Agent — UCX PR (Codestyle AUTHORS file update check) · commit c2c6ea84

TL;DR: The "AUTHORS file check" failed because the backport commit c2c6ea8 is authored as shasson5 <103439971+shasson5@users.noreply.github.com> (a GitHub noreply identity) which isn't recognized by contrib/authors_update.sh, so the script appended a new AUTHORS line and git diff --exit-code returned 1. Fix by re-authoring the backport commit with the canonical identity (Shachar Hasson <shasson@nvidia.com>, already in AUTHORS) or by committing the AUTHORS update.

Full analysis

Summary: Codestyle stage "AUTHORS file check" exited 1 — git diff --exit-code found an uncommitted AUTHORS modification after running contrib/authors_update.sh 67a58abaa..c2c6ea844.

Root cause: This is not a code bug; it is an author-identity mismatch. The check computes the PR commit range and runs contrib/authors_update.sh, which for each author does grep -iqw "$email" and grep -iq "$name" against the existing AUTHORS entries. The backport commit's author is shasson5 <103439971+shasson5@users.noreply.github.com> (log line: ++ shasson5 <103439971+shasson5@users.noreply.github.com>), i.e. the commit was created through GitHub (web UI / backport automation) rather than preserving the original author identity. Neither test matches: the noreply email is absent from AUTHORS, and the name shasson5 is not a substring of the existing entry Shachar Hasson <shasson@nvidia.com> (AUTHORS:116) — shasson@ ≠ shasson5. The script therefore appends shasson5 <103439971+shasson5@users.noreply.github.com> to AUTHORS, leaving a dirty tree, and git diff --exit-code fails the stage. The same PR on master would have passed only if it carried the canonical author, so this failure is specific to how the v1.23.x backport commit was created.

Implicated commit: [REDACTED:Hex High Entropy String] (author shasson5, backport of PR #11691 onto v1.23.x); the check logic itself comes from f8c7e7a9 (Thomas Vegas, "CONTRIB: Automate AUTHORS file update") and cfd9cd1f (Yossi Itigin, "BUILD/CI: Limit PR commit range checks", #11527).

File: contrib/authors_update.sh:36 (identity match), AUTHORS:116 (existing canonical entry)

Suggested fix: Preferred, per-PR fix — re-author the backport commit with the identity already in AUTHORS and force-push:
git commit --amend --author="Shachar Hasson <shasson@nvidia.com>" --no-edit && git push -f
(or configure the backport tooling / git config user.email shasson@nvidia.com so noreply addresses are never used as commit author). Alternatively, run ./contrib/authors_update.sh <base>..<head> locally and commit the resulting AUTHORS change to satisfy the gate.

Durable fix to stop this class of failure: add a .mailmap at repo root mapping the GitHub noreply address to the canonical identity, e.g.
Shachar Hasson <shasson@nvidia.com> <103439971+shasson5@users.noreply.github.com>
and make contrib/authors_update.sh:18 mailmap-aware by using --pretty=format:"%aN%x09%aE" (capital N/E) instead of %an%x09%ae. This also hardens the fragile substring match for future contributors whose GitHub username differs from their AUTHORS name.

Related: PR #11863 (this backport), original PR #11691, #11527 (limited PR commit range checks); no existing issue found tracking noreply-author AUTHORS failures.

🛡️ This comment had 1 potential secret(s) redacted (Hex High Entropy String). See request_id 4148da1d-6dd5-465b-9e48-6a3af51df7f7 in the triage console for the audit trail.

@shasson5
shasson5 requested a review from tvegas1 August 31, 2026 13:14
@shasson5
shasson5 force-pushed the backport-pr11691-v1.23.x branch from c2c6ea8 to 7a246f1 Compare August 31, 2026 13:31
@svc-nvidia-pr-review

svc-nvidia-pr-review commented Aug 31, 2026

Copy link
Copy Markdown

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

svc-nvidia-pr-review
svc-nvidia-pr-review reviewed Aug 31, 2026
View reviewed changes
Comment thread src/uct/ib/base/ib_iface.c
Comment thread src/uct/ib/base/ib_iface.c
@svc-nvidia-pr-review

svc-nvidia-pr-review commented Aug 31, 2026

Copy link
Copy Markdown

Test coverage note: the new test_uct_ib_perf tests only exercise the multiplane branches when the underlying hardware actually reports an 800 Gbps multiplane port; otherwise they fall back to the plain equal-bandwidth assertion. On CI runners without such hardware the multiplane get/put bandwidth logic is not covered. If practical, consider a way to force UCT_IB_DEVICE_FLAG_MULTIPLANE / speed in the test so the new branch is exercised deterministically.

@shasson5
shasson5 enabled auto-merge (squash) August 31, 2026 15:41
@shasson5
shasson5 merged commit c1149a2 into openucx:v1.23.x Aug 31, 2026
162 checks passed
@svc-ucx svc-ucx mentioned this pull request Sep 7, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tvegas1 tvegas1 tvegas1 approved these changes
+1 more reviewer
@svc-nvidia-pr-review svc-nvidia-pr-review svc-nvidia-pr-review left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@shasson5 @svc-nvidia-pr-review @svc-ucx @tvegas1