UCP/WIREUP: Implement common copying of lanes function for futher re-use by dmitrygx · Pull Request #5610 · openucx/ucx

dmitrygx · 2020-08-24T10:22:13Z

What

Implement common copying of lanes function for futher re-use.

Why ?

To shorten #5582 and simplify maintaining the main EP's and TMP EP's lanes that may point to the same WIREUP EP.
Using different WIRUEP EPs that point to the same UCT EPs (with different is_owner flag - depends on the state of WIREUP) makes it possible.

How ?

Accept is_owner flag in ucp_wireup_ep_set_next_ep() to mark WIREUP EPs that are owner/not owner for UCT EP that's under the hood.
Repalce ucp_cm_client_restore_ep() by ucp_cm_copy_ep_lanes() that copies lanes? creates new WIREUP EPs for lanes and changes ownership of UCT EPs if needed.

dmitrygx · 2020-08-24T16:34:39Z

@evgeny-leksikov @alinask @brminich could you review pls?

yosefe · 2020-08-25T09:33:57Z

+            continue;
+        }
+
+        uct_ep = ucp_wireup_extract_lane(from_ep, lane_idx);


how can we know if from_ep lane was the owner of uct_ep?

we don't need to know whether it's owner or not
if we are changing ownership, to_ep will be owner, otherwise - from_ep is owner

but if "from_ep" was NOT the owner, it means the "to_ep" should not be the owner as well, right?

let's add assertion that from_ep is owner for the UCT EP - it is expected behavior

dmitrygx · 2020-08-25T15:10:12Z

@brminich @evgeny-leksikov could you review pls?

…T EP

brminich · 2020-08-26T06:30:11Z

