UCP/WIREUP: Support EP reconfiguration for non-reused p2p scenarios #9949

shasson5 · 2024-06-11T09:57:06Z

What

Support partial EP reconfiguration: only non-reused, fully wired-up scenarios. (Lanes must not be reused and must all be p2p).

Why ?

PR 2 for EP reconfiguration.

How ?

Add a new function for replacing wireup lane by moving internal AUX EP to the new lane.
The following steps are performed:

Set uct_ep to be used for new lane:
a) for CM flow -> use CM EP.
b) otherwise -> create a new wireup_ep.
Move aux ep from old config to new wireup lane.
Delete old wireup lane.
Select lane to be used as new wireup lane:
a) for CM flow -> use CM lane.
b) otherwise -> select a random lane. (Will be modified in next PRs to look for a non-reused lane).

shasson5 · 2024-07-23T08:22:36Z

@yosefe @gleon99 please review

gleon99 · 2024-07-28T16:19:32Z

@shasson5 please see clang-format output.

test/gtest/ucp/test_ucp_reconfigure.cc

test/gtest/ucp/ucp_test.h

test/gtest/ucp/test_ucp_reconfigure.cc

src/ucp/wireup/wireup.c

src/ucp/wireup/wireup_ep.c

shasson5 · 2024-08-01T14:29:09Z

/azp run

azure-pipelines · 2024-08-01T14:29:39Z

Azure Pipelines successfully started running 5 pipeline(s).

brminich · 2024-08-06T12:42:52Z

src/ucp/wireup/wireup.c

+        ucs_assert(reuse_lane_map[ucp_ep_get_cm_lane(ep)] != UCP_NULL_LANE);
+        /* wireup lane hasn't been selected by the new configuration: only this
+         * function should select it */
+        ucs_assert(new_key->wireup_msg_lane == UCP_NULL_LANE);


Suggested change

ucs_assert(new_key->wireup_msg_lane == UCP_NULL_LANE);

ucs_assertv(new_key->wireup_msg_lane == UCP_NULL_LANE, "new_key->wireup_msg_lane=%u",

new_key->wireup_msg_lane);

that's code from master.
Was marked as "new added" code for some wrong reason.

This code is indeed from master. Marked as diff because of indentation (if block).
IMO the suggestions makes sense.

brminich · 2024-08-06T12:53:26Z

src/ucp/wireup/wireup.c

+
+    /* Select CM/non-reused lane as new wireup lane */
+    new_lane              = ucp_wireup_find_non_reused_lane(ep, key);
+    new_uct_eps[new_lane] = &new_ep->super.super;


can it be later overwritten in ucp_wireup_check_config_intersect? Is it possible to initialize new_uct_eps in one place? (would be less error-prone)

I think it may complicate the flow, because you need to return [new_lane,new_ep] from this function and then add extra "if" block in ucp_wireup_check_config_intersect in order to properly initialize new_uct_eps.

And anyway the value of new_uct_eps will not be overwritten, because in ucp_wireup_check_config_intersect we only handle "reused" lanes.

shasson5 · 2024-09-01T16:51:04Z

/azp run

azure-pipelines · 2024-09-01T16:51:28Z

Azure Pipelines successfully started running 5 pipeline(s).

src/ucp/wireup/wireup.c

test/gtest/ucp/test_ucp_reconfigure.cc

src/ucp/wireup/wireup.c

test/gtest/ucp/test_ucp_ep_reconfigure.cc

gleon99 · 2024-09-08T07:53:22Z

@yosefe please look, as discussed

yosefe · 2024-09-08T15:08:49Z

test/gtest/Makefile.am

@@ -151,6 +151,7 @@ gtest_SOURCES = \
 	ucp/test_ucp_mem_type.cc \
 	ucp/test_ucp_perf.cc \
 	ucp/test_ucp_proto.cc \
+	ucp/test_ucp_ep_reconfigure.cc \	


maybe rename to test_ucp_ep_reconfig.cc?

src/ucp/wireup/wireup.c

yosefe · 2024-09-08T15:25:11Z

src/ucp/wireup/wireup.c

