UCP/CORE/GTEST: Introduce the test to check an equality of CM and WIREUP configs by dmitrygx · Pull Request #5793 · openucx/ucx

dmitrygx · 2020-10-15T07:49:05Z

What

Introduce the test to check an equality of CM and WIREUP configs.

Why ?

To test #5696.

How ?

The test does the following:

Creates EP using CM, does send-recv, saves its config.
Creates EP using WIREUP, does send-recv, saves its config.
Compares configs.

src/ucp/core/ucp_ep.c

test/gtest/ucp/test_ucp_sockaddr.cc

yosefe · 2020-10-15T11:22:55Z

test/gtest/ucp/test_ucp_sockaddr.cc

    }
 }

+


yosefe · 2020-10-15T13:04:59Z

test/gtest/ucp/test_ucp_sockaddr.cc

        EXPECT_TRUE(((lane1 == UCP_NULL_LANE) && (lane2 == UCP_NULL_LANE)) ||
-                    ucp_ep_config_lane_is_peer_equal(key1, lane1, key2, lane2));
+                    ((lane1 != UCP_NULL_LANE) && (lane2 != UCP_NULL_LANE) &&
+                      ucp_ep_config_lane_is_peer_equal(key1, lane1, key2, lane2)));


yosefe · 2020-10-15T13:09:23Z

src/ucp/wireup/wireup.c

+    cm_wireup_ep = ucp_ep_get_cm_wireup_ep(ep);
+    if ((cm_wireup_ep != NULL) && (key.cm_idx == UCP_NULL_RESOURCE)) {
+        key.cm_idx = cm_wireup_ep->cm_idx;
+    }
+


i think it should be in ucp_ep_init_create_wireup, not here
for the new config we already copy the key by ucp_wireup_check_config_intersect

CM WIREUP EP is not created yet in ucp_ep_init_create_wireup()
fixed, by initilaizing CM_IDX in key when initilaizing CM_IDX in CM WIREUP EP

brminich · 2020-10-15T15:18:00Z

test/gtest/ucp/test_ucp_sockaddr.cc

+    sender().connect(&receiver(), get_ep_params());
+    ucp_ep_params_t params = get_ep_params();
+    params.field_mask     |= UCP_EP_PARAM_FIELD_USER_DATA;
+    params.user_data       = &receiver();


why need this?

yosefe · 2020-10-15T15:41:22Z

src/ucp/wireup/wireup_cm.c

-    wireup_ep->ep_init_flags = ucp_ep_init_flags(ucp_ep->worker, params);
-    wireup_ep->cm_idx        = 0;
+    wireup_ep->ep_init_flags          = ucp_ep_init_flags(ucp_ep->worker, params);
+    ucp_ep_config(ucp_ep)->key.cm_idx =


we should not change config key after it's already saved in the worker array

yosefe · 2020-10-15T15:44:50Z

src/ucp/wireup/wireup_cm.c

+    if (status == UCS_OK) {
+        key->cm_idx = wireup_ep->cm_idx;
+    }


can do it unconditionally in line 177

yosefe · 2020-10-15T15:57:59Z

src/ucp/wireup/wireup_cm.c

    }

-    cm_wireup_ep->cm_idx = next_cm_idx;
+    ucp_ep_config(ep)->key.cm_idx =


we should not change config key after it's already saved in the worker array

dmitrygx · 2020-10-16T05:35:02Z

/azp run

dmitrygx · 2020-10-16T05:35:15Z

bot:pipe:retest

azure-pipelines · 2020-10-16T05:35:16Z

Azure Pipelines successfully started running 1 pipeline(s).

src/ucp/wireup/wireup.c

src/ucp/wireup/wireup_cm.c

yosefe · 2020-10-17T11:26:29Z

src/ucp/wireup/wireup_cm.c


    wireup_ep->ep_init_flags = ucp_ep_init_flags(ucp_ep->worker, params);
