CONTRIB/IBMOCK: Inital add with EFA support #10069

tvegas1 · 2024-08-19T09:56:44Z

What

Add IB mocking (EFA support) capability.

Why ?

Need to run gtest without the need for EFA interface.

How ?

Implement loopback libibverbs/libefa. Keep build/code independent from UCX libraries.

Perftest/Info

vdi$ export LD_LIBRARY_PATH=$(pwd)/../efa_mock/build:$LD_LIBRARY_PATH
vdi$ UCX_TLS=srd ucx_perftest -t tag_bw -l
vdi$ UCX_TLS=ud ucx_perftest -t tag_bw -l
vdi$ ucx_info -d

Gtest:

vdi$ export LD_LIBRARY_PATH=$(pwd)/../hpc-poc/ibmock/build
vdi$  ./test/gtest/gtest --gtest_filter=*ud*
...
[----------] Global test environment tear-down
[==========] 2300 tests from 119 test suites ran. (642250 ms total)
[  PASSED  ] 2300 tests.

vdi$ valgrind --leak-check=full  ./test/gtest/gtest --gtest_filter=\
*ud*-*perf.enveloppe*

contrib/ibmock/config.h

contrib/ibmock/efa.c

contrib/ibmock/lib.h

contrib/ibmock/verbs.c

contrib/ibmock/lib.h

contrib/ibmock/verbs.c

brminich · 2025-01-09T09:32:23Z

buildlib/pr/efa.yml

+          AZP_AGENT_ID: $(AZP_AGENT_ID)
+          BUILD_NUMBER: "$(Build.BuildId)-$(Build.BuildNumber)"
+          JOB_URL: "$(System.TeamFoundationCollectionUri)$(System.TeamProject)/_build/results?buildId=$(Build.BuildId)"


why do you need these? looks like they are not used by the efa test script

brminich · 2025-01-09T09:34:25Z

contrib/test_efa.sh

+    LD_LIBRARY_PATH=$(pwd)/contrib/ibmock/build:$LD_LIBRARY_PATH
+    CLOUD_TYPE=aws
+    export LD_LIBRARY_PATH
+    export CLOUD_TYPE


minor: can init in place

brminich · 2025-01-09T09:36:57Z

contrib/ibmock/verbs.h

