UCT/IB/EFA/SRD: Initial interface add #10412

tvegas1 · 2025-01-10T19:03:14Z

What?

Initial add of SRD transport with bare interface instantiation with QP, tested like below.

Depends on #10069 to implement build and testing in CI.

Test

$ valgrind ./install/bin/ucx_info -d
...
#      Transport: srd
#         Device: rdmap1:1
#           Type: network
#  System device: <unknown>
#
#      capabilities:
#            bandwidth: 11797.08/ppn + 0.00 MB/sec
#              latency: 630 nsec
#             overhead: 105 nsec
#            get_bcopy: <= 4K
...

yosefe · 2025-01-12T09:52:54Z

src/uct/ib/base/ib_iface.h

+#if HAVE_EFA
+    UCT_IB_QPT_SRD,
+#else
+    UCT_IB_QPT_SRD = UCT_IB_QPT_UNKNOWN,


Do we really need to define UCT_IB_QPT_SRD is HAVE_EFA is 0?

Shall we define UCT_IB_QPT_SRD to IBV_QPT_DRIVER? Or unify them somehow? Seems weird to define UCT_IB_QPT_SRD without any reference to an IB macro

yosefe · 2025-01-12T09:54:23Z

src/uct/ib/efa/Makefile.am

@@ -15,9 +15,12 @@ libuct_ib_efa_la_LIBADD   = $(top_builddir)/src/ucs/libucs.la \
                            $(top_builddir)/src/uct/libuct.la \
                            $(top_builddir)/src/uct/ib/libuct_ib.la

-libuct_ib_efa_la_SOURCES  = ib_efa_md.c
+libuct_ib_efa_la_SOURCES  = ib_efa_md.c \


shall we move this to efa/base, now that we added the srd subdir?
to align with other transports

yosefe · 2025-01-12T09:55:03Z

src/uct/ib/efa/srd/srd_def.h

+typedef ucs_frag_list_sn_t uct_srd_psn_t;
+typedef struct uct_srd_iface uct_srd_iface_t;
+typedef struct uct_srd_iface_addr uct_srd_iface_addr_t;
+typedef struct uct_srd_iface_peer uct_srd_iface_peer_t;
+typedef struct uct_srd_ep_addr uct_srd_ep_addr_t;
+typedef struct uct_srd_send_op uct_srd_send_op_t;
+typedef struct uct_srd_send_desc uct_srd_send_desc_t;


do we really need all these forward declarations (and in this PR)?

removed for now

yosefe · 2025-01-12T09:56:31Z

src/uct/ib/efa/srd/srd_def.h

+typedef uint32_t uct_srd_ep_conn_sn_t;
+
+typedef struct uct_srd_neth {
+    uint32_t      packet_type;
+    uint8_t       fc;
+    uct_srd_psn_t psn;
+} UCS_S_PACKED uct_srd_neth_t;
+
+typedef struct uct_srd_recv_desc {
+    uct_ib_iface_recv_desc_t super;
+} uct_srd_recv_desc_t;


in order to reduce the PR size to the required LOC count, can we move the mpool and desc related code to next PR?

if we strictly add when used, it will be added all at once, so it thought to include the creation/init of qp in pre posting buffers, init + teardown path.

need to split it somehow to be <500 LOC in this PR

yosefe · 2025-01-12T09:56:55Z

src/uct/ib/efa/srd/srd_iface.c

+#endif
+
+#include "srd_iface.h"
+#include "../ib_efa.h"


pls use non-relative path

yosefe · 2025-01-12T09:59:01Z

src/uct/ib/efa/srd/srd_iface.c

+    qp_init_attr.cap.max_send_sge    = 1 + ucs_min(config->super.tx.min_sge,
+                                                   efa_attr->max_sq_sge - 1);
+    qp_init_attr.cap.max_recv_sge    = 1;
+    qp_init_attr.cap.max_inline_data = efa_attr->inline_buf_size;


whall we take it from user config for TX_MIN_INLINE?

yosefe · 2025-01-12T10:00:35Z

src/uct/ib/efa/srd/srd_iface.c

+
+    ucs_debug("iface=%p: created %s QP 0x%x on " UCT_IB_IFACE_FMT
+              " TX wr:%d sge:%d inl:%d resp:%d RX wr:%d sge:%d resp:%d",
+              iface, uct_ib_qp_type_str(UCT_IB_QPT_SRD), iface->qp->qp_num,


IMO using uct_ib_qp_type_str(UCT_IB_QPT_SRD) is redundant, it's always SRD here

yosefe · 2025-01-12T10:00:52Z

src/uct/ib/efa/srd/srd_iface.c

+    qp_attr.qkey       = UCT_IB_KEY;
+    ret                = ibv_modify_qp(iface->qp, &qp_attr,
+                                       IBV_QP_STATE | IBV_QP_PKEY_INDEX | IBV_QP_PORT |
+                                               IBV_QP_QKEY);


yosefe · 2025-01-12T10:03:18Z

src/uct/ib/efa/srd/srd_iface.c

+    {"FC_WND_SIZE", "512",
+     "The size of flow control window per endpoint. limits the number of AM\n"
+     "which can be sent w/o acknowledgment.",
+     ucs_offsetof(uct_srd_iface_config_t, fc.wnd_size), UCS_CONFIG_TYPE_UINT},
+
+    {"FC_SOFT_THRESH", "0.5",
+     "Threshold for sending soft request for FC credits to the peer. This "
+     "value\n"
+     "refers to the percentage of the FC_WND_SIZE value. (must be > "
+     "HARD_THRESH and < 1)",
+     ucs_offsetof(uct_srd_iface_config_t, fc.soft_thresh),
+     UCS_CONFIG_TYPE_DOUBLE},
+
+    {"FC_HARD_THRESH", "0.25",
+     "Threshold for sending hard request for FC credits to the peer. This "
+     "value\n"
+     "refers to the percentage of the FC_WND_SIZE value. (must be > 0 and < 1)",
+     ucs_offsetof(uct_srd_iface_config_t, fc.hard_thresh),
+     UCS_CONFIG_TYPE_DOUBLE},


FC can be moved to next PR to reduce LOC?

actually could likely be among the last PRs

yosefe · 2025-01-12T10:04:24Z

src/uct/ib/efa/srd/srd_iface.h

+    uct_ib_iface_config_t        super;
+    uct_ud_iface_common_config_t ud_common;
+
+    struct {
+        size_t   max_get_zcopy;
+    } tx;
+
+    struct {
+        double   soft_thresh;
+        double   hard_thresh;
+        unsigned wnd_size;
+    } fc;


align fields on column

brminich · 2025-01-13T16:05:13Z

src/uct/ib/efa/Makefile.am

+libuct_ib_efa_la_LDFLAGS  = $(EFA_LIB) $(IBVERBS_LDFLAGS) \
+                            -version-info $(SOVERSION)


minor: why would need to move?

align order with IB Makefile.am

brminich · 2025-01-13T16:14:03Z

src/uct/ib/efa/srd/srd_iface.c

+    qp_init_attr.cap.max_send_sge    = 1 + ucs_min(config->super.tx.min_sge,
+                                                   efa_attr->max_sq_sge - 1);


seems simpler

Suggested change

qp_init_attr.cap.max_send_sge = 1 + ucs_min(config->super.tx.min_sge,

efa_attr->max_sq_sge - 1);

qp_init_attr.cap.max_send_sge = ucs_min(config->super.tx.min_sge + 1,

efa_attr->max_sq_sge);

brminich · 2025-01-13T16:17:37Z

src/uct/ib/efa/srd/srd_iface.c

+              iface->super.config.max_inl_cqe[UCT_IB_DIR_RX]);
+
+    /* Transition the QP to RTS */
+    memset(&qp_attr, 0, sizeof(qp_attr));


why not initializing in place as other attrs in this func?

brminich · 2025-01-13T16:20:39Z

src/uct/ib/efa/srd/srd_iface.c

+        return status;
+    }
+
+    memset(&init_attr, 0, sizeof(init_attr));


maybe init in place?

brminich · 2025-01-13T16:21:50Z

src/uct/ib/efa/srd/srd_iface.c

+    ret                = ibv_modify_qp(iface->qp, &qp_attr,
+                                       IBV_QP_STATE | IBV_QP_PKEY_INDEX |
+                                       IBV_QP_PORT | IBV_QP_QKEY);
+    if (ret) {


Suggested change

if (ret) {

if (ret != 0) {

?
or use the same style for all places in this file

brminich · 2025-01-13T16:40:15Z

src/uct/ib/efa/srd/srd_iface.c

+    /* TAG */
+    iface_attr->cap.tag.rndv.max_zcopy = iface->config.max_get_zcopy;
+


no need to initialize, because SRD does not support tag offload

thanks removed

brminich · 2025-01-13T16:41:16Z

src/uct/ib/efa/srd/srd_iface.c

+    iface_attr->cap.am.max_short = uct_ib_iface_hdr_size(
+            iface->config.max_inline, sizeof(uct_srd_neth_t));


you can move this part to line 313 (when AM short support is checked)

brminich · 2025-01-13T16:43:06Z

src/uct/ib/efa/srd/srd_iface.c

+    iface_attr->cap.am.max_short = uct_ib_iface_hdr_size(
+            md->dev.max_inline_data, sizeof(uct_srd_neth_t));


you already initialized on line 300 using different value. What is the correct one?

brminich · 2025-01-13T16:47:16Z

src/uct/ib/efa/srd/srd_iface.h

+
+#include "srd_def.h"
+
+/** @file srd_iface.h */


why needed?

brminich · 2025-01-13T16:48:05Z

src/uct/ib/efa/srd/srd_iface.h

+#ifndef UCT_SRD_IFACE_H
+#define UCT_SRD_IFACE_H
+
+#include <uct/ib/base/ib_iface.h>


minor - it is already included in srd_def.h

removed, will rely implicitly on it

UCT/IB/EFA/SRD: Initial interface add

4807fdf

yosefe reviewed Jan 12, 2025

View reviewed changes

tvegas1 added 8 commits January 13, 2025 10:30

UCT/IB/EFA/SRD: Initial interface add

600ac96

UCT/IB/EFA/SRD: Initial interface add

b8f41d7

UCT/IB/EFA/SRD: Initial interface add

b6cb92b

UCT/IB/EFA/SRD: Initial interface add

de7c758

UCT/IB/EFA/SRD: Initial interface add

937c6d5

UCT/IB/EFA/SRD: Initial interface add

0ce426d

UCT/IB/EFA/SRD: Initial interface add

a37946b

UCT/IB/EFA/SRD: Initial interface add

1eb85fb

brminich reviewed Jan 13, 2025

View reviewed changes

tvegas1 added 4 commits January 13, 2025 18:33

UCT/IB/EFA/SRD: Initial interface add

4770a4c

UCT/IB/EFA/SRD: Initial interface add

d8b685d

UCT/IB/EFA/SRD: Initial interface add

a1c1741

UCT/IB/EFA/SRD: Initial interface add

98d8623

		libuct_ib_efa_la_LDFLAGS = $(EFA_LIB) $(IBVERBS_LDFLAGS) \
		-version-info $(SOVERSION)

		qp_init_attr.cap.max_send_sge = 1 + ucs_min(config->super.tx.min_sge,
		efa_attr->max_sq_sge - 1);

		/* TAG */
		iface_attr->cap.tag.rndv.max_zcopy = iface->config.max_get_zcopy;

		iface_attr->cap.am.max_short = uct_ib_iface_hdr_size(
		iface->config.max_inline, sizeof(uct_srd_neth_t));

		iface_attr->cap.am.max_short = uct_ib_iface_hdr_size(
		md->dev.max_inline_data, sizeof(uct_srd_neth_t));

UCT/IB/EFA/SRD: Initial interface add #10412

Are you sure you want to change the base?

UCT/IB/EFA/SRD: Initial interface add #10412

Conversation

tvegas1 commented Jan 10, 2025

What?

Test

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment