UCP/WIREUP: Add flush+destroy of UCT EPs used by TMP EP lanes by dmitrygx · Pull Request #5613 · openucx/ucx

dmitrygx · 2020-08-24T18:07:52Z

What

Add flush+destroy of UCT EPs used by TMP EP lanes.

Why ?

Will be used by #5582 to flush+destroy of UCT EPs used only by TMP EP in order to correctly send data that has to be transferred before closing the UCT EP.
Depends on #5612.

How ?

When WIREUP EP needs to be closed (flush_flag=CANCEL) or replaced by real UCT EP (flush_flag=LOCAL), schedule flush+destroy of all UCT EP lanes used by TMP EP.
Just destroy lanes that are used by the main UCP EP to NULL in TMP EP lanes in order to not flush them.
When flush+destroy process completed, call complete_cb if it was specified by the caller (e.g. it allows continue ucp_wireup_ep_progress()).

dmitrygx · 2020-08-25T08:25:44Z

waiting #5612

dmitrygx · 2020-08-25T09:33:25Z

@brminich @evgeny-leksikov could you review pls?

dmitrygx · 2020-08-27T06:16:25Z

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

src/ucp/wireup/wireup.c

brminich · 2020-08-27T06:36:55Z

src/ucp/wireup/wireup_ep.c

+    ucp_wireup_ep_t *wireup_ep = (ucp_wireup_ep_t*)user_data;
+
+    if (wireup_ep == NULL) {
+        /* check for NULL pointer to workaround Coverity warning (it wrongly


would assert help here instead?

yes, will try ucs_assert_always()

ucs_assert() should be enough to suppress coverity

ucs_assert() should be enough to suppress coverity

yes, but I guess it's OK to have always, since it's not a critical path

brminich · 2020-08-27T06:43:16Z

src/ucp/wireup/wireup_ep.c

+    ucs_assert(tmp_ep != ep);
+
+    /* to prevent flush+destroy UCT EPs that are used by the main EP,
+     * they have to be remove from the TMP EP lanes and their WIREUP


src/ucp/wireup/wireup_ep.c

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

@brminich @evgeny-leksikov your comments were addressed, could you review again pls?

evgeny-leksikov · 2020-08-28T07:38:31Z

src/ucp/wireup/wireup.c

+
+    for (another_lane = 0; another_lane < another_ep_config_key->num_lanes;
+         ++another_lane) {
+        if (ucp_ep_config_lane_is_equal(ep_config_key,


this condition has the same result for any another_lane value.

whole ucp_wireup_ep_lane_used_by_another_ep_config func looks confusing to me

so since major change is based on this func it should be revised

good catch, fixed

evgeny-leksikov · 2020-08-28T13:18:55Z

src/ucp/core/ucp_ep.c

+    return (key1->lanes[lane1].rsc_index    == key2->lanes[lane2].rsc_index)    &&
+           (key1->lanes[lane1].proxy_lane   == key2->lanes[lane2].proxy_lane)   &&
+           (key1->lanes[lane1].dst_md_index == key2->lanes[lane2].dst_md_index) &&
+           (key1->lanes[lane1].path_index   == key2->lanes[lane2].path_index)   &&
+           ((key1->lanes[lane1].lane_types  == key2->lanes[lane2].lane_types) ||


memcmp? or even (key1->lanes[lane1] == key2->lanes[lane2]

no, wouldn't work since we don't want compare lane_tpyes if it wasn't requested

sorry, missed extra () around lane_types comparison, better don't align different block, maybe even separate by extra line to improve readability?

or

if (compare_types) { return key1->lanes[lane1] == key2->lanes[lane2]; } else { return (key1->lanes[lane].rsc_index == key2->lanes[lane2].rsc_index) && (key1->lanes[lane].proxy_lane == key2->lanes[lane2].proxy_lane) && (key1->lanes[lane].dst_md_index == key2->lanes[lane2].dst_md_index) && (key1->lanes[lane].path_index == key2->lanes[lane2].path_index); }

src/ucp/core/ucp_ep.c

src/ucp/wireup/wireup.c

brminich · 2020-08-28T13:39:00Z

src/ucp/wireup/wireup.c

+         ++another_lane) {
+        if (ucp_ep_config_lane_is_equal(ep_config_key,
+                                        another_ep_config_key,
+                                        lane, another_lane, 0)) {


btw, why don't we need to compare types?

the can be different, we need to check whether this is the same TL+device pair or not

brminich · 2020-08-28T13:41:31Z

src/ucp/wireup/wireup_ep.c

+
+    /* check for NULL pointer to workaround Coverity warning (it wrongly
+     * assumes that this callback could be called upon GET/PUT operation) */
+    ucs_assertv_always(wireup_ep == NULL,


good catch, sure
it is currently dead code, since all lanes in UCP EP are identical to TMP EP's one
so, UCP EP is responsible for them, and TMP EP don't' flush and delete them

src/ucp/wireup/wireup_ep.c

brminich · 2020-08-28T14:13:11Z

src/ucp/core/ucp_ep.c

-           (key1->lanes[lane].path_index   == key2->lanes[lane].path_index)   &&
-           ((key1->lanes[lane].lane_types  == key2->lanes[lane].lane_types) ||
+    return (key1->lanes[lane1].rsc_index    == key2->lanes[lane2].rsc_index)    &&
+           (key1->lanes[lane1].proxy_lane   == key2->lanes[lane2].proxy_lane)   &&


comparing lane numbers from different configs is erroneous, also does not seem to be needed

ye, fixed
no need this

dmitrygx · 2020-09-01T04:13:19Z

bot:pipe:retest

dmitrygx · 2020-09-01T07:16:57Z

bot:pipe:retest

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

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

brminich · 2020-09-01T08:06:32Z

src/ucp/core/ucp_ep.c

+                                        ucp_lane_index_t lane1,
+                                        ucp_lane_index_t lane2)
+{
+    return !memcmp(&key1->lanes[lane1], &key2->lanes[lane2],


can padding be different?

yes, better not to do this

brminich · 2020-09-01T08:09:00Z

src/ucp/wireup/select.c

+            ucs_assertv_always(dst_dev_index == lane_desc->dst_dev_index,
                               "lane[%d].dst_md_index=%d, dst_md_index=%d",
                               lane, lane_desc->dst_md_index, dst_md_index);
+            ucs_assertv_always(dst_md_index == lane_desc->dst_md_index,
+                               "lane[%d].dst_dev_index=%d, dst_dev_index=%d",


traces are incorrect, you mention dst_dev for dst_md and vice versa

evgeny-leksikov · 2020-09-01T10:20:41Z

src/ucp/core/ucp_ep.c

+int ucp_ep_config_lane_tl_is_equal(const ucp_ep_config_key_t *key1,
+                                   const ucp_ep_config_key_t *key2,
+                                   ucp_lane_index_t lane1,
+                                   ucp_lane_index_t lane2)


minor: maybe reorder args like key1, lane1, key2, lane2 here and below?

evgeny-leksikov · 2020-09-01T10:23:01Z

src/ucp/core/ucp_ep.h

                                                  defined by
                                                  UCP_WIREUP_SA_DATA_xx */
-    uint8_t                   dev_index;     /**< Device address index used to
+    ucp_rsc_index_t           dev_index;     /**< Device address index used to


pls use fixed sized types for wire protocols

evgeny-leksikov · 2020-09-01T10:24:46Z

src/ucp/wireup/select.c

+    dst_md_index  = select_params->address->address_list
+                                 [select_info->addr_index].md_index;
+    dst_dev_index = select_params->address->address_list
+                                 [select_info->addr_index].dev_index;


minor: can create local pointer to select_params->address->address_list or even its entry

evgeny-leksikov · 2020-09-01T10:26:32Z

src/ucp/wireup/wireup.c

+ucp_wireup_ep_configs_use_same_lane(ucp_ep_config_key_t *ep_config_key,
+                                    ucp_ep_config_key_t *another_ep_config_key,


just key1 and key2 like ucp_ep_config_lane_tl_is_equal

and ucp_wireup_ep_configs_use_same_tl_lane
to be consistent

evgeny-leksikov · 2020-09-01T10:29:17Z

src/ucp/wireup/wireup.c

+                                    ucp_ep_config_key_t *another_ep_config_key,
+                                    ucp_lane_index_t lane)
+{
+    ucp_lane_index_t another_lane;


lane_idx shorter and typically used

evgeny-leksikov · 2020-09-01T10:32:47Z

src/ucp/wireup/wireup_ep.c

    if (self->tmp_ep != NULL) {
        ucs_assert(!(self->tmp_ep->flags & UCP_EP_FLAG_USED));
-        ucp_ep_disconnected(self->tmp_ep, 1);
+        ucp_wireup_tmp_ep_destroy(ucp_ep, self, UCT_FLUSH_FLAG_CANCEL, NULL);


why ret value is not checked here? request can leak here

added a TODO that it will be replaced by ucp_worker_discard_uct_ep() when 5608 gets merged

src/ucp/wireup/wireup_ep.h

dmitrygx · 2020-09-02T08:44:15Z

@brminich @yosefe could you review pls?

brminich · 2020-09-02T09:15:55Z

@dmitrygx, plz fix merge conflicts

dmitrygx · 2020-09-02T09:36:28Z

@dmitrygx, plz fix merge conflicts

@brminich done

yosefe · 2020-09-02T09:42:25Z

src/ucp/core/ucp_ep.c

-int ucp_ep_config_lane_is_equal(const ucp_ep_config_key_t *key1,
-                                const ucp_ep_config_key_t *key2,
-                                ucp_lane_index_t lane, int compare_types)
+int ucp_ep_config_lane_tl_is_equal(const ucp_ep_config_key_t *key1,


ucp_ep_config_lane_is_same_peer

yosefe · 2020-09-02T09:42:57Z

src/ucp/core/ucp_ep.c

+                                        ucp_lane_index_t lane1,
+                                        ucp_lane_index_t lane2)


need only one lane

yosefe · 2020-09-02T09:44:44Z

src/ucp/wireup/wireup.c

 }

+ucp_lane_index_t
+ucp_wireup_ep_configs_use_same_tl_lane(ucp_ep_config_key_t *key1,


ucp_wireup_ep_configs_can_reuse_lane

yosefe · 2020-09-02T09:46:04Z

src/ucp/wireup/wireup_ep.c

+
+    wireup_ep->tmp_ep = NULL;
+
+    req = ucp_ep_flush_internal(tmp_ep, ep_flush_flags, 0, &param, NULL,


why do we need flush on tmp_ep? can't we just ep_discard all the lanes we don't need?

they need to be converted from WIREUP EP to UCT EP
I'll use @alinask implementation for destroying TMP EP and will add ep_discard there

but need to merge ep_discard first

dmitrygx force-pushed the topic/ucp/wireup_tmp_ep branch from a9319ee to 19b7064 Compare August 25, 2020 07:33

dmitrygx added the WIP-DNM Work in progress / Do not review label Aug 25, 2020

dmitrygx removed the WIP-DNM Work in progress / Do not review label Aug 25, 2020

dmitrygx force-pushed the topic/ucp/wireup_tmp_ep branch from 19b7064 to 104057c Compare August 25, 2020 09:33

dmitrygx force-pushed the topic/ucp/wireup_tmp_ep branch 4 times, most recently from 6bf6ea4 to c860575 Compare August 25, 2020 18:39

UCP/WIREUP: Add flush+destroy of UCT EPs used by TMP EP lanes

7de5445

dmitrygx force-pushed the topic/ucp/wireup_tmp_ep branch from c860575 to 7de5445 Compare August 25, 2020 18:57

brminich reviewed Aug 27, 2020

View reviewed changes

UCP/WIREUP: Fix review comments

631b32c

evgeny-leksikov reviewed Aug 28, 2020

View reviewed changes

UCP/WIREUP: Fix comments

72190be

evgeny-leksikov reviewed Aug 28, 2020

View reviewed changes

brminich reviewed Aug 28, 2020

View reviewed changes

dmitrygx added 2 commits August 28, 2020 15:37

UCP/WIREUP/CORE: Fix review comments

cdd175c

UCP/WIREUP: Use dst_dev_index instead of dst_md_index

4e1c626

dmitrygx force-pushed the topic/ucp/wireup_tmp_ep branch from aec5a88 to 4e1c626 Compare August 31, 2020 18:48

brminich reviewed Sep 1, 2020

View reviewed changes

UCP/CORE/WIREUP: Fix review comments

88b41c8

evgeny-leksikov reviewed Sep 1, 2020

View reviewed changes

UCP/WIREUP/CORE: Fix review comments

9df2346

dmitrygx force-pushed the topic/ucp/wireup_tmp_ep branch from 9f9bcdd to 9df2346 Compare September 2, 2020 05:55

evgeny-leksikov approved these changes Sep 2, 2020

View reviewed changes

Merge branch 'master' into topic/ucp/wireup_tmp_ep

41b63de

yosefe reviewed Sep 2, 2020

View reviewed changes

dmitrygx added the WIP-DNM Work in progress / Do not review label Sep 2, 2020

PREPARE

b94d406

dmitrygx closed this Sep 15, 2020

		ucp_wireup_ep_configs_use_same_lane(ucp_ep_config_key_t *ep_config_key,
		ucp_ep_config_key_t *another_ep_config_key,


		wireup_ep->tmp_ep = NULL;

		req = ucp_ep_flush_internal(tmp_ep, ep_flush_flags, 0, &param, NULL,

Conversation

dmitrygx commented Aug 24, 2020

What

Why ?

How ?

Uh oh!

dmitrygx commented Aug 25, 2020

Uh oh!

dmitrygx commented Aug 25, 2020

Uh oh!

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

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!

Uh oh!

dmitrygx commented Aug 27, 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!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmitrygx commented Sep 1, 2020

Uh oh!

dmitrygx commented Sep 1, 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!

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 •

edited

Loading