UCP/WIREUP: Introduce connection-based lanes intersection #9881

shasson5 · 2024-05-16T15:50:31Z

What

A new method for lanes intersection is introduced here, using ep_is_connected UCT API.

Why ?

To remove dependency on dst_rsc_index when performing lane intersection.
dst_rsc_index is only supported in CM flow, so in order to support EP reconfiguration in non-CM flow (next PRs), we need to replace it with connection-based method.
Tests are covered by sockaddr gtests, as it is the only affected module currently.
This PR is the first part of the EP reconfiguration feature.

brminich · 2024-05-17T11:40:31Z

src/ucp/wireup/wireup_ep.c

+uct_ep_h ucp_wireup_ep_get_tl_ep(uct_ep_h uct_ep)
+{
+    ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(uct_ep);
+    return (wireup_ep == NULL) ? uct_ep : wireup_ep->super.uct_ep;
+}


it's used in wireup.c only, maybe define it there as static func?

brminich · 2024-05-17T11:47:02Z

src/ucp/core/ucp_ep.c

+        return UCP_NULL_LANE;
+    }
+
+    /* Connect to matching lane in case it is not connected yet */


maybe

Suggested change

/* Connect to matching lane in case it is not connected yet */

/* Return matching lane in case it is not connected yet */

brminich · 2024-05-17T11:49:20Z

src/ucp/wireup/wireup.c

+ucp_wireup_fill_is_connected_params(uct_ep_is_connected_params_t *params,
+                                    const ucp_address_entry_t *addr_entry)


minor:
i'd make the out parameter to be last (for consistency with other places)

src/ucp/wireup/wireup.c

brminich · 2024-05-17T12:02:36Z

src/ucp/wireup/wireup.c

