UCT/GGA: basic iface and ep infrastructure #9845

evgeny-leksikov · 2024-04-26T07:58:18Z

What

Implement and test basic GGA iface and ep infrastructure

Why ?

Continue #9811

yosefe · 2024-05-02T11:23:41Z

@iyastreb can you pls review?

iyastreb · 2024-05-02T11:52:41Z

src/uct/ib/mlx5/dv/ib_mlx5_ifc.h

@@ -72,6 +72,7 @@ enum {
    UCT_IB_MLX5_CMD_OP_2ERR_QP                 = 0x507,
    UCT_IB_MLX5_CMD_OP_2RST_QP                 = 0x50a,
    UCT_IB_MLX5_CMD_OP_QUERY_QP                = 0x50b,
+    UCT_IB_MLX5_CMD_OP_INIT2INIT_QP            = 0x50e,


Are these changes covered by unit/integration tests?

yes, this is part of GGA ep_create flow

iyastreb · 2024-05-02T11:53:25Z

src/uct/ib/rc/accel/gga_mlx5.c

@@ -29,6 +29,46 @@ typedef struct uct_gga_mlx5_iface_config {
    uct_rc_mlx5_iface_common_config_t rc_mlx5_common;
 } uct_gga_mlx5_iface_config_t;

+typedef struct uct_gga_mlx5_dma_opaque {


minor: here and below, struct name is redundant, typedef should be sufficient

this is "UCX code style"

well, in many places in UCX I see only typedef, but up to you

we don't need names for internal structs (unless it's a forward declaration)

src/uct/ib/rc/accel/rc_mlx5_common.c

src/uct/ib/rc/accel/rc_mlx5_ep.c

src/uct/ib/rc/accel/gga_mlx5.c

iyastreb · 2024-05-02T12:34:09Z

src/uct/ib/rc/accel/gga_mlx5.c

+        return UCS_ERR_NO_MEMORY;
+    }
+
+    ep->dma_opaque.mr = ibv_reg_mr(md->super.pd, ep->dma_opaque.buf,


should we reuse here uct_ib_reg_mr?
uct_ib_reg_mr handles EAGAIN error by repeating the call, should it be the similar in this case?

should we reuse here uct_ib_reg_mr?

IMO it's overcomplicated in this case

uct_ib_reg_mr handles EAGAIN error by repeating the call, should it be the similar in this case?

no, that's only for ODP case

src/uct/ib/rc/accel/gga_mlx5.c

iyastreb · 2024-05-02T12:38:48Z

src/uct/ib/rc/accel/gga_mlx5.c

+    iface_attr->device_addr_len += ucs_offsetof(uct_gga_mlx5_dev_addr_t,
+                                                ib_addr);
+    iface_attr->ep_addr_len      = sizeof(uct_gga_mlx5_ep_address_t);
+    iface_attr->cap.flags = /*


nope, placeholder for a next patch

iyastreb · 2024-05-02T12:48:24Z

src/uct/ib/rc/accel/rc_mlx5_ep.c

-    uct_rc_ep_cleanup_qp(&self->super.super, &cleanup_ctx->super,
-                         self->super.tx.wq.super.qp_num,
-                         outstanding - wqe_count);
+    cleanup_ctx = ucs_malloc(sizeof(*cleanup_ctx), "mlx5_qp_cleanup_ctx");


It's unusual to allocate memory during cleanup. Where this memory is released?
Maybe allocation on stack would suffice?

this is async cleanup context, see IBV_EVENT_QP_LAST_WQE_REACHED

iyastreb · 2024-05-02T12:52:46Z

src/uct/ib/rc/accel/gga_mlx5.c

+    uct_gga_mlx5_iface_qp_cleanup_ctx_t *gga_ctx =
+            ucs_derived_of(ctx, uct_gga_mlx5_iface_qp_cleanup_ctx_t);
+
+    ibv_dereg_mr(gga_ctx->dma_opaque.mr);


maybe uct_ib_dereg_mr?

should be aligned with https://github.com/openucx/ucx/pull/9845/files#r1587560718

src/uct/ib/rc/accel/gga_mlx5.c

yosefe · 2024-05-05T11:54:56Z

src/uct/ib/rc/accel/gga_mlx5.c

@@ -29,6 +29,46 @@ typedef struct uct_gga_mlx5_iface_config {
    uct_rc_mlx5_iface_common_config_t rc_mlx5_common;
 } uct_gga_mlx5_iface_config_t;

+typedef struct uct_gga_mlx5_dma_opaque {


we don't need names for internal structs (unless it's a forward declaration)

src/uct/ib/rc/accel/gga_mlx5.c

src/uct/ib/rc/accel/rc_mlx5_iface.c

src/uct/ib/base/ib_iface.c

yosefe · 2024-05-15T09:42:58Z

src/uct/ib/mlx5/dv/ib_mlx5_dv.c

-    UCT_IB_MLX5DV_SET(qpc, qpc, rq_type, !!attr->super.srq_num);
+    UCT_IB_MLX5DV_SET(qpc, qpc, rq_type,
+                      attr->super.srq_num ? 1 /* SRQ */ :
+                      max_rx              ? 0 /* RQ */ :


where do we have a case of "RQ?

yosefe · 2024-05-19T13:41:47Z

src/uct/ib/rc/accel/gga_mlx5.c


    uct_ib_pack_uint24(gga_addr->qp_num, ep->super.tx.wq.super.qp_num);
    if (uct_rc_iface_flush_rkey_enabled(&iface->super)) {
        gga_addr->flags      = UCT_GGA_MLX5_EP_ADDRESS_FLAG_FLUSH_RKEY;
-        gga_addr->flush_rkey = (md->flush_rkey >> 16);
+        gga_addr->flush_rkey = md->flush_rkey;


flush_rkey in the address could be 24 bit since its lower 8 bits are always 0
see

ucx/src/uct/ib/mlx5/dv/ib_mlx5dv_md.c

Line 1739 in 407ab27

ucs_assert((md->super.flush_rkey & 0xff) == 0);

yosefe · 2024-05-19T13:44:11Z

src/uct/ib/rc/accel/gga_mlx5.c

+    iface = ucs_derived_of(tl_iface, uct_base_iface_t);
+    md    = ucs_derived_of(iface->md, uct_ib_mlx5_md_t);


init inline

yosefe · 2024-05-19T13:55:50Z

src/uct/ib/base/ib_iface.h

@@ -718,6 +718,10 @@ uct_ib_iface_roce_dscp(uct_ib_iface_t *iface)
    return iface->config.traffic_class >> 2;
 }

+int uct_ib_iface_dev_addr_is_reachable(uct_ib_iface_t *iface,


pls move this along with other function prototypes in this file

yosefe · 2024-05-19T14:10:18Z

src/uct/ib/rc/accel/gga_mlx5.c

+}
+
+static ucs_status_t
+uct_gga_mlx5_iface_init_rx(uct_rc_iface_t *rc_iface,


do we really need this wrapper?

yosefe · 2024-05-19T14:17:45Z

src/uct/ib/rc/accel/gga_mlx5.c

+    return UCS_OK;
+}
+
+static void uct_gga_mlx5_iface_qp_cleanup(uct_rc_iface_qp_cleanup_ctx_t *ctx)


I don't think we need cleanup mechanism for GGA. Cleanup is about waiting for LAST_WQE_REACHED event, which is not relevant since we don't have SEND/RECV operations. I'd suggest to not add it in this PR, unless we see some failure. Can test error handling and add it later if needed.

I avoid LAST_WQE_REACHED flow adding async parameter which is false for GGA, this approach adds some overhead on ep_destroy but re-uses dtors of base classed without significant change, otherwise need to refactor them deeper

yosefe · 2024-05-19T14:47:57Z

src/uct/ib/rc/accel/rc_mlx5_common.c

@@ -210,7 +210,8 @@ void uct_rc_mlx5_iface_common_prepost_recvs(uct_rc_mlx5_iface_common_t *iface)
 {
    /* prepost recvs only if quota available (recvs were not preposted
     * before) */
-    if (iface->super.rx.srq.quota == 0) {
+    if ((iface->super.rx.srq.quota == 0) ||
+        (iface->rx.srq.type == UCT_IB_MLX5_OBJ_TYPE_NULL)) {


why the check iface->super.rx.srq.quota == 0 is not enough? can we set quota to 0 for gga?

yosefe · 2024-05-19T14:48:22Z

src/uct/ib/rc/accel/rc_mlx5_common.c

@@ -526,6 +527,7 @@ void uct_rc_mlx5_iface_fill_attr(uct_rc_mlx5_iface_common_t *iface,
                               srq->verbs.srq);
        break;
    case UCT_IB_MLX5_OBJ_TYPE_DEVX:
+    case UCT_IB_MLX5_OBJ_TYPE_NULL:


why UCT_IB_MLX5_OBJ_TYPE_NULL is not an empty operation?

good question, I don't know why SRQ obj type defines QP attrs for TX too :)

yosefe · 2024-05-19T14:50:05Z

src/uct/ib/rc/base/rc_iface.c

    ucs_mpool_params_reset(&mp_params);
-    mp_params.elem_size       = init_attr->fc_req_size;
+    mp_params.elem_size       = ucs_max(init_attr->fc_req_size,


why needed to change?

because fc_req_size == 0 for GGA

yosefe · 2024-05-27T14:06:43Z

test/gtest/uct/ib/test_ib.cc

@@ -237,6 +237,10 @@ test_uct_ib_with_specific_port::test_uct_ib_with_specific_port() {
 }

 void test_uct_ib_with_specific_port::init() {
+    if (has_transport("gga_mlx5")) {


remove the skip

evgeny-leksikov · 2024-05-29T16:21:04Z

/azp run

azure-pipelines · 2024-05-29T16:21:26Z

Azure Pipelines successfully started running 4 pipeline(s).

evgeny-leksikov added 5 commits April 26, 2024 10:44

UCT/GGA: enable testing for md, iface, ep_connect

9f98e9f

GTEST/UCT/GGA: skip irrelevant tests

6c33f8f

UCT/GGA: add iface_event_fd_get and iface_arm required by UCP

68fffab

UCT/GGA: set ep_addr_len by iface_query

5e6db01

UCT/GGA: fix memleak on rkey_release

e9bfe80

iyastreb reviewed May 2, 2024

View reviewed changes

src/uct/ib/rc/accel/gga_mlx5.c Outdated Show resolved Hide resolved

src/uct/ib/rc/accel/gga_mlx5.c Outdated Show resolved Hide resolved

src/uct/ib/rc/accel/gga_mlx5.c Outdated Show resolved Hide resolved

UCT/GGA: enable testing for md, iface, ep_connect, CR1

abcbcca

iyastreb previously approved these changes May 3, 2024

View reviewed changes

evgeny-leksikov requested a review from yosefe May 3, 2024 09:00

yosefe reviewed May 5, 2024

View reviewed changes

UCT/GGA: enable testing for md, iface, ep_connect, CR2

b744e8e

evgeny-leksikov dismissed iyastreb’s stale review via b744e8e May 8, 2024 19:29

evgeny-leksikov added 2 commits May 10, 2024 16:47

UCT/GGA: enable testing for md, iface, ep_connect, CR3

14191ad

UCT/GGA: enable testing for md, iface, ep_connect, CR4

f6804bb

evgeny-leksikov requested a review from yosefe May 12, 2024 20:02

yosefe reviewed May 19, 2024

View reviewed changes

evgeny-leksikov added 3 commits May 21, 2024 23:20

UCT/GGA: enable testing for md, iface, ep_connect, CR5

27f1076

UCT/GGA: enable testing for md, iface, ep_connect, CR6

be7d2a9

UCT/IB: revert unrelated changes

796613b

yosefe reviewed May 27, 2024

View reviewed changes

evgeny-leksikov added 2 commits May 28, 2024 17:58

UCT/GGA: enable testing for md, iface, ep_connect, CR7

4ed525c

UCT/GGA: fix memory leak

79e9393

evgeny-leksikov requested a review from yosefe June 3, 2024 06:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCT/GGA: basic iface and ep infrastructure #9845

UCT/GGA: basic iface and ep infrastructure #9845

evgeny-leksikov commented Apr 26, 2024

yosefe commented May 2, 2024

iyastreb May 2, 2024

evgeny-leksikov May 2, 2024

iyastreb May 2, 2024

evgeny-leksikov May 2, 2024

iyastreb May 3, 2024

yosefe May 5, 2024

iyastreb May 2, 2024

evgeny-leksikov May 2, 2024

iyastreb May 2, 2024

evgeny-leksikov May 2, 2024

iyastreb May 2, 2024

evgeny-leksikov May 2, 2024

iyastreb May 2, 2024

evgeny-leksikov May 2, 2024

yosefe May 5, 2024

yosefe May 15, 2024

yosefe May 19, 2024

yosefe May 19, 2024

yosefe May 19, 2024

yosefe May 19, 2024

yosefe May 19, 2024

evgeny-leksikov May 22, 2024

yosefe May 19, 2024

yosefe May 19, 2024

evgeny-leksikov May 22, 2024

yosefe May 19, 2024

evgeny-leksikov May 22, 2024

yosefe May 27, 2024

evgeny-leksikov commented May 29, 2024

azure-pipelines bot commented May 29, 2024

		iface = ucs_derived_of(tl_iface, uct_base_iface_t);
		md = ucs_derived_of(iface->md, uct_ib_mlx5_md_t);

UCT/GGA: basic iface and ep infrastructure #9845

Are you sure you want to change the base?

UCT/GGA: basic iface and ep infrastructure #9845

Conversation

evgeny-leksikov commented Apr 26, 2024

What

Why ?

yosefe commented May 2, 2024

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

evgeny-leksikov commented May 29, 2024

azure-pipelines bot commented May 29, 2024