UCP/WIREUP: Support EP reconfiguration for non wired-up scenarios #10337

shasson5 · 2024-11-28T12:58:02Z

What?

UCP/WIREUP: Support EP reconfiguration for non wired-up scenarios

Why?

This PR introduces 2 additional usecases supported:

Reconfiguration during send/recv phase (after wireup completion).
Reconfiguration during wireup phase for non p2p transports, given that one of the following conditions takes place:
- wireup lane and AM lane are equal.
- AM lane is reused.
- AM transport is p2p.

roiedanino · 2024-12-10T09:08:28Z

src/ucp/core/ucp_ep.c

+    for (lane = 0; lane < ucp_ep_num_lanes(ep); lane++) {
+        if (ucp_wireup_ep_test(ucp_ep_get_lane(ep, lane))) {
+            return lane;
+        }
+    }


Wouldn't it be better to save wireup_ep in worker, and then access it in O(1)?

roiedanino · 2024-12-10T09:13:39Z

test/gtest/ucp/test_ucp_ep_reconfig.cc

+            continue;
+        }
+
+        if (dev_count++ >= max_num_devs) {


why do you need dev_count? lane has the same values here

evgeny-leksikov · 2025-01-16T09:15:23Z

src/ucp/wireup/wireup.c

    send_reply = /* Always send the reply in case of CM, the client's EP has to
                  * be marked as REMOTE_CONNECTED */
-        has_cm_lane || (msg->dst_ep_id == UCS_PTR_MAP_KEY_INVALID) ||
-        ucp_ep_config(ep)->p2p_lanes;
+            has_cm_lane || (msg->dst_ep_id == UCS_PTR_MAP_KEY_INVALID) ||
+            ucp_ep_config(ep)->p2p_lanes || is_am_replaced;


pls add a comment for the new case

evgeny-leksikov · 2025-01-16T09:16:10Z

src/ucp/wireup/wireup.c

@@ -720,7 +723,7 @@ ucp_wireup_process_request(ucp_worker_h worker, ucp_ep_h ep,
    /* don't mark as connected to remote now in case of CM, since it destroys
     * CM wireup EP (if it is hidden in the CM lane) that is used for sending
     * WIREUP MSGs */
-    if (!ucp_ep_config(ep)->p2p_lanes && !has_cm_lane) {
+    if (!ucp_ep_config(ep)->p2p_lanes && !has_cm_lane && !is_am_replaced) {


pls add a comment

evgeny-leksikov · 2025-01-16T09:19:59Z

src/ucp/wireup/wireup.c

+    return !ucp_ep_has_cm_lane(ep) && (ep->am_lane != UCP_NULL_LANE) &&
+           (reuse_lane_map[ep->am_lane] == UCP_NULL_LANE) &&
+           !ucp_wireup_ep_test(ucp_ep_get_lane(ep, ep->am_lane));


why do we check cached am_lane lane value but not config key?

pls comment every statememt why is it in the condition

evgeny-leksikov · 2025-01-16T09:25:09Z

src/ucp/core/ucp_ep.c

+ucp_lane_index_t ucp_ep_config_find_match_lane(const ucp_ep_config_key_t *key1,
+                                               ucp_lane_index_t lane1,
+                                               const ucp_ep_config_key_t *key2)


maybe invert:

ucp_lane_index_t ucp_ep_config_find_match_lane(const ucp_ep_config_key_t *key1, const ucp_ep_config_key_t *key2, ucp_lane_index_t lane2, )

to align with ucp_ep_config_lane_is_equal?

my suggestion also assumed inverting an implementation - to find the lane in key1 which equals to lane2 in key2, otherwise mixing of the arguments in usage is not clear

not sure it will be clearer because currently key1 corresponds to old_key and key2 corresponds to new_key

evgeny-leksikov · 2025-01-16T09:31:59Z

src/ucp/wireup/wireup.c

+        }
+    }
+
+    return ucp_ep_num_lanes(ep) != num_lanes;


I would move this check before the loop

evgeny-leksikov · 2025-01-16T09:42:15Z

test/gtest/ucp/test_ucp_ep_reconfig.cc

@@ -147,7 +149,7 @@ class test_ucp_ep_reconfig : public ucp_test {
        static const unsigned num_iterations = 1000;
 /* TODO: remove this when 100MB asan bug is solved */
 #ifdef __SANITIZE_ADDRESS__
-        static const size_t msg_sizes[] = {8, 1024, 16384, 65536};
+        static const size_t msg_sizes[] = {8, 1024, 16384, 32768};


why reduced?

there's a bug in rcache when data size is larger then ~70MB

pls update the TODO above

evgeny-leksikov · 2025-01-16T09:45:47Z

test/gtest/ucp/test_ucp_ep_reconfig.cc

+                                num_shm_lanes();
+}
+
+unsigned test_ucp_ep_reconfig::entity::num_shm_lanes() const


I'm not sure that num devices always equals to num lanes, pls rename the function and comment the assumption on usage

evgeny-leksikov · 2025-01-16T09:49:18Z

test/gtest/ucp/test_ucp_ep_reconfig.cc

+
+    void init() override
+    {
+        static const std::string ib_tls[] = {"rc_mlx5", "dc_mlx5", "rc_verbs",


with modern C++ you can use std::vector then usage of std::any looks simpler
btw, mybe has_any_transport can be reused?

has_any_transport does not help for usecase of shm,ib because it will return true for IB even if there's only SHM on the setup

evgeny-leksikov · 2025-01-16T09:56:18Z

test/gtest/ucp/test_ucp_ep_reconfig.cc

 UCS_TEST_SKIP_COND_P(test_reconfig_asymmetric, request_reset,
-                     has_transport("shm"), "PROTO_REQUEST_RESET=y")
+                     !has_transport("ib"), "PROTO_REQUEST_RESET=y")


new condition reduces test coverage, why?

evgeny-leksikov · 2025-01-16T10:06:49Z

src/ucp/wireup/wireup.c

+    /* TODO: 2) Support reconfiguration for separated wireup and AM lanes
+     *          during wireup process (request sent). */
+    return !(ep->flags & UCP_EP_FLAG_CONNECT_REQ_QUEUED) ||
+           (ep->am_lane == wireup_lane) ||
+           !ucp_wireup_is_am_lane_replaced(ep, reuse_lane_map);


so, from my understanding this PR firstly has the deal with wireup_msg_lane, not an am_lane, then need to rename all related things, like is_am_replaced -> is_wireup_msg_replaced, use corresponding lane access wrapper, etc

actually it's also deals with different AM lane and wireup lane, provided that wireup process was completed.

In the code I see changes related to ep->am_lane which is equals to wireup_msg_lane in most particular configurations. And this case is implemented in this PR but common case is in TODO (2)
pls add a summary to the PR, probably my understanding of the change is wrong

why is it common case?

roiedanino assigned shasson5 Dec 10, 2024

roiedanino reviewed Dec 10, 2024

View reviewed changes

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

shasson5 force-pushed the reconf4 branch from 9b91d9d to b681b04 Compare December 10, 2024 11:41

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

shasson5 requested review from yosefe and gleon99 December 10, 2024 14:22

shasson5 removed their assignment Dec 10, 2024

shasson5 force-pushed the reconf4 branch 2 times, most recently from 6194b9a to 05a18e8 Compare December 12, 2024 19:39

shasson5 removed request for yosefe and gleon99 December 12, 2024 19:43

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

shasson5 force-pushed the reconf4 branch 2 times, most recently from 7261edc to 3eb6604 Compare January 2, 2025 16:48

UCP/WIREUP: Support EP reconfiguration for non wired-up scenarios

fb4a8f9

shasson5 force-pushed the reconf4 branch from 3eb6604 to fb4a8f9 Compare January 2, 2025 17:47

shasson5 requested review from yosefe and gleon99 January 2, 2025 17:54

shasson5 removed the WIP-DNM Work in progress / Do not review label Jan 2, 2025

shasson5 added 4 commits January 2, 2025 18:56

UCP/TEST: fix CI failure

8787274

UCP/TEST: CI fix

5fe0825

UCP/TEST: minor fix

618f4f8

UCP/TEST: minor fix

0201829

evgeny-leksikov reviewed Jan 16, 2025

View reviewed changes

shasson5 added 2 commits January 16, 2025 11:15

Merge remote-tracking branch 'upstream/master' into reconf4

89c1b2b

UCP/WIREUP: CR fixes

bbdf202

shasson5 requested a review from evgeny-leksikov January 16, 2025 14:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCP/WIREUP: Support EP reconfiguration for non wired-up scenarios #10337

UCP/WIREUP: Support EP reconfiguration for non wired-up scenarios #10337

shasson5 commented Nov 28, 2024 •

edited

Loading

roiedanino Dec 10, 2024

roiedanino Dec 10, 2024 •

edited

Loading

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

shasson5 Jan 16, 2025

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

shasson5 Jan 16, 2025

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

shasson5 Jan 16, 2025

evgeny-leksikov Jan 16, 2025

evgeny-leksikov Jan 16, 2025

shasson5 Jan 16, 2025

evgeny-leksikov Jan 16, 2025

shasson5 Jan 16, 2025

UCP/WIREUP: Support EP reconfiguration for non wired-up scenarios #10337

Are you sure you want to change the base?

UCP/WIREUP: Support EP reconfiguration for non wired-up scenarios #10337

Conversation

shasson5 commented Nov 28, 2024 • edited Loading

What?

Why?

Choose a reason for hiding this comment

roiedanino Dec 10, 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

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 commented Nov 28, 2024 •

edited

Loading

roiedanino Dec 10, 2024 •

edited

Loading