UCP/GTEST: fix and test rndv requests leak from the ptr_map. by alinask · Pull Request #5769 · openucx/ucx

alinask · 2020-10-05T08:59:28Z

No description provided.

alinask · 2020-10-05T10:11:32Z

@evgeny-leksikov @brminich Can you please review?

brminich · 2020-10-05T11:22:34Z


 UCS_TEST_P(test_ucp_sockaddr_protocols, tag_rndv_exp, "RNDV_THRESH=10k")
 {
+    /* run send_recv twice so that the rndv threshold would be used */


what does it mean?

when the rndv_thresh is set, it's not used for the first message, so even if the message is larger than the set rndv_thresh, rndv will not be selected. it will be selected on the second message.

maybe send smaller message as a first one?

@brminich why?

maybe send smaller message as a first one?

@brminich initially this test was added to check that first large message works correctly

what is meant by correctly? That it is correctly sent, even though using eager protocol?

yes, aka ep/request re-configuration issue due to protocol mismatch

@evgeny-leksikov the 'first large message' will be sent with rndv if it's not the first message sent (before the connection was established). so sending a small message first shouldn't contradict what you are saying.

AFAIU this is exactly before wireup is finished case

let's change "test_tag_send_recv" to run several iterations (e.g 10) and add this comment there:
/* send multiple messages to test the protocol both before and after connection establishment */

brminich · 2020-10-05T11:25:25Z

-                    ucs_ptr_map_key_t req_id;         /* the send request ID on receiver side */
+                    ucs_ptr_map_key_t sreq_id;        /* the send request ID on receiver side */
                    ucp_request_t     *rreq;          /* pointer to the receive request */
+                    ucs_ptr_map_key_t rreq_id;        /* the receive request ID on receiver side */


do we need both rreq and rreq_id now?

since there is room for both, we thought to leave it there for now to avoid a look-up in the ptr_map.

Is there any chance to access the pointer when rreq is released? let's say it's canceled on receiver side but sender is sending ACK.
IMO it looks strange I would keep something one and ID is preferrable

if the recv side tries to remove an id of a non-existing rreq then the extract function would return NO_ELEM, isn't that what is expected?
or you mean to say we should remove the rreq from the map when it's canceled?

yes, we have to remove id from map before it's put to the pool or in worse case will be reused by next op

yes, my concern about rreq pointer

maybe we should add rreq_id as a filed in the receive request itself (in the 'recv' part)
IMO it makes more sense each request will hold its own id

alinask · 2020-10-07T13:41:51Z

@evgeny-leksikov @brminich @yosefe Can you please review again?

alinask · 2020-10-07T17:30:57Z

failure on #3897

evgeny-leksikov · 2020-10-08T08:01:54Z

    UCS_PROFILE_REQUEST_EVENT(req, multi_func_str, req->send.length);
 }

+static void ucp_request_get_id(ucp_request_t *req)


maybe ucp_send_request_init_id ?

@evgeny-leksikov I renamed. please take a look.

alinask · 2020-10-08T16:51:43Z

failure on #4616

yosefe · 2020-10-11T12:42:27Z

+
+                    ucs_ptr_map_key_t      sreq_id;     /* send request ID on the
+                                                           sender side */


add it before rreq_id

yosefe · 2020-10-11T12:54:31Z

