UCP/WIREUP: Allow EP check during WIREUP phase by dmitrygx · Pull Request #6069 · openucx/ucx

dmitrygx · 2020-12-29T14:49:41Z

What

Allow EP check during WIREUP phase, i.e. UCP will do KA during WIREUP pahse.

Why ?

The remote peer could be failed or forcibly closed during WIREUP MSG phase, e.g. as in #6011 issue:

server closes EP forcibly after receiving and handling RDMA CM event "CONNECT REQUEST".
client receives RDMA CM event "CONNECT RESPONSE".
client does rdma_establish() and UCT informs client about connect completion. CM phase is considered as completed on a client side. Server will start WIREUP MSG phase after from CM's conn_notify_cb callback
client hangs for a while since neither WIREUP "PRE_REQUEST" message nor RDMA CM event "DISCONNECTED" was received.

Fixes #6011.
Enabling KA by default (#6059) will fix the issue fully.

How ?

Do WIREUP EP check if UCT EP is locally connected or auxiliary UCT EP is not a NULL and supports UCT_IFACE_FLAG_EP_CHECK; otherwise - return UCS_OK.

yosefe · 2020-12-29T15:07:43Z

+
+    if (wireup_ep->flags & UCP_WIREUP_EP_FLAG_LOCAL_CONNECTED) {
+        return uct_ep_check(wireup_ep->super.uct_ep, flags, comp);
+    } else if ((wireup_ep->aux_ep != NULL) &&


can remove 'else'
BTW, why need to check UCT_IFACE_FLAG_EP_CHECK flag? i would expect UCP keepalive mechanism to check this already

removed
The check EP_CHECK flag is needed, since UCP KA mechanism checks it only for iface which wireup_ep->super.uct_ep belongs to (e.g. next_ep - RC, aux_ep - UD. but UD doesn't have EP_CHECK support)

so we need to make sure only transports which have CHECK support are used as aux_ep

then UD couldn't be used as aux_ep

ok, is it a problem?
BTW need to make sure UCP would really send keepalive on the ep used for wireup messages, and that ep keepalive list is updated when ep configuration changes

done, now doing EP check over UCT if connected, and otherwise - over AUX EP

dmitrygx · 2020-12-30T20:18:50Z

depends on #6080 and #6078

dmitrygx · 2021-01-01T11:10:17Z

/azp run

dmitrygx · 2021-01-01T11:10:27Z

bot:pipe:retest

azure-pipelines · 2021-01-01T11:10:29Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2021-01-01T17:06:48Z

@hoopoepg could you review pls?

dmitrygx · 2021-01-02T20:13:22Z

re-run tests against the current master, since #6059 was merged

dmitrygx · 2021-01-02T20:13:33Z

/azp run

azure-pipelines · 2021-01-02T20:13:45Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2021-01-02T20:13:49Z

bot:pipe:retest

Artemy-Mellanox · 2021-01-03T20:05:04Z


 void ucp_ep_destroy_internal(ucp_ep_h ep)
 {
+    if (--ep->ref_cnt != 0) {


This way we leave ucp_ep visible to keepalive_progress but not checkable (ep_check returns timeout). Hence warnings in ucp_ep_do_keepalive().

This way we leave ucp_ep visible to keepalive_progress but not checkable (ep_check returns timeout). Hence warnings in ucp_ep_do_keepalive().

@Artemy-Mellanox good catch, thank you!
moved this check to ucp_ep_destroy_base() where UCP EP is about to be removed

dmitrygx · 2021-01-04T23:16:33Z

@yosefe @Artemy-Mellanox could you review pls?

yosefe · 2021-01-05T10:58:17Z

+            !(/* if error handling and keepalive were requested, UCT iface has to
+               * support peer failure (i.e. UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE)
+               * and either built-in Keep Alive (i.e. UCT_IFACE_FLAG_EP_KEEPALIVE)
+               * or EP checking (i.e. UCT_IFACE_FLAG_EP_CHECK) */
+              !ucp_worker_keepalive_is_enabled(worker) ||
+              !(select_params->ep_init_flags & UCP_EP_INIT_ERR_MODE_PEER_FAILURE) ||
+              ucp_wireup_check_flags(resource, iface_attr->cap.flags,
+                                      UCT_IFACE_FLAG_EP_KEEPALIVE, criteria->title,
+                                      ucp_wireup_iface_flags, p, endp - p) ||
+              ucp_wireup_check_flags(resource, iface_attr->cap.flags,
+                                     UCT_IFACE_FLAG_EP_CHECK, criteria->title,
+                                     ucp_wireup_iface_flags, p, endp - p)) ||


maybe use a helper function?

added the helper function

yosefe · 2021-01-05T10:59:19Z

+            return uct_ep_check(wireup_ep->aux_ep, flags, comp);
+        }
+
+        ucs_assert(ucp_worker_iface(worker,


pls add comment

yosefe · 2021-01-06T09:20:21Z

+    return /* if error handling and keepalive were requested, UCT iface has to
+            * support peer failure (i.e. UCT_IFACE_FLAG_ERRHANDLE_PEER_FAILURE)
+            * and either built-in keepalive (i.e. UCT_IFACE_FLAG_EP_KEEPALIVE)
+            * or EP checking (i.e. UCT_IFACE_FLAG_EP_CHECK) */
+           !ucp_worker_keepalive_is_enabled(worker) ||
+           !(select_params->ep_init_flags & UCP_EP_INIT_ERR_MODE_PEER_FAILURE) ||
+           ucp_wireup_check_flags(resource, flags, UCT_IFACE_FLAG_EP_KEEPALIVE, title,
+                                  ucp_wireup_iface_flags, reason, max) ||
+           ucp_wireup_check_flags(resource, flags, UCT_IFACE_FLAG_EP_CHECK, title,
+                                  ucp_wireup_iface_flags, reason, max);


does it mean that every transport we select must support keepalive or ep_check?
it means that before this PR, we could have a case where the user requested err handling mode, but keepalive may not be working because selected transport did not support?

does it mean that every transport we select must support keepalive or ep_check?

if KA is enabled (now it is by default) and error handling requested, every selected transport has to support built-in KA or ep_check

it means that before this PR, we could have a case where the user requested err handling mode, but keepalive may not be working because selected transport did not support?

yes, this was a case, e.g. for UD before EP_CHECK was introduced

dmitrygx · 2021-01-06T17:02:59Z

@yosefe ok to merge?

yosefe reviewed Dec 29, 2020

View reviewed changes

dmitrygx force-pushed the topic/ucp/wireup_ep_check branch from fca6775 to 7f21d44 Compare January 2, 2021 23:02

Artemy-Mellanox reviewed Jan 3, 2021

View reviewed changes

dmitrygx added the WIP-DNM Work in progress / Do not review label Jan 4, 2021

yosefe removed the WIP-DNM Work in progress / Do not review label Jan 4, 2021

yosefe reviewed Jan 5, 2021

View reviewed changes

yosefe reviewed Jan 6, 2021

View reviewed changes

yosefe approved these changes Jan 6, 2021

View reviewed changes

UCP/WIREUP: Allow EP check during WIREUP phase

389768e

dmitrygx force-pushed the topic/ucp/wireup_ep_check branch from b9e33b9 to 389768e Compare January 6, 2021 09:43

yosefe merged commit fd8f8b2 into openucx:master Jan 6, 2021

Conversation

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

Uh oh!

What

Why ?

How ?

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 Dec 30, 2020

Uh oh!

dmitrygx commented Jan 1, 2021

Uh oh!

dmitrygx commented Jan 1, 2021

Uh oh!

azure-pipelines Bot commented Jan 1, 2021

Uh oh!

dmitrygx commented Jan 1, 2021

Uh oh!

dmitrygx commented Jan 2, 2021

Uh oh!

dmitrygx commented Jan 2, 2021

Uh oh!

azure-pipelines Bot commented Jan 2, 2021

Uh oh!

dmitrygx commented Jan 2, 2021

Uh oh!

Artemy-Mellanox Jan 3, 2021 • 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 commented Jan 4, 2021

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!

dmitrygx commented Jan 6, 2021

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dmitrygx commented Dec 29, 2020 •

edited

Loading

Artemy-Mellanox Jan 3, 2021 •

edited

Loading