@@ -1501,7 +1677,8 @@ ucs_status_t ucp_wireup_init_lanes(ucp_ep_h ep, unsigned ep_init_flags,
     * without CM to prevent reconfiguration error.
     */
    if ((ep->cfg_index != UCP_WORKER_CFG_INDEX_NULL) &&
-        !ucp_ep_has_cm_lane(ep)) {
+        !ucp_wireup_can_reconfigure(ep, ep_init_flags, &tl_bitmap,


better avoid calling init_lanes another time just for checking if we need to limit tl_bitmap,
can we do this hack (restrict tl_bitmap) only if we are going to print a fatal error that reconfig not supported?

src/ucp/wireup/wireup.c

yosefe · 2024-09-08T16:13:06Z

src/ucp/wireup/wireup.c

+    msg_ep = ucp_wireup_ep_extract_msg_ep(old_ep, &pending_queue);
+    ucp_wireup_ep_set_aux(new_ep, msg_ep, aux_rsc_index, is_p2p);
+
+    /* Add wireup messages to pending queue */


ucp_wireup_ep_extract_msg_ep is currently NOT returning wireup message requests

yosefe · 2024-09-08T16:20:42Z

src/ucp/wireup/wireup.c

+
+    /* Get old wireup lane */
+    old_lane = ucp_wireup_get_msg_lane(ep, UCP_WIREUP_MSG_REQUEST);
+    old_ep   = ucp_wireup_ep(ucp_ep_get_lane(ep, old_lane));


old_wireup_ep?

yosefe · 2024-09-08T16:21:15Z

src/ucp/wireup/wireup.c

+    ucp_ep_set_lane(ep, old_lane, NULL);
+
+    /* Select CM/non-reused lane as new wireup lane */
+    new_lane              = ucp_wireup_find_non_reused_lane(ep, key, 


new_wireup_lane?

yosefe · 2024-09-08T16:21:45Z

src/ucp/wireup/wireup.c

+    if (uct_ep != NULL) {
+        uct_ep_destroy(uct_ep);
+    }
+


remove spaceline

yosefe · 2024-09-08T16:23:45Z

src/ucp/wireup/wireup_ep.c

+    uct_ep_h msg_ep = ucp_wireup_ep_get_msg_ep(wireup_ep);
+
+    uct_ep_pending_purge(&wireup_ep->super.super,
+                         ucp_wireup_ep_append_pending_cb, pending_queue);


seems ucp_wireup_ep_pending_purge (called from here) already calls ucp_wireup_ep_pending_req_release which decrements pending_count by 1

yosefe · 2024-09-10T16:52:17Z

src/ucp/wireup/wireup.c

+    /* Get all reachable MDs from full remote address list and join with
+     * current ep configuration
+     */
+    key->dst_md_cmpts = dst_md_storage;


minor: move it after line 1609

you mean move it to be before 1609?

either before or after key->dst_version = remote_address->dst_version;

src/ucp/wireup/wireup.c

yosefe · 2024-09-10T17:00:46Z

src/ucp/wireup/wireup_ep.c

+    ucp_wireup_ep_pending_purge(&wireup_ep->super.super,
+                                ucs_empty_function_do_assert_void, NULL);


IMO we can just assert that wireup_ep->pending_q length is 0. and call uct_ep_pending_purge. then no need to move the functions ucp_wireup_ep_pending_purge, ucp_wireup_ep_pending_req_release

ok but notice it requires duplication of ucs_assert(wireup_ep->pending_count == 0); as well

why need to check pending_count==0? ucp_wireup_ep_pending_purge would release them anyway so it doesn't have to be empty IMO

src/ucp/wireup/wireup_ep.c

test/gtest/ucp/test_ucp_ep_reconfig.cc

yosefe · 2024-09-10T17:14:17Z

test/gtest/ucp/test_ucp_ep_reconfig.cc

+        /* Verify local and remote lanes are identical */
+        EXPECT_TRUE(has_matching_lane(ep(), lane, other));


do we really need this? isn't it enough to test that communication send/recv is working?
same for checking num_lanes

how is it possible to check communication on all lanes without using multiple protocols/message sizes?

IMO we can do basic test like active message or tag match on several message sizes and extend it
since communication ability is something we need to test anyway to ensure configuration is working

maybe better as a complement?

IMO in the general case - when not all transports are p2p - we do not need to require that number of lanes is the same.

I see, so removed the num_lanes check

src/ucp/wireup/wireup_ep.c

test/gtest/ucp/test_ucp_ep_reconfig.cc

yosefe · 2024-09-11T14:07:27Z

test/gtest/ucp/test_ucp_ep_reconfig.cc

+        /* Verify local and remote lanes are identical */
+        EXPECT_TRUE(has_matching_lane(ep(), lane, other));


IMO in the general case - when not all transports are p2p - we do not need to require that number of lanes is the same.

shasson5 · 2024-09-11T15:10:14Z

@yosefe please approve

yosefe

pls squash

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

shasson5 changed the title ~~UCP/WIREUP: Support EP reconfiguration for non-reused wired-up scenarios.~~ UCP/WIREUP: Support EP reconfiguration for non-reused wired-up scenarios Jun 11, 2024

shasson5 changed the title ~~UCP/WIREUP: Support EP reconfiguration for non-reused wired-up scenarios~~ UCP/WIREUP: Support EP reconfiguration for non-reused p2p scenarios Jun 11, 2024

shasson5 changed the title ~~UCP/WIREUP: Support EP reconfiguration for non-reused p2p scenarios~~ UCP/WIREUP: Support EP reconfiguration for non-reused wireup scenarios Jun 16, 2024

shasson5 changed the title ~~UCP/WIREUP: Support EP reconfiguration for non-reused wireup scenarios~~ UCP/WIREUP: Support EP reconfiguration for non-reused p2p scenarios Jun 16, 2024

shasson5 force-pushed the no_reuse branch 4 times, most recently from 986a5c8 to 3ccadc6 Compare July 10, 2024 11:03

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

shasson5 requested review from yosefe and gleon99 July 10, 2024 11:47

shasson5 closed this Jul 29, 2024

shasson5 reopened this Jul 29, 2024

gleon99 reviewed Jul 31, 2024

View reviewed changes

shasson5 requested a review from gleon99 August 1, 2024 14:25

shasson5 requested review from gleon99 and removed request for gleon99 August 4, 2024 17:02

brminich reviewed Aug 6, 2024

View reviewed changes

shasson5 requested a review from brminich August 6, 2024 13:35

evgeny-leksikov reviewed Sep 4, 2024

View reviewed changes

shasson5 requested a review from evgeny-leksikov September 5, 2024 08:51

evgeny-leksikov reviewed Sep 5, 2024

View reviewed changes

src/ucp/wireup/wireup.c Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_ep_reconfigure.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_ep_reconfigure.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_ep_reconfigure.cc Outdated Show resolved Hide resolved

shasson5 requested a review from evgeny-leksikov September 5, 2024 13:42

evgeny-leksikov reviewed Sep 5, 2024

View reviewed changes

test/gtest/ucp/test_ucp_ep_reconfigure.cc Outdated Show resolved Hide resolved

test/gtest/ucp/test_ucp_ep_reconfigure.cc Outdated Show resolved Hide resolved

shasson5 requested a review from evgeny-leksikov September 5, 2024 14:55

evgeny-leksikov reviewed Sep 5, 2024

View reviewed changes

test/gtest/ucp/test_ucp_ep_reconfigure.cc Outdated Show resolved Hide resolved

shasson5 requested a review from evgeny-leksikov September 5, 2024 16:52

yosefe reviewed Sep 8, 2024

View reviewed changes

shasson5 requested a review from yosefe September 9, 2024 13:02

yosefe reviewed Sep 10, 2024

View reviewed changes

shasson5 requested a review from yosefe September 11, 2024 12:52

yosefe reviewed Sep 11, 2024

View reviewed changes

shasson5 requested a review from yosefe September 11, 2024 14:52

yosefe approved these changes Sep 11, 2024

View reviewed changes

UCP/WIREUP: Support EP reconfiguration for non-reused p2p scenarios

84523e9

shasson5 force-pushed the no_reuse branch from 93f5be2 to 84523e9 Compare September 11, 2024 15:21

yosefe approved these changes Sep 11, 2024

View reviewed changes

yosefe enabled auto-merge September 11, 2024 16:19

yosefe merged commit 5b7fb19 into openucx:master Sep 12, 2024
143 checks passed

	ucs_assert(new_key->wireup_msg_lane == UCP_NULL_LANE);
	ucs_assertv(new_key->wireup_msg_lane == UCP_NULL_LANE, "new_key->wireup_msg_lane=%u",
	new_key->wireup_msg_lane);

		ucp_wireup_ep_pending_purge(&wireup_ep->super.super,
		ucs_empty_function_do_assert_void, NULL);

		/* Verify local and remote lanes are identical */
		EXPECT_TRUE(has_matching_lane(ep(), lane, other));

UCP/WIREUP: Support EP reconfiguration for non-reused p2p scenarios #9949

UCP/WIREUP: Support EP reconfiguration for non-reused p2p scenarios #9949

Conversation

shasson5 commented Jun 11, 2024 • edited Loading

What

Why ?

How ?

shasson5 commented Jul 23, 2024

gleon99 commented Jul 28, 2024

shasson5 commented Aug 1, 2024

azure-pipelines bot commented Aug 1, 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

shasson5 commented Sep 1, 2024

azure-pipelines bot commented Sep 1, 2024

gleon99 commented Sep 8, 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

shasson5 Sep 11, 2024 • edited Loading

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 Sep 11, 2024

yosefe left a comment

Choose a reason for hiding this comment

shasson5 commented Jun 11, 2024 •

edited

Loading

shasson5 Sep 11, 2024 •

edited

Loading