UCP/RNDV: Refactor rendezvous protocol to use rma_bw lanes. #2092

yosefe · 2017-12-19T16:43:55Z

No description provided.

jenkinsornl · 2017-12-19T17:34:18Z

Build finished.

mellanox-github · 2017-12-19T19:06:48Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/5559/ for details (Mellanox internal link).

hoopoepg · 2017-12-20T09:42:46Z

src/ucp/core/ucp_rkey.c

@@ -34,6 +34,26 @@ size_t ucp_rkey_packed_size(ucp_context_h context, ucp_md_map_t md_map)
    return size;
 }

+void ucp_rkey_packed_copy(ucp_context_h context, ucp_md_map_t md_map,
+                          void *rkey_buffer, const void* uct_rkeys[])


const void* uct_rkeys[] is interpreted as void**, as I can see incoming parameter is just non-initialized array, not array of pointers (and coverity agree with me :) )

it should be a const array of pointers

yes, I see
the simplest way to suppress error (without hack with suppress coverity by comment) is create separate array of pointers, like

const void *array[2] = {key_buf, NULL};

and pass it to this function (technically compiler makes same things, should not affect performane)

hoopoepg · 2017-12-20T09:45:07Z

src/ucp/core/ucp_rkey.c

+
+         rkey_buffer += md_size;
+
+         snprintf(p, endp - p, md_map ? "," : "");


md_map is always NOT NULL here (coverity report)

hoopoepg · 2017-12-20T12:05:01Z

