UCP/AM: Drop AM data if rx ep is closed #5899

brminich · 2020-11-13T09:30:25Z

What

If single fragment AM received without UCP_AM_SEND_REPLY flag it
should be delivered to the user even if the corresponding rx ep is
closed/failed.
If single fragment AM received with UCP_AM_SEND_REPLY flag it
should be droppped if the corresponding rx ep is closed/failed,
because reply ep can not be provided in the data callback.
If some fragment of multi-fragmented AM received and the corresponding rx ep
is closed/failed, this fragment should be dropped regardless of send AM flags
(no way to assemble a message without aux info storedd in ep extension)
If AM RTS is received and the corresponding rx ep is closed/failed,
this RTS should be droppped and ATS with EP_TIMEOUT should be sent
back to the sender.

Why ?

Proper error handling

brminich · 2020-11-14T15:40:13Z

@hoopoepg, @dmitrygx, please review

hoopoepg · 2020-11-16T10:42:33Z

src/ucp/core/ucp_am.c

+    }
+
+    ep_ext      = ucp_ep_ext_proto(ep);
+


empty line?

remove the empty line or don't need alignment by =

remove the empty line or don't need the alignment by =

i'll remove the line

dmitrygx · 2020-11-16T11:51:00Z

src/ucp/core/ucp_am.c