-    wireup_ep->cm_idx        = 0;
+    wireup_ep->cm_idx        = ucp_ep_config(ucp_ep)->key.cm_idx;


maybe we can remove cm_idx field from wireup_ep altogether?

unfortunately, can't remove it
we will be unable to determine whether we need to use next CM index in ucp_cm_client_priv_pack_cb() or not when initilaizing an initial config (ucp_cm_ep_client_initial_config_get()). I tried to understand checking previous config (if EP has prev config - then it is fallback, otherwise - not), but an EP config walys exist in ucp_cm_client_priv_pack_cb()

we should be able to take cm_idx from current ep config key: when ucp_worker_create_ep() is called from ucp_ep_create_to_sock_addr it should set the cm_idx to 0

Yes, but if we do fallback, ucp_cm_client_priv_pack_cb() could be called several times for each CM until CM connection is successful.
So, we don't have the way to determine whether it was called after initial config get (CM idx has to be 0) or from fallback (CM idx has to be ++).
Whatd do you think about not initializing CM index to 0 when initializing config for the first time? Initialize it or increment it only in ucp_cm_client_priv_pack_cb()?

done in another way:
saved CM index in EP extension

yosefe · 2020-10-17T19:51:05Z

src/ucp/wireup/wireup.c

+                                                ucp_ep_has_cm_lane(ep) ?
+                                                ucp_ep_config(ep)->key.cm_idx :


can't we always pass ucp_ep_config(ep)->key.cm_idx

unfortunately, returned back, since EP cfg_index could be NULL

dmitrygx · 2020-10-19T06:34:59Z

bot:pipe:retest

dmitrygx · 2020-10-19T06:35:46Z

/azp run

azure-pipelines · 2020-10-19T06:35:59Z

Azure Pipelines successfully started running 1 pipeline(s).

…EUP configs

dmitrygx · 2020-10-19T14:22:06Z

/azp run

azure-pipelines · 2020-10-19T14:22:25Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-10-20T10:33:20Z

@yosefe your comments were fixed, could you check pls?

dmitrygx · 2020-10-21T16:55:48Z

/azp run

azure-pipelines · 2020-10-21T16:56:05Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe reviewed Oct 15, 2020

View reviewed changes

src/ucp/core/ucp_ep.c Show resolved Hide resolved

test/gtest/ucp/test_ucp_sockaddr.cc Show resolved Hide resolved

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

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

yosefe reviewed Oct 15, 2020

View reviewed changes

brminich reviewed Oct 15, 2020

View reviewed changes

yosefe reviewed Oct 15, 2020

View reviewed changes

yosefe reviewed Oct 17, 2020

View reviewed changes

UCP/CORE/GTEST: Introduce the test to check an equality of CM and WIR…

48ba3ec

…EUP configs

dmitrygx force-pushed the topic/ucp/cm_mlane_gtest branch from ea6436e to 48ba3ec Compare October 19, 2020 07:54

yosefe approved these changes Oct 22, 2020

View reviewed changes

yosefe merged commit 53ff49d into openucx:master Oct 22, 2020

Conversation

dmitrygx commented Oct 15, 2020

What

Why ?

How ?

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrygx commented Oct 16, 2020

Uh oh!

dmitrygx commented Oct 16, 2020

Uh oh!

azure-pipelines bot commented Oct 16, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrygx commented Oct 19, 2020

Uh oh!

dmitrygx commented Oct 19, 2020

Uh oh!

azure-pipelines bot commented Oct 19, 2020

Uh oh!

dmitrygx commented Oct 19, 2020

Uh oh!

azure-pipelines bot commented Oct 19, 2020

Uh oh!

dmitrygx commented Oct 20, 2020

Uh oh!

dmitrygx commented Oct 21, 2020

Uh oh!

azure-pipelines bot commented Oct 21, 2020

Uh oh!

Reviewers