-        } else {
-            ucs_assert(addr_index != UINT_MAX);
-            ae            = &remote_address->address_list[addr_index];
-            dst_rsc_index = ae->iface_attr.dst_rsc_index;


seems we do not need ucp_address_iface_attr->dst_rsc_index field anymore, can remove it

ok but still need to keep placeholder for UCP_ADDRESS_PACK_FLAG_TL_RSC_IDX in packed address (for wire compat).

gleon99 · 2024-05-19T08:17:34Z

@shasson5 as we discussed, please add a bit more details on the motivation in the PR description.

src/ucp/core/ucp_ep.c

src/ucp/wireup/wireup.c

yosefe · 2024-05-20T11:18:39Z

test/gtest/ucp/test_ucp_wireup.cc

+
+        for (rsc_index = 0; rsc_index < context->num_tls; ++rsc_index) {
+            if ((context->tl_rscs[rsc_index].md_index == ae->md_index) &&
+                (context->tl_rscs[rsc_index].tl_name_csum ==


is this enough? shouldn't we check is_connected?

this is a pack/unpack test, not related to EP reconf

IMO md and tl_name check is not enough. should also check is_same_device

src/ucp/core/ucp_ep.c

src/ucp/wireup/wireup.c

src/ucp/core/ucp_ep.c

src/ucp/wireup/wireup.c

yosefe · 2024-05-21T08:51:45Z

test/gtest/ucp/test_ucp_wireup.cc

+
+        for (rsc_index = 0; rsc_index < context->num_tls; ++rsc_index) {
+            if ((context->tl_rscs[rsc_index].md_index == ae->md_index) &&
+                (context->tl_rscs[rsc_index].tl_name_csum ==


IMO md and tl_name check is not enough. should also check is_same_device

test/gtest/ucp/test_ucp_wireup.cc

yosefe · 2024-05-22T08:37:50Z

test/gtest/ucp/test_ucp_wireup.cc

@@ -1769,14 +1769,14 @@ class test_ucp_address_v2 : public test_ucp_wireup {
        ucp_rsc_index_t rsc_index;

        for (rsc_index = 0; rsc_index < context->num_tls; ++rsc_index) {
-            if ((context->tl_rscs[rsc_index].md_index == ae->md_index) &&
+            if ((context->tl_rscs[rsc_index].dev_index == ae->dev_index) &&


dev_iondex in in a different space in context and unpacked address, so cannot compare them
need to use uct_iface_is_reachable_v2 with SAME_DEVICE scope

yosefe · 2024-05-22T12:32:14Z

src/ucp/wireup/wireup.c

+
+    if (addr_entry->num_ep_addrs == 0) {
+        /* Verify this lane is connecting to iface */
+        ucs_assertv(!ucp_ep_is_lane_p2p(ep, lane), "ep=%p lane=%u", ep, lane);


uct_rc_ep_is_connected should check UCT_RC_EP_FLAG_CONNECTED and return 0 if the flag is not present

here,

if (!ucp_ep_is_lane_p2p(ep, lane)) { /* Check if the lane is connected to the remote iface */ ucs_assertv(addr_entry->num_ep_addrs == 0, "num_ep_addres=%d", addr_entry->num_ep_addrs); return uct_ep_is_connected(ucp_wireup_get_tl_ep(uct_ep), &params); }

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

yosefe · 2024-05-22T16:45:01Z

test/gtest/ucp/test_ucp_wireup.cc

+            /* Compare resources by device and transport */
+            if ((context->tl_rscs[rsc_index].tl_name_csum ==
+                 ae->tl_name_csum) &&
+                uct_iface_is_reachable_v2(wiface->iface, &params)) {


EXPECT_EQ(ae->md_index, context->tl_rscs[rsc_index].md_index)

shasson5 · 2024-05-26T08:13:14Z

/azp run

azure-pipelines · 2024-05-26T08:13:33Z

Azure Pipelines successfully started running 3 pipeline(s).

shasson5 · 2024-05-27T09:57:00Z

squashed

shasson5 added the WIP-DNM Work in progress / Do not review label May 16, 2024

brminich reviewed May 17, 2024

View reviewed changes

shasson5 requested review from brminich and gleon99 May 20, 2024 10:27

shasson5 removed the WIP-DNM Work in progress / Do not review label May 20, 2024

shasson5 requested a review from yosefe May 20, 2024 10:27

yosefe reviewed May 20, 2024

View reviewed changes

shasson5 requested a review from yosefe May 20, 2024 12:59

brminich previously approved these changes May 21, 2024

View reviewed changes

yosefe reviewed May 21, 2024

View reviewed changes

shasson5 dismissed brminich’s stale review via 66e3a70 May 21, 2024 13:58

shasson5 requested a review from yosefe May 21, 2024 13:58

yosefe reviewed May 22, 2024

View reviewed changes

shasson5 requested a review from yosefe May 22, 2024 15:44

yosefe reviewed May 22, 2024

View reviewed changes

shasson5 requested a review from yosefe May 23, 2024 10:20

yosefe approved these changes May 23, 2024

View reviewed changes

UCP/WIREUP: Support is_connect-based lane intersection

f06df3d

shasson5 force-pushed the ep_is_connect branch from 60eda72 to f06df3d Compare May 27, 2024 09:56

yosefe enabled auto-merge May 27, 2024 10:09

yosefe merged commit a63c53f into openucx:master May 28, 2024
140 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCP/WIREUP: Introduce connection-based lanes intersection #9881

UCP/WIREUP: Introduce connection-based lanes intersection #9881

shasson5 commented May 16, 2024 •

edited

brminich May 17, 2024

brminich May 17, 2024

brminich May 17, 2024

brminich May 17, 2024

shasson5 May 19, 2024 •

edited

gleon99 commented May 19, 2024

yosefe May 20, 2024

shasson5 May 20, 2024

yosefe May 21, 2024

yosefe May 21, 2024

yosefe May 22, 2024

yosefe May 22, 2024

yosefe May 22, 2024

shasson5 commented May 26, 2024

azure-pipelines bot commented May 26, 2024

shasson5 commented May 27, 2024

	/* Connect to matching lane in case it is not connected yet */
	/* Return matching lane in case it is not connected yet */

		ucp_wireup_fill_is_connected_params(uct_ep_is_connected_params_t *params,
		const ucp_address_entry_t *addr_entry)

UCP/WIREUP: Introduce connection-based lanes intersection #9881

UCP/WIREUP: Introduce connection-based lanes intersection #9881

Conversation

shasson5 commented May 16, 2024 • edited

What

Why ?

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

shasson5 May 19, 2024 • edited

Choose a reason for hiding this comment

gleon99 commented May 19, 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

shasson5 commented May 26, 2024

azure-pipelines bot commented May 26, 2024

shasson5 commented May 27, 2024

shasson5 commented May 16, 2024 •

edited

shasson5 May 19, 2024 •

edited