UCP/CORE: Drop packets with invalid REQ or UCP EP IDs #6001

dmitrygx · 2020-12-09T09:52:45Z

What

Drop packets with invalid REQ or UCP EP IDs

Why ?

Just drop packets instead of asserting that we expect that requests or EPs are found by their ids.

How ?

Add additional if to check the result from ucp_worker_get_ep_by_id() and ucp_worker_get_request_by_id(). If it unsuccessful to find a request or an EP, just drop a packet.
Add gtests to reproduce an invalid EP or request ID handling during peer failure flow.

dmitrygx · 2020-12-15T12:56:20Z

failure was #6029
bot:pipe:retest

dmitrygx · 2020-12-16T07:31:54Z

tests are passing, unrelated change was rolled back.
@hoopoepg @brminich could you review pls?

dmitrygx · 2020-12-16T08:48:47Z

/azp run

azure-pipelines · 2020-12-16T08:48:59Z

Azure Pipelines successfully started running 1 pipeline(s).

src/ucp/core/ucp_request.inl

src/ucp/proto/proto_am.inl

src/ucp/rma/amo_sw.c

src/ucp/rma/rma_sw.c

src/ucp/rndv/rndv.c

src/ucp/tag/eager_snd.c

test/gtest/ucp/test_ucp_sockaddr.cc

dmitrygx · 2020-12-17T14:33:25Z

botLpipe:retest

dmitrygx · 2020-12-17T19:46:56Z

botLpipe:retest

dmitrygx · 2020-12-18T07:07:05Z

bot:pipe:retest

dmitrygx · 2020-12-18T07:07:22Z

@brminich @hoopoepg could you review pls?

src/ucp/core/ucp_am.c

src/ucp/proto/proto_am.inl

src/ucp/rma/amo_sw.c

src/ucp/rma/rma_sw.c

src/ucp/tag/eager_snd.c

src/ucp/wireup/wireup.c

test/gtest/ucp/test_ucp_sockaddr.cc

dmitrygx · 2020-12-18T13:22:10Z

