UCT/IB: Fix RoCE LAG put bandwidth estimate by shasson5 · Pull Request #11691 · openucx/ucx

shasson5 · 2026-07-22T15:09:35Z

What?

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.

Impact

This fixes bandwidth estimation for RoCE LAG PUT operations without changing the existing GET/NDR/XDR read bandwidth limits.

shasson5 · 2026-07-26T15:31:51Z

@svc-nvidia-pr-review

svc-nvidia-pr-review · 2026-07-26T15:32:10Z

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

gleon99 · 2026-07-28T13:20:14Z

        if (uct_ib_iface_is_roce(iface)) {
            /* RoCE - number of paths is RoCE LAG level */
            iface->num_paths = uct_ib_iface_roce_lag_level(iface);
+            if (uct_ib_iface_port_is_xdr(iface)) {


if (uct_ib_iface_port_active_speed(iface) > UCT_IB_SPEED_NDR)
or => UCT_IB_SPEED_XDR ?

XDR detection is different than NDR so we cannot merge them to the same function and use it here

shasson5 · 2026-07-28T14:09:35Z

@svc-nvidia-pr-review

svc-nvidia-pr-review · 2026-07-28T14:09:50Z

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

svc-nvidia-pr-review · 2026-07-28T14:13:44Z

Test coverage note: the changed logic (set_num_paths, estimate_path_bw, port_is_xdr) is only exercisable on real XDR/NDR hardware; test/gtest/uct/v2/test_uct_query.cc covers estimate_perf generically but not the new RoCE-XDR width path. Given the classification heuristic above, a small unit test around uct_ib_iface_port_active_width / the XDR detection (feeding known width+speed combos) would guard against regressions. Could you clarify the coverage plan here?

shasson5 · 2026-08-04T08:40:56Z

@svc-nvidia-pr-review

svc-nvidia-pr-review · 2026-08-04T08:41:06Z

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

shasson5 · 2026-08-05T06:16:42Z

@iyastreb can review?

iyastreb · 2026-08-05T13:00:14Z

I'm not comfortable with this statement:

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.

I thought a single RoCE LAG QP/path transmits through one physical port. Full LAG bandwidth requires multiple QPs/paths. Or I miss something?

iyastreb · 2026-08-05T13:08:27Z

            /* RoCE - number of paths is RoCE LAG level */
            iface->num_paths = uct_ib_iface_roce_lag_level(iface);
+            if (uct_ib_iface_port_is_xdr(iface)) {
+                iface->num_paths = UCT_IB_HIGH_SPEED_NUM_PATHS;


So here we essentially reduce the number of possible paths, but why?

because number of required paths for full BW is reduced on XDR

but what if it is a LAG consisting of, say 3 devices?

iyastreb · 2026-08-05T13:14:02Z

-    if (uct_ib_iface_is_roce(iface) &&
-        (uct_ib_iface_roce_lag_level(iface) > 1)) {
+    if (uct_ep_op_is_get(op) && uct_ib_iface_port_is_xdr(iface)) {
+        max_path_bandwidth = UCT_IB_XDR_READ_PATH_BANDWIDTH;


But this also modifies non-RoCE code..

actually it doesn't because it's just lines reorder (last "else" case moved to the top)

why is this change needed?
imo it is harder to read now

shasson5 · 2026-08-06T08:18:24Z

I'm not comfortable with this statement:

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.

I thought a single RoCE LAG QP/path transmits through one physical port. Full LAG bandwidth requires multiple QPs/paths. Or I miss something?

CX8 has four planes per port, and DDP allows a single QP/WQE to spray packets across all planes. RDMA WRITE payload is placed immediately, whereas RDMA READ requires responder execution and response generation.

PUT is a streaming, packet-sprayed operation; GET is a request/response operation bounded by the per-QP read-credit window. On XDR that window is 16, so four QPs recreate 64-way concurrency.

https://nvidia.atlassian.net/wiki/spaces/SW/pages/2899157185/DDP+-+Direct+Data+Placement
Running on Lyris — explicitly states that ucp_get requires at least four QPs

shasson5 · 2026-08-09T08:04:13Z

@iyastreb

brminich

did you run some basic perf tests (osu?) with XDR and NDR

brminich · 2026-08-10T15:15:20Z

            /* RoCE - number of paths is RoCE LAG level */
            iface->num_paths = uct_ib_iface_roce_lag_level(iface);
+            if (uct_ib_iface_port_is_xdr(iface)) {
+                iface->num_paths = UCT_IB_HIGH_SPEED_NUM_PATHS;


but what if it is a LAG consisting of, say 3 devices?

brminich · 2026-08-10T15:18:50Z

-    if (uct_ib_iface_is_roce(iface) &&
-        (uct_ib_iface_roce_lag_level(iface) > 1)) {
+    if (uct_ep_op_is_get(op) && uct_ib_iface_port_is_xdr(iface)) {
+        max_path_bandwidth = UCT_IB_XDR_READ_PATH_BANDWIDTH;


why is this change needed?
imo it is harder to read now

brminich · 2026-08-10T15:20:38Z

+    uint32_t active_speed = uct_ib_iface_port_active_speed(iface);
+    uint8_t active_width  = uct_ib_iface_is_roce(iface) ?
+                            uct_ib_iface_port_active_width(iface) : 1;
+
+    return (active_width * active_speed) == UCT_IB_SPEED_XDR;


shasson5 force-pushed the fix-doris-put-get-estimation branch from 54a47f5 to 8b04ca4 Compare July 23, 2026 12:05

shasson5 added the WIP-DNM Work in progress / Do not review label Jul 26, 2026

shasson5 force-pushed the fix-doris-put-get-estimation branch from 42b0d01 to 22a708c Compare July 26, 2026 15:29

shasson5 marked this pull request as ready for review July 26, 2026 15:30

svc-nvidia-pr-review reviewed Jul 26, 2026

View reviewed changes

Comment thread src/uct/ib/base/ib_iface.c

Comment thread src/uct/ib/base/ib_iface.h

gleon99 reviewed Jul 28, 2026

View reviewed changes

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

View reviewed changes

Comment thread src/uct/ib/base/ib_iface.h

Comment thread src/uct/ib/base/ib_iface.c

shasson5 force-pushed the fix-doris-put-get-estimation branch from 0ba6e1a to 2667a53 Compare August 4, 2026 08:37

UCT/IB: Use RoCE width for XDR detection

8d51df3

shasson5 force-pushed the fix-doris-put-get-estimation branch from 2667a53 to 8d51df3 Compare August 4, 2026 08:40

shasson5 removed the WIP-DNM Work in progress / Do not review label Aug 4, 2026

svc-nvidia-pr-review reviewed Aug 4, 2026

View reviewed changes

Comment thread src/uct/ib/base/ib_iface.c

Comment thread src/uct/ib/base/ib_iface.h

shasson5 requested review from gleon99 and iyastreb August 4, 2026 09:22

iyastreb reviewed Aug 5, 2026

View reviewed changes

UCT/IB: Clean up active width query

2bf1fd0

shasson5 requested a review from iyastreb August 6, 2026 09:02

brminich reviewed Aug 10, 2026

View reviewed changes

Uh oh!

Conversation

shasson5 commented Jul 22, 2026

What?

Why?

Impact

Uh oh!

shasson5 commented Jul 26, 2026

Uh oh!

svc-nvidia-pr-review commented Jul 26, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shasson5 commented Jul 28, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

shasson5 commented Aug 4, 2026

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

shasson5 commented Aug 5, 2026

Uh oh!

iyastreb commented Aug 5, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shasson5 Aug 6, 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!

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!

shasson5 commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shasson5 commented Aug 9, 2026

Uh oh!

brminich 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shasson5 Aug 6, 2026 •

edited

Loading

shasson5 commented Aug 6, 2026 •

edited

Loading