UCP/RNDV: Adjust max_frag to be at least of minimal RNDV chunk size #10407

iyastreb · 2025-01-07T10:49:02Z

What?

This PR addresses an assertion failure described in https://jirasw.nvidia.com/browse/UCX-1054
Assertion happens when running io-demo with cuda support, currently it's reproducible only on rock05.
Steps to reproduce:

UCX_TLS=rc,cuda UCX_IB_NUM_PATHS=1 UCX_RC_MAX_GET_ZCOPY=32k ./bin/io_demo -d 32768 -p 20000

UCX_TLS=rc,cuda UCX_IB_NUM_PATHS=2 UCX_RC_MAX_GET_ZCOPY=32k ./bin/io_demo -m cuda -i 1 -d 32768 -o read 1.1.60.5:20000

This results in an assertion failure on client side (if read operation is requested) or server side (with write operation):

[rock05:2421956:0:2421956] proto_rndv.inl:286  Assertion `max_payload <= lpriv->max_frag' failed: req=0x147ad40 max_payload=16384 max_frag=9728                                                                                    

/labhome/iyastrebov/ws/ucx4/bld-devel/src/ucp/../../../src/ucp/rndv/proto_rndv.inl: [ ucp_proto_rndv_bulk_max_payload() ]                                                                                                          
      ...                                               
      283                   total_length, lpriv->max_frag_sum, max_frag_sum, max_payload);                                                                                                                                         
      284                                               
      285     /* Check that send length is not greater than maximal fragment size */                                                                                                                                               
==>   286     ucs_assertv(max_payload <= lpriv->max_frag,                                                        
      287                 "req=%p max_payload=%zu max_frag=%zu", req, max_payload,                               
      288                 lpriv->max_frag);                                                                      
      289     return max_payload;                       

==== backtrace (tid:2421956) ====                       
 0 0x000000000010574a ucp_proto_rndv_bulk_max_payload()  /labhome/iyastrebov/ws/ucx4/bld-devel/src/ucp/../../../src/ucp/rndv/proto_rndv.inl:286                                                                                    
 1 0x000000000010574a ucp_proto_rndv_bulk_max_payload_align()  /labhome/iyastrebov/ws/ucx4/bld-devel/src/ucp/../../../src/ucp/rndv/proto_rndv.inl:315                                                                              
 2 0x000000000010574a ucp_proto_rndv_get_zcopy_send_func()  /labhome/iyastrebov/ws/ucx4/bld-devel/src/ucp/../../../src/ucp/rndv/rndv_get.c:153                                                                                     
 3 0x00000000001072a4 ucp_proto_multi_progress()  /labhome/iyastrebov/ws/ucx4/bld-devel/../src/ucp/proto/proto_multi.inl:182                                                                                                       
 4 0x00000000001072a4 ucp_proto_multi_zcopy_progress()  /labhome/iyastrebov/ws/ucx4/bld-devel/../src/ucp/proto/proto_multi.inl:251

Why?

The issue boils down to the corner case when lpriv->max_frag is smaller than min_rndv_chunk. This happens due to low max_zcopy limit that we set with UCX_RC_MAX_GET_ZCOPY=32k config.

How?

Adjust max_frag to be at least of min_rndv_chunk size.
Tested with io-demo
Reproduced issue with test_ucp_proto_mock_rcx, but cannot append this unit test here because it requires #10369

yosefe · 2025-01-07T11:04:52Z

Can we reproduce it in gtest?

iyastreb · 2025-01-07T13:52:50Z

Can we reproduce it in gtest?

Yes, as I wrote in description I reproduced it with mock test
But I cannot commit this test, because it depends on #10369 which is not merged yet

tvegas1 · 2025-01-09T14:30:14Z

src/ucp/proto/proto_common.c

@@ -382,7 +382,9 @@ ucp_proto_common_get_lane_perf(const ucp_proto_common_init_params_t *params,
                                                       &perf_attr.latency) +
                                  params->latency;
    tl_perf->sys_latency        = 0;
-    tl_perf->min_length         = ucs_max(params->min_length, tl_min_frag);
+    /* min_length must be within [tl_min_frag, tl_max_frag] range */


i guess comment can be removed

tvegas1 · 2025-01-09T14:42:48Z

src/ucp/proto/proto_common.c

@@ -382,7 +382,9 @@ ucp_proto_common_get_lane_perf(const ucp_proto_common_init_params_t *params,
                                                       &perf_attr.latency) +
                                  params->latency;
    tl_perf->sys_latency        = 0;
-    tl_perf->min_length         = ucs_max(params->min_length, tl_min_frag);
+    /* min_length must be within [tl_min_frag, tl_max_frag] range */
+    tl_perf->min_length         = ucs_max(ucs_min(params->min_length, tl_max_frag),


is this specific range forcing covered by some parameter combination/gtest?

No, it's not covered.. This is rather a common sense to keep the min_length within the HW limit

i think that if params->min_length > tl_max_frag, the protocol should be disabled: it requires a fragment length that is not supported.

tvegas1 · 2025-01-09T14:48:28Z

src/ucp/proto/proto_multi.c

+        min_rndv_chunk  = lane_perf->bandwidth *
+                          context->config.ext.min_rndv_chunk_size /
+                          min_bandwidth;
+        /* Minimal RNDV chunk must be within [min_length, tl_max_frag] range */


maybe something more like: "we still to operate within iface/hw limits" or something, else it repeats the code a bit

I better remove that comment

tvegas1 · 2025-01-09T14:50:09Z

src/ucp/proto/proto_multi.c

+        /* For RNDV only: max scaled fragment must be at least min_rndv_chunk */
+        if ((params->super.send_op == UCT_EP_OP_PUT_ZCOPY) ||
+            (params->super.send_op == UCT_EP_OP_GET_ZCOPY)) {
+            max_frag = ucs_max(max_frag, min_rndv_chunk);


this is the actual fix, right? we have also test for that in maybe mock?

Yes, this line is the real fix, other changes are just the boundary checks
Yes, I have a test, but as written in description I cannot commit it until #10369 is merged

seems weird that proto_muti.c has a specific case for rndv.

we should not increase the size to be more than transport max frag, or the transport may not be able to send it. Maybe in the case here it works because UCX_RC_MAX_GET_ZCOPY is not "real" HW limitation but just a SW config.

Agree, it looks a bit weird, but it does the right thing: we adjust max_frag only for RNDV, according to the minimal RNDV chunk size.

No, it shouldn't increase max_frag above tl_max_frag, here is the reasoning:
Initial value of max_frag is capped by tl_max_frag:

max_frag = ucs_double_to_sizet(lane_perf->bandwidth / max_frag_ratio, lane_perf->max_frag);

lane_perf->min_length is guaranteed to be within [tl_min_frag, tl_max_frag]
=> min_rndv_chunk is guaranteed to be within [min_length, tl_max_frag]
=> max_frag = ucs_max(max_frag, min_rndv_chunk) can never exceed tl_max_frag

Can we move this code to rndv protocol or make it more generic? maybe using a flag in params?

I'm afraid moving this code outside is gonna be very hard, because other calculations are tightly coupled with max_frag. Maybe in the future we can refactor this function, as it does a lot of things.

Using flag in params seems viable option to me, and btw there are already suitable flags, indicating that RNDV is used: UCP_PROTO_COMMON_INIT_FLAG_SEND_ZCOPY and UCP_PROTO_COMMON_INIT_FLAG_RECV_ZCOPY. And the end it will be the same as checking send_op

I have refactored code around min_chunk:

A separate function ucp_proto_multi_get_min_chunk returns min_rndv_chunk_size for RNDV protocols, 0 otherwise

ucp_proto_multi_init is pure generic wrt min_chunk

yosefe · 2025-01-13T16:46:19Z

src/ucp/proto/proto_common.c

+        ucs_debug("protocol %s min_length %zu is larger than lane[%d] max_frag "
+                  "%zu", ucp_proto_id_field(params->super.proto_id, name),
+                  params->min_length, lane, tl_max_frag);
+        return UCS_ERR_OUT_OF_RANGE;


UCS_ERR_INVALID_PARAM - params->min_length is invalid

yosefe · 2025-01-13T16:47:13Z

src/ucp/proto/proto_multi.c

+        /* For RNDV only: max scaled fragment must be at least min_rndv_chunk */
+        if ((params->super.send_op == UCT_EP_OP_PUT_ZCOPY) ||
+            (params->super.send_op == UCT_EP_OP_GET_ZCOPY)) {
+            max_frag = ucs_max(max_frag, min_rndv_chunk);


Can we move this code to rndv protocol or make it more generic? maybe using a flag in params?

yosefe · 2025-01-14T10:53:56Z

src/ucp/proto/proto_common.c

@@ -355,6 +355,10 @@ ucp_proto_common_get_lane_perf(const ucp_proto_common_init_params_t *params,

    ucp_proto_common_get_frag_size(params, &wiface->attr, lane, &tl_min_frag,
                                   &tl_max_frag);
+    if (params->min_length > tl_max_frag) {
+        ucs_debug("params->min_length=%zu is invalid", params->min_length);


pls print more details

yosefe · 2025-01-14T10:55:14Z

src/ucp/proto/proto_multi.c

+{
+    ucp_context_h context = params->super.super.worker->context;
+
+    if (params->super.flags & (UCP_PROTO_COMMON_INIT_FLAG_SEND_ZCOPY |


can we take it from params->min_length?

UCP/RNDV: Adjust max_frag to be at least of minimal RNDV chunk size

9dba379

iyastreb requested review from tvegas1 and yosefe January 7, 2025 10:49

tvegas1 reviewed Jan 9, 2025

View reviewed changes

iyastreb added 3 commits January 9, 2025 19:40

UCP/RNDV: Addressed PR comments

127cdff

UCP/RNDV: Don't select protocol if min_length > max_frag

5f7e5ce

UCP/RNDV: Fixed typo

5fe2171

yosefe reviewed Jan 13, 2025

View reviewed changes

iyastreb added 2 commits January 14, 2025 07:24

UCP/RNDV: Minor change

423c3f0

UCP/RNDV: Refactor min_chunk processing

5ca4485

yosefe reviewed Jan 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCP/RNDV: Adjust max_frag to be at least of minimal RNDV chunk size #10407

UCP/RNDV: Adjust max_frag to be at least of minimal RNDV chunk size #10407

iyastreb commented Jan 7, 2025

yosefe commented Jan 7, 2025

iyastreb commented Jan 7, 2025

tvegas1 Jan 9, 2025

iyastreb Jan 9, 2025

tvegas1 Jan 9, 2025

iyastreb Jan 9, 2025

yosefe Jan 12, 2025

iyastreb Jan 13, 2025

tvegas1 Jan 9, 2025

iyastreb Jan 9, 2025

tvegas1 Jan 9, 2025

iyastreb Jan 9, 2025

yosefe Jan 12, 2025

iyastreb Jan 13, 2025

yosefe Jan 13, 2025

iyastreb Jan 14, 2025

iyastreb Jan 14, 2025

yosefe Jan 13, 2025

iyastreb Jan 14, 2025

yosefe Jan 13, 2025

yosefe Jan 14, 2025

yosefe Jan 14, 2025

UCP/RNDV: Adjust max_frag to be at least of minimal RNDV chunk size #10407

Are you sure you want to change the base?

UCP/RNDV: Adjust max_frag to be at least of minimal RNDV chunk size #10407

Conversation

iyastreb commented Jan 7, 2025

What?

Why?

How?

yosefe commented Jan 7, 2025

iyastreb commented Jan 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment