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
...

src/uct/ib/base/ib_iface.h

src/uct/ib/efa/Makefile.am

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

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

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

src/uct/ib/efa/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

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

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

yosefe · 2025-01-16T08:35:16Z

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

+    memset(&self->tx.wr_inl, 0, sizeof(self->tx.wr_inl));
+    self->tx.wr_inl.opcode            = IBV_WR_SEND;
+    self->tx.wr_inl.wr_id             = 0xBEEBBEEB;
+    self->tx.wr_inl.wr.ud.remote_qkey = UCT_IB_KEY;
+    self->tx.wr_inl.imm_data          = 0;
+    self->tx.wr_inl.next              = 0;
+    self->tx.wr_inl.sg_list           = self->tx.sge;
+
+    memset(&self->tx.wr_desc, 0, sizeof(self->tx.wr_desc));
+    self->tx.wr_desc.opcode            = IBV_WR_SEND;
+    self->tx.wr_desc.wr_id             = 0xFAAFFAAF;
+    self->tx.wr_desc.wr.ud.remote_qkey = UCT_IB_KEY;
+    self->tx.wr_desc.imm_data          = 0;
+    self->tx.wr_desc.next              = 0;
+    self->tx.wr_desc.sg_list           = self->tx.sge;
+    self->tx.wr_desc.num_sge           = 1;


we can add it to ud_iface_common.h/c

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

yosefe · 2025-01-18T18:20:43Z

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

@@ -12,10 +12,10 @@
 #include <ucs/datastruct/ptr_array.h>
 #include <ucs/datastruct/conn_match.h>

+#include "srd_def.h"


minor: include local files before the other includes

brminich · 2025-01-20T11:45:11Z

src/uct/ib/ud/verbs/ud_verbs.c

@@ -790,6 +790,25 @@ ucs_status_t uct_ud_verbs_qp_max_send_sge(uct_ud_verbs_iface_t *iface,
    return UCS_OK;
 }

+void uct_ud_iface_send_wr_init(struct ibv_send_wr *wr, struct ibv_sge *sge,


is it preparation for the next PR?

no, it is already being used where applicable

why not to make it static then?

it is being used in INIT iface function in srd_iface.c

double checking, you are right, there is naming issue.

fixed, thanks!

brminich · 2025-01-20T18:47:08Z

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

+    iface_attr->overhead   = 105e-9;
+
+    /* AM */
+


minor: extra line

brminich · 2025-01-20T18:48:17Z

src/uct/ib/ud/verbs/ud_verbs.c

@@ -790,6 +790,25 @@ ucs_status_t uct_ud_verbs_qp_max_send_sge(uct_ud_verbs_iface_t *iface,
    return UCS_OK;
 }

+void uct_ud_iface_send_wr_init(struct ibv_send_wr *wr, struct ibv_sge *sge,


why not to make it static then?

yosefe reviewed Jan 12, 2025

View reviewed changes

brminich reviewed Jan 13, 2025

View reviewed changes

yosefe reviewed Jan 16, 2025

View reviewed changes

yosefe reviewed Jan 18, 2025

View reviewed changes

brminich reviewed Jan 20, 2025

View reviewed changes

yosefe previously approved these changes Jan 20, 2025

View reviewed changes

brminich reviewed Jan 20, 2025

View reviewed changes

tvegas1 dismissed yosefe’s stale review via 82d5c51 January 21, 2025 08:27

brminich approved these changes Jan 21, 2025

View reviewed changes

yosefe approved these changes Jan 21, 2025

View reviewed changes

UCT/IB/EFA/SRD: Initial interface add

8e199aa

tvegas1 force-pushed the srd_iface branch from 82d5c51 to 8e199aa Compare January 21, 2025 17:57

brminich merged commit 969ed1d into openucx:master Jan 23, 2025
147 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

tvegas1 commented Jan 10, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

brminich Jan 13, 2025

tvegas1 Jan 13, 2025

yosefe Jan 16, 2025

yosefe Jan 18, 2025

tvegas1 Jan 20, 2025

brminich Jan 20, 2025

tvegas1 Jan 20, 2025

brminich Jan 20, 2025

tvegas1 Jan 21, 2025

tvegas1 Jan 21, 2025

tvegas1 Jan 21, 2025

brminich Jan 20, 2025

tvegas1 Jan 21, 2025

brminich Jan 20, 2025

		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

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