+        return;
+    }
+
+    if (ucs_unlikely(!ucs_list_is_empty(&ep_ext->am.started_ams))) {


if I'm not mistaken we use likely/unlikely only at fast path

dmitrygx · 2020-11-16T11:53:39Z

src/ucp/core/ucp_am.c

@@ -1279,19 +1312,26 @@ UCS_PROFILE_FUNC(ucs_status_t, ucp_am_long_first_handler,
    size_t remaining;
    uint64_t recv_flags;

+    ep = ucp_worker_get_ep_by_id(worker, first_hdr->super.ep_id);


can move to declaration?

dmitrygx · 2020-11-16T11:54:22Z

src/ucp/core/ucp_am.c

+    }
+
+    ep_ext      = ucp_ep_ext_proto(ep);
+


remove the empty line or don't need alignment by =

dmitrygx · 2020-11-16T11:54:49Z

src/ucp/core/ucp_am.c

    void *hdr;

+    ep = ucp_worker_get_ep_by_id(worker, rts->super.sreq.ep_id);


can move to declaration?

dmitrygx · 2020-11-16T11:59:33Z

test/gtest/ucp/test_ucp_am.cc

+            receiver().progress();
+            if (m_am_received) {
+                request_wait(sreq);
+                UCS_TEST_SKIP_R("received all AMs before ep close");


dmitrygx · 2020-11-16T12:04:17Z

src/ucp/core/ucp_am.c

+    }
+
+    ep_ext      = ucp_ep_ext_proto(ep);
+


remove the empty line or don't need the alignment by =

dmitrygx · 2020-11-16T12:05:34Z

src/ucp/core/ucp_am.c

    void *hdr;

+    ep = ucp_worker_get_ep_by_id(worker, rts->super.sreq.ep_id);


can initialize along with declaration?

dmitrygx · 2020-11-16T12:05:45Z

src/ucp/core/ucp_am.c

@@ -1279,19 +1312,26 @@ UCS_PROFILE_FUNC(ucs_status_t, ucp_am_long_first_handler,
    size_t remaining;
    uint64_t recv_flags;

+    ep = ucp_worker_get_ep_by_id(worker, first_hdr->super.ep_id);


can initialize along with declaration?

dmitrygx · 2020-11-16T12:06:59Z

src/ucp/core/ucp_am.c

@@ -62,17 +62,36 @@ void ucp_am_ep_init(ucp_ep_h ep)
 void ucp_am_ep_cleanup(ucp_ep_h ep)
 {
    ucp_ep_ext_proto_t *ep_ext = ucp_ep_ext_proto(ep);
+    ucp_recv_desc_t *first_rdesc, *tmp, *mid_rdesc;


can have only:

ucp_recv_desc_t *rdesc, *tmp_rdesc;

and use rdesc and tmp_rdesc for first/middle fragments processing

dmitrygx · 2020-11-16T12:10:02Z

src/ucp/core/ucp_am.c

    ucs_status_t status;

+    if (ucs_unlikely(!ucp_am_ep_is_valid(ep))) {


btw, what if this is the last fragment received?
I guess, we could process it and report to a user, if it is not a REPLY proto.

imo, this would make code more complicated (need to process the message before dropping) while does not provide valuable benefit

brminich · 2020-11-18T06:24:24Z

@yosefe, please review

src/ucp/core/ucp_am.c

yosefe · 2020-11-18T11:50:04Z

src/ucp/core/ucp_am.c

+    return ((ep != NULL) &&
+            !(ep->flags & (UCP_EP_FLAG_CLOSED | UCP_EP_FLAG_FAILED)));


maybe remove ep from ptr map before closed? avoid branch
@evgeny-leksikov

do you mean directly from ucp_ep_close_nb and before request is completed?

IIRC we don't have any EPs wire exchanges which can't be dropped after EP started closing, so should work

should I do it here, or it is a separate PR?

src/ucp/core/ucp_am.c

yosefe · 2020-11-18T11:59:34Z

test/gtest/ucp/test_ucp_am.cc

+    {
+        test_ucp_am_nbx *self = reinterpret_cast<test_ucp_am_nbx*>(arg);
+        self->m_am_received   = true;
+


can remove space line

brminich · 2020-11-18T12:12:26Z

test/gtest/ucp/test_ucp_am.cc

+        }
+
+        EXPECT_EQ(rx_expected, m_am_received);
+


brminich · 2020-11-21T15:41:18Z

bot:pipe:retest

yosefe · 2020-11-22T08:54:56Z

src/ucp/core/ucp_am.c

@@ -26,6 +26,18 @@
 #define UCP_AM_SHORT_REPLY_MAX_SIZE  (UCS_ALLOCA_MAX_SIZE - \
                                      sizeof(ucs_ptr_map_key_t))

+#define UCP_WORKER_GET_EP_BY_ID(_worker, _ep_id, _str, _action) \
+    ({ \
+         ucp_ep_h __ep = ucp_worker_get_ep_by_id(_worker, _ep_id); \


why __ep and not _ep?

and i think ucp_worker_get_ep_by_id fails on assert if NULL?

no, it is ucs_assertv((ep == NULL) || (ep->worker == worker)

yosefe · 2020-11-22T08:56:38Z

src/ucp/core/ucp_am.c

+                          ((__ep)->flags & (UCP_EP_FLAG_CLOSED | \
+                                            UCP_EP_FLAG_FAILED)))) { \


remove from map during ep_close/fail

brminich · 2020-11-26T06:09:35Z

/azp run

azure-pipelines · 2020-11-26T06:09:46Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2020-11-26T09:49:27Z

src/ucp/core/ucp_worker.inl

+{
+    ucp_ep_h ep = ucp_worker_get_ep_by_id(worker, id);
+
+    if (ucs_likely(ep && !(ep->flags &


yosefe · 2020-11-26T09:50:58Z

src/ucp/core/ucp_am.c

             ucs_trace_data("worker %p: drop AM %s on closed/failed ep %p", \
-                            _worker, _str, __ep); \
+                            _worker, _str, _ep); \


change log message to remote "AM"

can you please clarify?

brminich · 2020-11-27T05:11:26Z

@dmitrygx, do you have other comments?

- If single fragment AM received without UCP_AM_SEND_REPLY flag it should be delivered to the user even if the corresponding rx ep is closed/failed. - If single fragment AM received with UCP_AM_SEND_REPLY flag it should be droppped if the corresponding rx ep is closed/failed, because reply ep can not be provided in the data callback. - If some fragment of multi-fragmented AM received and the corresponding rx ep is closed/failed, this fragment should be dropped regardless of send AM flags (no way to assemble a message without aux info storedd in ep extension) - If AM RTS is received and the corresponding rx ep is closed/failed, this RTS should be droppped and ATS with EP_TIMEOUT should be sent back to the sender.

brminich · 2020-11-30T12:33:29Z

bot:pipe:retest

brminich · 2020-12-01T06:42:11Z

#5337 and #5765

bot:pipe:retest

brminich · 2020-12-01T12:56:20Z

bot:pipe:retest

brminich · 2020-12-02T07:15:57Z

issue is #5978

bot:pipe:retest

brminich · 2020-12-03T08:25:25Z

bot:pipe:retest

brminich · 2020-12-03T15:08:05Z

bot:pipe:retest

hoopoepg approved these changes Nov 16, 2020

View reviewed changes

dmitrygx suggested changes Nov 16, 2020

View reviewed changes

yosefe reviewed Nov 18, 2020

View reviewed changes

brminich commented Nov 18, 2020

View reviewed changes

yosefe reviewed Nov 22, 2020

View reviewed changes

yosefe reviewed Nov 26, 2020

View reviewed changes

yosefe approved these changes Nov 26, 2020

View reviewed changes

dmitrygx approved these changes Nov 27, 2020

View reviewed changes

brminich force-pushed the topic/ucp_am_drop_on_closed_ep branch from 4ec26d1 to ee224c5 Compare November 27, 2020 11:06

brminich merged commit 18fca96 into openucx:master Dec 4, 2020

it-is-a-robot mentioned this pull request Nov 30, 2021

Huawei 1230 kunpengcompute/hucx#49

Closed

		void *hdr;

		ep = ucp_worker_get_ep_by_id(worker, rts->super.sreq.ep_id);

		ucs_status_t status;

		if (ucs_unlikely(!ucp_am_ep_is_valid(ep))) {

		return ((ep != NULL) &&
		!(ep->flags & (UCP_EP_FLAG_CLOSED \| UCP_EP_FLAG_FAILED)));

		((__ep)->flags & (UCP_EP_FLAG_CLOSED \| \
		UCP_EP_FLAG_FAILED)))) { \

UCP/AM: Drop AM data if rx ep is closed #5899

UCP/AM: Drop AM data if rx ep is closed #5899

Conversation

brminich commented Nov 13, 2020

What

Why ?

brminich commented Nov 14, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brminich commented Nov 18, 2020

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

brminich commented Nov 21, 2020

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

brminich commented Nov 26, 2020

azure-pipelines bot commented Nov 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brminich commented Nov 27, 2020

brminich commented Nov 30, 2020

brminich commented Dec 1, 2020

brminich commented Dec 1, 2020

brminich commented Dec 2, 2020

brminich commented Dec 3, 2020

brminich commented Dec 3, 2020