@@ -0,0 +1,21 @@
+/**
+ * Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.


Suggested change

* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

* Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.

brminich · 2025-01-09T09:37:20Z

contrib/ibmock/verbs.c

@@ -0,0 +1,1064 @@
+/**
+ * Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.


Suggested change

* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

* Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.

brminich · 2025-01-09T09:39:20Z

contrib/ibmock/config.h

+#include <infiniband/efadv.h>
+#include <infiniband/verbs.h>
+
+#define NUM_DEVS 2


maybe define in verbs.c? anyways it needs to be queried by ibv_get_device_list

brminich · 2025-01-09T09:40:42Z

contrib/ibmock/config.c

@@ -0,0 +1,109 @@
+/**
+ * Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.


Suggested change

* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

* Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.

brminich · 2025-01-09T09:42:32Z

contrib/ibmock/config.h

@@ -0,0 +1,22 @@
+/**
+ * Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.


Suggested change

* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

* Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.

brminich · 2025-01-09T09:42:45Z

contrib/ibmock/efa.c

@@ -0,0 +1,102 @@
+/**
+ * Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.


Suggested change

* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

* Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.

brminich · 2025-01-09T09:46:47Z

contrib/ibmock/verbs.c

+        }
+    }
+
+    printf("ibmock: failed to destroy QP#%d\n", qp->qp_num);


maybe print to stderr instead?

fixed for all

brminich · 2025-01-09T09:51:48Z

contrib/ibmock/verbs.c

+    if (node_type < IBV_NODE_CA || node_type > IBV_NODE_UNSPECIFIED)
+        return "unknown";


in some places you use different style - brackets around each statement, and also brackets for every block even if it is one line. Better to use the same style thru the whole lib

tvegas1 · 2025-01-09T14:05:31Z

contrib/ibmock/README.md

+```
+cd ucx
+./autogen.sh
+./contrib/configure-devel --with-verbs=$(pwd)/../rdma-core/build/include --with-efa


actually, remove the include suffix

neeed to apply the comment?

contrib/test_efa.sh

contrib/ibmock/lib.h

contrib/ibmock/verbs.c

brminich · 2025-01-27T08:23:24Z

@tvegas1, can you pls squash?

tvegas1 · 2025-01-28T10:20:20Z

CI failure fixed by #10457

yosefe · 2025-01-30T11:41:17Z

/azp run UCX PR

azure-pipelines · 2025-01-30T11:41:31Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2025-02-05T09:17:46Z

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

@@ -357,8 +364,7 @@ static uct_iface_ops_t uct_srd_iface_tl_ops = {
        ucs_empty_function_return_unsupported,
    .ep_pending_purge         = (uct_ep_pending_purge_func_t)
        ucs_empty_function_return_unsupported,
-    .iface_flush              = (uct_iface_flush_func_t)
-        ucs_empty_function_return_unsupported,
+    .iface_flush              = uct_srd_iface_flush,


can we use uct_base_iface_flush?

yosefe · 2025-02-05T09:19:01Z

test/gtest/ucp/test_ucp_perf.cc

@@ -327,6 +327,10 @@ UCS_TEST_SKIP_COND_P(test_ucp_perf, envelope, has_transport("self"))
    size_t max_iter = std::numeric_limits<size_t>::max();
    test_spec test  = tests[get_variant_value(VARIANT_TEST_TYPE)];

+    if (ucs::is_aws() && (test.wait_mode == UCX_PERF_WAIT_MODE_SLEEP)) {


what happens if we don'k skip aws here?
i would expect UCP layer or libperf be able to handle the missing support

there is the error below, actually it is seen when only ud_v is available, updated the condition in the meantime. I am understanding that it is related to missing event support, do you see an easy way to detect it before inside libperf?

[ RUN ] ud/test_ucp_perf.envelope/10 <ud_v/tag_bw_b> [1738752485.488782] [rock28:3736565:p-0] select.c:645 UCX ERROR no active messages transport to rock28:3736565: ud_verbs/rdmap0:1 - no send completion event, ud_verbs/rdmap1:1 - no send completion event [1738752485.488791] [rock28:3736565:p-1] select.c:645 UCX ERROR no active messages transport to rock28:3736565: ud_verbs/rdmap0:1 - no send completion event, ud_verbs/rdmap1:1 - no send completion event contrib/../test/gtest/common/test.cc:366: Failure Failed Got 2 errors and 0 warnings during the test [ INFO ] < contrib/../src/ucp/wireup/select.c:645 no active messages transport to rock28:3736565: ud_verbs/rdmap0:1 - no send completion event, ud_verbs/rdmap1:1 - no send completion event > [ INFO ] < contrib/../src/ucp/wireup/select.c:645 no active messages transport to rock28:3736565: ud_verbs/rdmap0:1 - no send completion event, ud_verbs/rdmap1:1 - no send completion event > [ FAILED ] ud/test_ucp_perf.envelope/10, where GetParam() = ud_v/tag_bw_b (6 ms)

AFAIR the issue was solicited even support, maybe we can support wakeup/sleep in ud_v trasnport with arming cq for any event (not just solicited), it will not be optimal but at least will work
we can add here "// TODO support wakeup in UD transport without requiring IBV_SEND_SOLICITED"

added comment

yosefe · 2025-02-05T11:57:06Z

test/gtest/ucp/test_ucp_perf.cc

+    ss << GetParam().transports;
+
+    if (ucs::is_aws() && (test.wait_mode == UCX_PERF_WAIT_MODE_SLEEP) &&
+        ss.str() == "ud_v") {


add ( )

maybe use has_transport() instead of std::stringstream ?

iyastreb reviewed Aug 19, 2024

View reviewed changes

contrib/ibmock/lib.h Outdated Show resolved Hide resolved

contrib/ibmock/lib.h Outdated Show resolved Hide resolved

contrib/ibmock/verbs.c Outdated Show resolved Hide resolved

iyastreb reviewed Aug 19, 2024

View reviewed changes

tvegas1 force-pushed the ibmock branch from 269309f to d0c2718 Compare January 9, 2025 05:33

brminich reviewed Jan 9, 2025

View reviewed changes

tvegas1 commented Jan 9, 2025

View reviewed changes

contrib/test_efa.sh Show resolved Hide resolved

tvegas1 force-pushed the ibmock branch from 90a1ee0 to 45f4a08 Compare January 9, 2025 21:29

tvegas1 mentioned this pull request Jan 10, 2025

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

Merged

iyastreb reviewed Jan 21, 2025

View reviewed changes

contrib/ibmock/lib.h Outdated Show resolved Hide resolved

contrib/ibmock/verbs.c Outdated Show resolved Hide resolved

contrib/ibmock/verbs.c Show resolved Hide resolved

contrib/ibmock/verbs.c Outdated Show resolved Hide resolved

contrib/ibmock/verbs.c Outdated Show resolved Hide resolved

iyastreb previously approved these changes Jan 21, 2025

View reviewed changes

brminich previously approved these changes Jan 27, 2025

View reviewed changes

tvegas1 force-pushed the ibmock branch from bc69b7d to d504e34 Compare January 27, 2025 10:15

tvegas1 dismissed stale reviews from iyastreb and brminich via b78567a January 27, 2025 14:44

tvegas1 force-pushed the ibmock branch from d504e34 to b78567a Compare January 27, 2025 14:44

tvegas1 force-pushed the ibmock branch 2 times, most recently from cb7e3da to 610ec40 Compare February 3, 2025 14:37

yosefe reviewed Feb 5, 2025

View reviewed changes

yosefe approved these changes Feb 5, 2025

View reviewed changes

CONTRIB/MOCK: Address review comments

362f501

tvegas1 force-pushed the ibmock branch from bcfdc42 to 362f501 Compare February 6, 2025 08:16

yosefe enabled auto-merge February 6, 2025 10:44

yosefe merged commit 077fc07 into openucx:master Feb 6, 2025
148 checks passed

		@@ -0,0 +1,21 @@
		/**
		* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

	* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.
	* Copyright (c) 2025, NVIDIA CORPORATION. All rights reserved.

		@@ -0,0 +1,1064 @@
		/**
		* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

		@@ -0,0 +1,109 @@
		/**
		* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

		@@ -0,0 +1,22 @@
		/**
		* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

		@@ -0,0 +1,102 @@
		/**
		* Copyright (c) 2024, NVIDIA CORPORATION. All rights reserved.

		if (node_type < IBV_NODE_CA \|\| node_type > IBV_NODE_UNSPECIFIED)
		return "unknown";

CONTRIB/IBMOCK: Inital add with EFA support #10069

CONTRIB/IBMOCK: Inital add with EFA support #10069

Conversation

tvegas1 commented Aug 19, 2024 • edited Loading

What

Why ?

How ?

Perftest/Info

Gtest:

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

brminich commented Jan 27, 2025

tvegas1 commented Jan 28, 2025

yosefe commented Jan 30, 2025

azure-pipelines bot commented Jan 30, 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

tvegas1 commented Aug 19, 2024 •

edited

Loading