@brminich @yosefe I'd keep the behavior as is (except dropping packets if EP doesn't exist anymore in PTR MAP), since there are many tests (e.g. test_ucp_wireup_1sided.send_disconnect_reply1) which wait for a sender's operation completion after receiver's ep issued ucp_ep_close_nbx(). e.g. if this is the SW PUT implementation, a target side receives PUT_REQ and checks whether EP exists or not and then copies data to its buffer and answers with completion to PUT operation. But if we will check EP state (CLOSED or not), we will not answer with completion. So, an initiator of PUT will hang and operation will be timed out, since no completion was received and no errors were detected (since those tests don't request error handling support).
I think we should handle this behavior, we should wait for #5821 and enable error handling in those tests.

dmitrygx · 2020-12-18T17:28:51Z

failure was #6011
bot:pipe:retest

dmitrygx · 2020-12-18T22:56:06Z

bot:pipe:retest

dmitrygx · 2020-12-18T22:56:13Z

/azp run

azure-pipelines · 2020-12-18T22:56:24Z

Azure Pipelines successfully started running 1 pipeline(s).

yosefe · 2020-12-19T12:10:15Z

bot:pipe:retest

dmitrygx · 2020-12-30T16:40:52Z

@brminich failure is not related, could you review pls?

dmitrygx · 2020-12-30T20:40:19Z

/azp run

azure-pipelines · 2020-12-30T20:40:31Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2021-01-04T08:52:01Z

@yosefe @hoopoepg could you review pls?

src/ucp/core/ucp_ep.inl

src/ucp/tag/eager_snd.c

src/ucp/core/ucp_ep.inl

src/ucp/core/ucp_worker.inl

src/ucp/rndv/rndv.c

yosefe · 2021-01-04T09:01:32Z

test/gtest/ucp/test_ucp_sockaddr.cc

-                rreq = ucp_tag_recv_nb(receiver().worker(), &recv_buf[0], size,
-                                       ucp_dt_make_contig(1), 0, 0,
-                                       rtag_complete_cb);
+                ucp_tag_recv_info_t recv_info = {};


put this is separate function

yosefe · 2021-01-04T09:02:08Z

test/gtest/ucp/test_ucp_sockaddr.cc

+            if (!err_handling) {
+                request_wait(sreq);
+                request_wait(rreq);


make std::vector of the requests we need to wait for

yosefe · 2021-01-04T09:03:23Z

test/gtest/ucp/test_ucp_sockaddr.cc

+            if (!err_handling) {
+                compare_buffers(send_buf, recv_buf);
+            } else {
+                delete slh;


use ucs::auto_ptr<scoped_log_handler>

yosefe · 2021-01-04T09:04:07Z

test/gtest/ucp/test_ucp_sockaddr.cc

+                                                        (get_variant_value() &
+                                                         SEND_STOP) ?
+                                                        send_stop : NULL,
+                                                        (get_variant_value() &
+                                                         RECV_STOP) ?
+                                                        recv_stop : NULL,


can we check the variant inside test_tag_send_recv?

we can't since this is the function from the base class
but we need to check variants and apply them only warmup

any other way to simplify this code?

fixed as you suggested, but it was required to move functions to test_ucp_sockaddr_protocols class

yosefe · 2021-01-04T09:04:38Z

test/gtest/ucp/test_ucp_sockaddr.cc

+                (err_str.find("ptr map ") != std::string::npos) ||
+                (err_str.find("was not returned to mpool ucp_requests") !=


why are we OK to have leaks?

yes, it will be fixed then by the tracking UCP requests
it is ok, since we close EP for send, we don't expect that recv request won't be completed and vice versa

can we force-release the leaked request somehow?

unfortunately, it could be problematic, since we need to fix C++ compilation of ucp_request.inl file to use ucp_request_put() function for force-release of UCP requests and more complex thing is to release PTR map key since it is needed in RNDV, synchronized protocols
tracking UCP requests will allow us just remove the warn_leak_hander() and we would expect that no warnings/errors are printed by UCP

can we force-release the leaked request somehow?

@yosefe done by forcibly setting COMPLETED flag and releasing a request ID if a request is in PTR MAP.

dmitrygx · 2021-01-07T08:41:31Z

@yosefe could you review pls?

yosefe · 2021-01-07T18:46:26Z

@dmitrygx is it possible to remove the merge commit(496192b) and squash the fixes (starting from 94bbe10) to one commit?
it's not easy to see what has changed since last review (bd9ece1)

dmitrygx · 2021-01-07T19:26:42Z

@dmitrygx is it possible to remove the merge commit(496192b) and squash the fixes (starting from 94bbe10) to one commit?
it's not easy to see what has changed since last review (bd9ece1)

@yosefe hope it is what you mean
removed the merge commit, but rebased on master due to conflicts
pls review the last commit af2d0b1 - it has fixes for the latest comments

dmitrygx · 2021-01-07T22:27:34Z

bot:pipe:retest

dmitrygx · 2021-01-10T09:34:47Z

@yosefe could you review pls?

src/ucp/core/ucp_am.c

src/ucp/core/ucp_ep.inl

src/ucp/core/ucp_worker.inl

src/ucp/rma/amo_sw.c

src/ucp/rndv/rndv.c

test/gtest/ucp/test_ucp_sockaddr.cc

test/gtest/ucp/ucp_test.cc

test/gtest/ucp/test_ucp_sockaddr.cc

src/ucp/core/ucp_am.c

src/ucp/rndv/rndv.c

dmitrygx · 2021-01-11T15:55:48Z

@brminich @yosefe are you ok with the latest changes? there is merge-conflict, I guess better to fix it when squashing

yosefe · 2021-01-11T15:56:22Z

@brminich @yosefe are you ok with the latest changes? there is merge-conflict, I guess better to fix it when squashing

need to fix it to run tests

dmitrygx · 2021-01-11T16:04:22Z

@brminich @yosefe are you ok with the latest changes? there is merge-conflict, I guess better to fix it when squashing

need to fix it to run tests

@yosefe done

dmitrygx · 2021-01-11T16:34:36Z

static checkers found errors:
Error: CPPCHECK_WARNING:
/src/ucm/util/reloc.c:71: error[unknownMacro]: There is an unknown macro here somewhere. Configuration is required. If ElfW is a macro then please configure it.
##[error]static checkers found errors

the error is unrelated and will be fixed by @gleon99 in #6112

dmitrygx · 2021-01-12T07:26:13Z

@yosefe could you review pls? failures are unrelated

dmitrygx · 2021-01-12T12:13:38Z

/azp run

azure-pipelines · 2021-01-12T12:13:51Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2021-01-13T09:12:26Z

@yosefe ok to merge?

dmitrygx force-pushed the topic/ucp/drop_by_id branch from f025184 to 570373b Compare December 9, 2020 19:37

dmitrygx mentioned this pull request Dec 11, 2020

UCS/MPOOL: Make elem defined for Valgrind #6017

Merged

dmitrygx force-pushed the topic/ucp/drop_by_id branch 2 times, most recently from b1e6aaa to d284266 Compare December 15, 2020 11:22

dmitrygx force-pushed the topic/ucp/drop_by_id branch 5 times, most recently from a304d9f to ec740b3 Compare December 16, 2020 07:27

brminich reviewed Dec 16, 2020

View reviewed changes

brminich reviewed Dec 18, 2020

View reviewed changes

hoopoepg reviewed Jan 4, 2021

View reviewed changes

src/ucp/core/ucp_ep.inl Outdated Show resolved Hide resolved

src/ucp/tag/eager_snd.c Outdated Show resolved Hide resolved

yosefe requested changes Jan 4, 2021

View reviewed changes

dmitrygx force-pushed the topic/ucp/drop_by_id branch from 64c4221 to af2d0b1 Compare January 7, 2021 19:25

yosefe reviewed Jan 10, 2021

View reviewed changes

yosefe reviewed Jan 11, 2021

View reviewed changes

brminich reviewed Jan 11, 2021

View reviewed changes

src/ucp/core/ucp_am.c Outdated Show resolved Hide resolved

src/ucp/rndv/rndv.c Outdated Show resolved Hide resolved

src/ucp/rndv/rndv.c Outdated Show resolved Hide resolved

brminich approved these changes Jan 11, 2021

View reviewed changes

yosefe approved these changes Jan 12, 2021

View reviewed changes

dmitrygx force-pushed the topic/ucp/drop_by_id branch from f4c4d00 to 991ace1 Compare January 12, 2021 10:27

yosefe approved these changes Jan 12, 2021

View reviewed changes

UCP/CORE: Drop packets with invalid REQ or UCP EP IDs

a5f9ea7

dmitrygx force-pushed the topic/ucp/drop_by_id branch from 991ace1 to a5f9ea7 Compare January 12, 2021 23:52

yosefe approved these changes Jan 13, 2021

View reviewed changes

yosefe merged commit 454c60b into openucx:master Jan 13, 2021

dmitrygx mentioned this pull request Jan 19, 2021

UCP/PROTO: Handle AM short failure correctly #6157

Merged

pentschev mentioned this pull request Jan 19, 2021

UCX 1.10 issues rapidsai/ucx-py#668

Closed

dmitrygx mentioned this pull request Jan 25, 2021

UCP/CORE/RNDV/GTEST: Drop packets with invalid ID and fix handling of status in RNDV RTS/RTR/data [v1.10.x] #6187

Merged

		(err_str.find("ptr map ") != std::string::npos) \|\|
		(err_str.find("was not returned to mpool ucp_requests") !=

UCP/CORE: Drop packets with invalid REQ or UCP EP IDs #6001

UCP/CORE: Drop packets with invalid REQ or UCP EP IDs #6001

Conversation

dmitrygx commented Dec 9, 2020

What

Why ?

How ?

dmitrygx commented Dec 15, 2020

dmitrygx commented Dec 16, 2020

dmitrygx commented Dec 16, 2020

azure-pipelines bot commented Dec 16, 2020

dmitrygx commented Dec 17, 2020

dmitrygx commented Dec 17, 2020

dmitrygx commented Dec 18, 2020

dmitrygx commented Dec 18, 2020

dmitrygx commented Dec 18, 2020 • edited Loading

dmitrygx commented Dec 18, 2020

dmitrygx commented Dec 18, 2020

dmitrygx commented Dec 18, 2020

azure-pipelines bot commented Dec 18, 2020

yosefe commented Dec 19, 2020

dmitrygx commented Dec 30, 2020

dmitrygx commented Dec 30, 2020

azure-pipelines bot commented Dec 30, 2020

dmitrygx commented Jan 4, 2021

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

dmitrygx commented Jan 7, 2021

yosefe commented Jan 7, 2021

dmitrygx commented Jan 7, 2021

dmitrygx commented Jan 7, 2021

dmitrygx commented Jan 10, 2021

dmitrygx commented Jan 11, 2021

yosefe commented Jan 11, 2021

dmitrygx commented Jan 11, 2021

dmitrygx commented Jan 11, 2021

dmitrygx commented Jan 12, 2021

dmitrygx commented Jan 12, 2021

azure-pipelines bot commented Jan 12, 2021

dmitrygx commented Jan 13, 2021

dmitrygx commented Dec 18, 2020 •

edited

Loading