-            w_ep->super.ucp_ep = ucp_ep;
-        }
+    if (change_ownership) {
+        to_is_owner   = 1;


minor: can do shorter:

to_is_owner = change_ownership; from_is_owner = !change_ownership;

evgeny-leksikov · 2020-08-26T07:21:05Z


-static void ucp_cm_client_restore_ep(ucp_wireup_ep_t *wireup_cm_ep,
-                                     ucp_ep_h ucp_ep)
+static void ucp_cm_copy_ep_lanes(ucp_ep_h to_ep, ucp_ep_h from_ep,


pls document all the parameters. How the EPs has to be created and initialized before the call? if to_ep is not initialized, then maybe make it [out] parameter of ucp_cm_ep_dup?

done, added the description
to_ep has to be initialized, so, no need to duplicate it here

so in this case here should be 2 iterators and to_ep should be reconfigured since to_ep can have some lanes initialized.

we have assert that to_ep doesn't have UCT_EPs for the lanes

which is removed?

which is removed?

assert() doesn't work, since configurations are not equal:

TMP EP has two lanes: CM lane and transport lane

UCP EP has only one lane: CM lane

After we copy UCT EPs from TMP EP to UCP EP, it will do init_lanes() where new config will be selected
The new config could be equal to what we have in TMP EP, then UCT EPs copied to UCP EP will be re-used in the new config. In the current code, it has to be the same.

yosefe · 2020-08-26T09:06:49Z


+        status = ucp_wireup_ep_create(to_ep, &to_ep->uct_eps[lane_idx]);
+        if (status != UCS_OK) {
+            /* coverity[leaked_storage] */


if we failed to create wireup_ep, what happens to the lane we extracted?

added ucs_fatal() instead of continue;

dmitrygx · 2020-08-27T19:28:00Z

@brminich @evgeny-leksikov @yosefe your comments were addressed, could you review pls?
the CI was restarted, since the PR has small merge conflict with #5618 that was merged today morning

brminich · 2020-08-28T05:59:06Z

errors look relevant:

[     INFO ] server listening on fe80::268a:7ff:fe8d:af27:48488
[swx-rdmz-ucx-new-01:22258:0:22258]   wireup_ep.c:42   Assertion `!(wireup_ep->flags & UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED)' failed

/scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucp/wireup/wireup_ep.c: [ ucp_wireup_ep_connect_to_ep() ]
      ...
       39     ucp_wireup_ep_t *wireup_ep = ucp_wireup_ep(uct_ep);
       40 
       41     ucs_assert(!(wireup_ep->flags & UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED));
==>    42     wireup_ep->flags |= UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED;
       43     return uct_ep_connect_to_ep(wireup_ep->super.uct_ep, dev_addr, ep_addr);
       44 }
       45

dmitrygx · 2020-08-28T11:42:01Z

errors look relevant:

@brminich fixed

brminich · 2020-08-28T14:30:42Z

    }

-    ucp_proxy_ep_set_uct_ep(&wireup_ep->super, next_ep, 1);
+    ucp_wireup_ep_set_next_ep(uct_ep, next_ep, 1, 0);


why needed?

ucp_wireup_ep_set_next_ep() does additional checks

brminich · 2020-08-28T14:31:20Z

+    if (is_local_connected) {
+        wireup_ep->flags |= UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED;
+    }


why need this? can just call ucp_proxy_ep_set_uct_ep instead like it is done now

I wanted to use ucp_wireup_ep_set_next_ep() akways instead of:

if (local_conencted) { ucp_wireup_ep_set_next_ep(); } else { ucp_proxy_ep_set_uct_ep(); }

brminich · 2020-08-28T14:34:39Z

+                             (from_wireup_ep->flags &
+                              UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED);
+
+        status = ucp_wireup_ep_create(to_ep, &to_ep->uct_eps[lane_idx]);


can we just take existing wireup ep, like it was done before?

no, since we will need this to set who is the owner of the UCT EP- TMP EP or user's UCP EP

dmitrygx · 2020-08-29T05:07:38Z

/azp run

azure-pipelines · 2020-08-29T05:07:51Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-09-01T07:46:20Z

@brminich @evgeny-leksikov @yosefe could you review pls?

brminich · 2020-09-01T07:58:44Z

+                                  to_is_owner, is_local_connected);
+
+        if (from_wireup_ep == NULL) {
+            /* from_ep must be the owner of the UCT EP in this case */


can you please elaborate? why?

evgeny-leksikov · 2020-09-01T10:15:12Z

+        is_local_connected = (from_wireup_ep == NULL) ||
+                             (from_wireup_ep->flags &
+                              UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED);


should not we inherit other wireup flags? if yes - maybe set them outside of ucp_wireup_ep_set_next_ep or pass instead of is_local_connected?

dmitrygx · 2020-09-02T11:42:31Z

@brminich @evgeny-leksikov @yosefe could you review pls?

brminich · 2020-09-02T12:37:51Z

+
+        if (from_wireup_ep == NULL) {
+            /* wireup EPs couldn't exist for the from_ep lanes if this is
+             * called on the server side of the conenction to copy the lanes


what does it mean? This function is not invoked on the server side

yes, but it will be called

dmitrygx mentioned this pull request Aug 24, 2020

UCP/WIREUP: Implement multi-lane/shm support for CM #5582

Closed

UCP/WIREUP: Implement common copying of lanes function for futher re-use

d96e4da

dmitrygx force-pushed the topic/ucp/pre_cm branch from 00773c2 to d96e4da Compare August 25, 2020 07:37

yosefe reviewed Aug 25, 2020

View reviewed changes

dmitrygx added 2 commits August 25, 2020 16:26

UCP/WIREUP: Add assertion to make sure that from_ep is an owner of UC…

9f70f8d

…T EP

UCP/WIREUP: Fix assertion

9b9e72d

brminich reviewed Aug 26, 2020

View reviewed changes

UCP/WIREUP: Don't create WIREUP EP in to_ep if there is no UCT_EP

224b244

evgeny-leksikov reviewed Aug 26, 2020

View reviewed changes

UCP/WIREUP: Fix review comments

312321a

yosefe reviewed Aug 26, 2020

View reviewed changes

dmitrygx added 6 commits August 26, 2020 10:29

UCP/WIREUP: Report fatal error if failed to allocate WIREUP EP

2754df7

UCP/WIREUP: Check that to_ep and from_ep have the same config

d8c27f1

UCP/WIREUP: Compare config_kets instead of cfg_index

469168c

UCP/WIREUP: No need for assert

d698bf6

UCP/WIREUP: Improve documentation

ed8a029

Merge branch 'master' into topic/ucp/pre_cm

4f3c94e

dmitrygx added 2 commits August 28, 2020 09:23

UCP/WIREUP: Set local_conencted if needed

be70c93

UCP/WIREUP: Fix resolving proxy lanes

27d718c

brminich reviewed Aug 28, 2020

View reviewed changes

brminich reviewed Sep 1, 2020

View reviewed changes

evgeny-leksikov reviewed Sep 1, 2020

View reviewed changes

dmitrygx added 4 commits September 1, 2020 13:04

UCP/WIREUP: Fix review comments

ec96435

UCP/WIREUP: Remove debug note from LOCAL_CONENCTED flag

5d2aa0a

UCP/WIREUP: Fix static checker issue

863b57d

Merge branch 'master' into topic/ucp/pre_cm

4c78ae0

brminich reviewed Sep 2, 2020

View reviewed changes

dmitrygx closed this Sep 15, 2020

Conversation

dmitrygx commented Aug 24, 2020

What

Why ?

How ?

Uh oh!

dmitrygx commented Aug 24, 2020

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 Aug 25, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrygx Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrygx Aug 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Aug 27, 2020

Uh oh!

brminich commented Aug 28, 2020

Uh oh!

dmitrygx commented Aug 28, 2020

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 Aug 29, 2020

Uh oh!

azure-pipelines Bot commented Aug 29, 2020

Uh oh!

dmitrygx commented Sep 1, 2020

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 Sep 2, 2020

Uh oh!

Choose a reason for hiding this comment

dmitrygx Aug 26, 2020 •

edited

Loading

dmitrygx Aug 26, 2020 •

edited

Loading