src/ucp/core/ucp_rkey.c

    ucs_for_each_bit(md_index, md_map) {
         md_size      = *((uint8_t*)rkey_buffer);
         rkey_buffer += sizeof(uint8_t);

+         if (!first) {
+             snprintf(p, endp - p, ",");


why not p+= snprintf?

https://linux.die.net/man/3/snprintf
The functions snprintf() and vsnprintf() do not write more than size bytes (including the terminating null byte ('\0')). If the output was truncated due to this limit then the return value is the number of characters (excluding the terminating null byte) which would have been written to the final string if enough space had been available. Thus, a return value of size or more means that the output was truncated. (See also below under NOTES.)

jenkinsornl · 2017-12-20T12:48:53Z

Build finished.

swx-jenkins1 · 2017-12-20T12:52:32Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/3423/ for details.

mellanox-github · 2017-12-20T14:42:59Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/5563/ for details (Mellanox internal link).

yosefe · 2017-12-20T15:32:10Z

@hoopoepg @brminich pls take a look

brminich · 2017-12-20T19:22:05Z

src/ucp/core/ucp_request.h

-                    uct_rkey_bundle_t rkey_bundle;
-                    ucp_request_t *sreq;          /* send request on the send side */
+                    uint64_t         remote_address; /* address of the receiver's data buffer */
+                    uintptr_t        remote_request; /* pointer to the receiver's send request */


it is receiver's recv request

brminich · 2017-12-20T20:36:35Z

src/ucp/tag/offload.c

-    ucp_lane_index_t lane = ucp_ep_get_tag_lane(ep);
+    ucp_ep_t *ep = sreq->send.ep;
+
+    ucs_assert(sreq->send.lane == ucp_ep_get_tag_lane(ep));


where do you set a send.lane?

will add comment

brminich · 2017-12-20T21:26:03Z

src/ucp/tag/rndv.h

@@ -28,7 +23,6 @@ typedef struct {
    ucp_request_hdr_t         sreq;     /* send request on the rndv initiator side */
    uint64_t                  address;  /* holds the address of the data buffer on the sender's side */
    size_t                    size;     /* size of the data for sending */
-    uint16_t                  flags;


How do we check on receiver whether we have rkeys or not now?

according to md_map in the packed rkey

but we send no rkey in case of SW rndv, non-contig RNDV, etc

in this case, we can tell it by address==NULL

BTW, for SW rndv, we do send rkeys (if the send buffer is contig but large, for example)

aa, right, we check the address. Yes, SW rndv may carry the rkey for large messages, but in other cases it won't.

brminich · 2017-12-20T21:28:11Z

src/ucp/core/ucp_rkey.c

+        ucs_assert_always(md_size <= UINT8_MAX);
+        *((uint8_t*)p++) = md_size;
+        memcpy(p, *uct_rkeys, md_size);
+        p += sizeof(uint8_t) + md_size;


seems like need to do p += md_size, because you already made p++ before

right, it worked because offload currently has only 1 key

…factor

swx-jenkins1 · 2017-12-21T01:45:23Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/3435/ for details.

jenkinsornl · 2017-12-21T04:54:34Z

Build finished.

mellanox-github · 2017-12-21T04:59:12Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/5576/ for details (Mellanox internal link).

jenkinsornl · 2017-12-21T05:45:41Z

Build finished.

mellanox-github · 2017-12-21T05:48:38Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/5577/ for details (Mellanox internal link).

brminich · 2017-12-21T06:30:47Z

src/ucp/tag/rndv.c

+            }
+        }
+
+        sreq->send.lane     = ucp_ep_get_am_lane(ep);


looks like no need to set lane here, can do assert that it is AM lane (for consistency with offload branch)

but here we set to am_lane, and in offload.c we expect tag_lane

I mean that request lane is set in ucp_tag_send_req_init(). No need to set it here again.

but ucp_tag_send_req_init() always sets it to tag_lane, and in case of rndv we may need to switch to am_lane.. do you think they would have be the same in this case?

if (!ep->flags & UCP_EP_FLAG_TAG_OFFLOAD_ENABLED), tag_lane should be am_lane

ucx/src/ucp/core/ucp_ep.c

Line 831 in bc72a8f

config->tag.lane = lane;

yosefe · 2017-12-21T07:53:36Z

bot:retest

jenkinsornl · 2017-12-21T10:27:20Z

Build finished.

mellanox-github · 2017-12-21T11:39:00Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/5582/ for details (Mellanox internal link).

swx-jenkins1 · 2017-12-21T15:41:56Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/3445/ for details.

jenkinsornl · 2017-12-21T15:55:28Z

Build finished.

yosefe · 2017-12-21T16:44:48Z

bot:bgate:retest

swx-jenkins1 · 2017-12-21T16:46:51Z

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/3451/ for details.

swx-jenkins1 · 2017-12-21T17:54:01Z

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/3453/ for details.

mellanox-github · 2017-12-21T18:07:20Z

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/5588/ for details (Mellanox internal link).

mellanox-github · 2017-12-21T20:47:39Z

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/5591/ for details (Mellanox internal link).

UCP/RNDV: Refactor rendezvous protocol to use rma_bw lanes.

7f63a11

yosefe added the Cleanup label Dec 19, 2017

hoopoepg reviewed Dec 20, 2017

View reviewed changes

UCP/RKEY/RNDV: Fix coverity errors.

e2f475a

hoopoepg reviewed Dec 20, 2017

View reviewed changes

brminich reviewed Dec 20, 2017

View reviewed changes

yosefe added 2 commits December 21, 2017 03:13

UCP/RKEY/TAG: Fix code review comments.

64f509f

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

64157ad

…factor

brminich reviewed Dec 21, 2017

View reviewed changes

brminich approved these changes Dec 21, 2017

View reviewed changes

UCP/RNDV: When tag offload is disabled, tag_lane == am_lane.

94c7f2a

yosefe merged commit 5b1aeb3 into openucx:master Dec 21, 2017

yosefe deleted the topic/ucp-rndv-refactor branch December 21, 2017 22:16


		rkey_buffer += md_size;

		snprintf(p, endp - p, md_map ? "," : "");

UCP/RNDV: Refactor rendezvous protocol to use rma_bw lanes. #2092

UCP/RNDV: Refactor rendezvous protocol to use rma_bw lanes. #2092

Conversation

yosefe commented Dec 19, 2017

jenkinsornl commented Dec 19, 2017

mellanox-github commented Dec 19, 2017

hoopoepg Dec 20, 2017 • edited Loading

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

jenkinsornl commented Dec 20, 2017

swx-jenkins1 commented Dec 20, 2017

mellanox-github commented Dec 20, 2017

yosefe commented Dec 20, 2017

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

swx-jenkins1 commented Dec 21, 2017

jenkinsornl commented Dec 21, 2017

mellanox-github commented Dec 21, 2017

jenkinsornl commented Dec 21, 2017

mellanox-github commented Dec 21, 2017

Choose a reason for hiding this comment

yosefe Dec 21, 2017 • edited Loading

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

yosefe commented Dec 21, 2017

jenkinsornl commented Dec 21, 2017

mellanox-github commented Dec 21, 2017

swx-jenkins1 commented Dec 21, 2017

jenkinsornl commented Dec 21, 2017

yosefe commented Dec 21, 2017

swx-jenkins1 commented Dec 21, 2017

swx-jenkins1 commented Dec 21, 2017

mellanox-github commented Dec 21, 2017

mellanox-github commented Dec 21, 2017

hoopoepg Dec 20, 2017 •

edited

Loading

yosefe Dec 21, 2017 •

edited

Loading