-        std::string recv_buf(size, 'y');
+        /* send multiple messages to test the protocol both before and after
+         * connection establishment */
+        for (int i = 0; i < 10; i++) {


yosefe

pls squash

yosefe · 2020-10-11T19:33:28Z

seems relevant
IMO it's because we delete send request id when receiving RTR, and not right before completing the send request
https://dev.azure.com/ucfconsort/ucx/_build/results?buildId=10002&view=logs&jobId=73bd2a9e-6688-5c22-d122-7b1a0b6cbb81&j=73bd2a9e-6688-5c22-d122-7b1a0b6cbb81&t=cd3d3953-51ff-5d5c-c5ca-2864f3c1d670

2020-10-11T16:05:17.4336436Z [swx-rdmz-ucx-gpu-01:13731:0:13731] ucp_worker.inl:93   Assertion `status == UCS_OK' failed
2020-10-11T16:05:17.9239136Z 
2020-10-11T16:05:17.9242320Z /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucp/core/ucp_worker.inl: [ ucp_worker_del_request_id() ]
2020-10-11T16:05:17.9243701Z       ...
2020-10-11T16:05:17.9244575Z        90 
2020-10-11T16:05:17.9245861Z        91     status = ucs_ptr_map_del(&worker->ptr_map, id);
2020-10-11T16:05:17.9247692Z        92     ucs_assert(status == UCS_OK);
2020-10-11T16:05:17.9248635Z ==>    93 }
2020-10-11T16:05:17.9249447Z        94 
2020-10-11T16:05:17.9250476Z        95 static UCS_F_ALWAYS_INLINE ucp_request_t*
2020-10-11T16:05:17.9251630Z        96 ucp_worker_extract_request_by_id(ucp_worker_h worker, ucs_ptr_map_key_t id)
2020-10-11T16:05:17.9252602Z 
2020-10-11T16:05:18.4153688Z ==== backtrace (tid:  13731) ====
2020-10-11T16:05:18.4155672Z  0 0x000000000005c098 ucs_debug_print_backtrace()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucs/debug/debug.c:656
2020-10-11T16:05:18.4157227Z  1 0x0000000000057c7b ucp_worker_del_request_id()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucp/core/ucp_worker.inl:93
2020-10-11T16:05:18.4158659Z  2 0x0000000000057c7b ucs_ptr_map_del()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucs/datastruct/ptr_map.inl:159
2020-10-11T16:05:18.4160160Z  3 0x0000000000057c7b ucp_worker_del_request_id()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucp/core/ucp_worker.inl:92
2020-10-11T16:05:18.4161615Z  4 0x0000000000057c7b ucp_rndv_rtr_handler_inner()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucp/rndv/rndv.c:1677
2020-10-11T16:05:18.4162960Z  5 0x0000000000057c7b ucp_rndv_rtr_handler()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/ucp/rndv/rndv.c:1654
2020-10-11T16:05:18.4164339Z  6 0x000000000006da12 uct_iface_invoke_am()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/uct/base/uct_iface.h:650
2020-10-11T16:05:18.4165796Z  7 0x000000000006da12 uct_rc_mlx5_iface_common_am_handler()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/uct/ib/rc/accel/rc_mlx5.inl:428
2020-10-11T16:05:18.4167298Z  8 0x000000000006da12 uct_rc_mlx5_iface_common_poll_rx()  /scrap/azure/agent-06/AZP_WORKSPACE/2/s/contrib/../src/uct/ib/rc/accel/rc_mlx5.inl:143

evgeny-leksikov · 2020-10-12T15:47:17Z

    void *value;

    if (ucs_likely(!(key & UCS_PTR_MAP_KEY_INDIRECT_FLAG))) {
+        ucs_assert(key != 0);


why? NULL is also pointer, kind of...
if this happens unexpectedly, IMO, it should be fixed/asserted in UCP

why would you want to put a null pointer into the ptr_map..?

IMO NULL pointer is incorrect usage (we expect an id to resolve to a valid object), and ptr_map can help by catching this early

can add assert when adding element

I don't :) but IMHO it should not be restricted in UCS
anyway let's do it symmetric - if assert then for both add/del

evgeny-leksikov · 2020-10-12T15:48:47Z

+    if (ucp_ep_use_indirect_id(ep)) {
+        ucp_worker_del_request_id(sreq->send.ep->worker, sreq->send.msg_proto.sreq_id);
+    }


according to assert in UCS, sreq_id == NULL here? if yes, is it expected by protocol?

when tag_offload is used, the msg_proto struct should not be access here, but rather the tag_offload struct (which doesn't have the sreq_id field).
but since tag_offload doesn't support error handling, the del_req_id function "won't do anything" anyway so it should be ok not to call it in this case.

so it should be in

if (sreq->flags & UCP_REQUEST_FLAG_OFFLOADED) { ... } else { ucp_worker_del_request_id(sreq->send.ep->worker, sreq->send.msg_proto.sreq_id); }

?

checking ucp_ep_use_indirect_id(ep) makes the del_req_id call dependant on err_handling support rather than the used protocol (when checking the sreq->flags)

but union in sreq->send (to access sreq->send.msg_proto.sreq_id) is protocol specific but not err_handleng mode

yes but, adding and removing from the ptr_map is err_handling dependant.

ucp_worker_del_request_id already should handle err handling inside by indirect flag presence, if sreq_id is not valid by some other logic - it should be handled accordingly

i'm fine with both ways.
@yosefe wdyt?

for now let's check for NULL in ptr_map, it's usefult for TCP ep's as well and document it
if we'll have use case where map should hold NULL can move the check to upper layers

what do you mean? we can't access msg_proto here for tag_offload

yosefe · 2020-10-12T22:18:24Z

/azp run

azure-pipelines · 2020-10-12T22:18:38Z

Azure Pipelines successfully started running 1 pipeline(s).

…r-map-fix

yosefe · 2020-10-17T11:27:32Z

/azp run

azure-pipelines · 2020-10-17T11:27:45Z

Azure Pipelines successfully started running 1 pipeline(s).

dmitrygx · 2020-11-27T23:00:24Z

-        std::string recv_buf(size, 'y');
+        /* send multiple messages to test the protocol both before and after
+         * connection establishment */
+        for (int i = 0; i < 100; i++) {


@alinask @yosefe do we want the same logic for RMA and AM (to test RNDV)?

I have a dependency on this change. @alinask what about creating a separate PR with sockaddr-only change to test before/after connection establishment? If you wish, I could help with that

@dmitrygx which PR is dependent on this?
if you add only this change, of a loop around this code, it will fail on the ptr_map leak which this PR is fixing (and this change is reproducing).

@dmitrygx which PR is dependent on this?
if you add only this change, of a loop around this code, it will fail on the ptr_map leak which this PR is fixing (and this change is reproducing).

I see
yes, #5821 depends on the tests, but with error handling

alinask · 2020-12-14T19:17:56Z

Replaced with #6006

brminich reviewed Oct 5, 2020

View reviewed changes

evgeny-leksikov reviewed Oct 8, 2020

View reviewed changes

evgeny-leksikov approved these changes Oct 8, 2020

View reviewed changes

brminich approved these changes Oct 8, 2020

View reviewed changes

yosefe reviewed Oct 11, 2020

View reviewed changes

yosefe approved these changes Oct 11, 2020

View reviewed changes

UCP/GTEST: fix and test ucp requests leak from the ptr_map.

7d1cbe6

alinask force-pushed the topic/ucp-rndv-ptr-map-fix branch from 3a4362d to 7d1cbe6 Compare October 11, 2020 14:47

UCP: remove sreq_id from the ptr_map only if the ptr is indirect

a8a3db6

evgeny-leksikov reviewed Oct 12, 2020

View reviewed changes

alinask added 2 commits October 13, 2020 15:36

Merge remote-tracking branch 'upstream/master' into topic/ucp-rndv-pt…

91c8e11

…r-map-fix

UCP/UCS: ucp request leak in ptr_map - apply CR comments.

284df93

dmitrygx reviewed Nov 27, 2020

View reviewed changes

alinask closed this Dec 14, 2020

svc-ucx mentioned this pull request Aug 26, 2026

UCT/CUDA_COPY: Fix reachability between interfaces #11783

Open


		ucs_ptr_map_key_t sreq_id; /* send request ID on the
		sender side */

Conversation

alinask commented Oct 5, 2020

Uh oh!

alinask commented Oct 5, 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!

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!

Choose a reason for hiding this comment

Uh oh!

alinask commented Oct 7, 2020

Uh oh!

alinask commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alinask commented Oct 8, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosefe left a comment

Choose a reason for hiding this comment

Uh oh!

yosefe commented Oct 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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

yosefe commented Oct 11, 2020 •

edited

Loading

alinask commented Dec 14, 2020 •

edited

Loading