UCP:rndv multirail support infrastructure + protocol #1894

hoopoepg · 2017-10-06T14:20:13Z

added infrastructure to support multi-rail on rndv-RMA:
- updated rndv-rma lane to array
- added memory handles to store registered buffer and remote keys
- added wireup configuration for mrail
- added context parameter to enable mrail (disabled by default) UCX_RNDV_RAILS=1

swx-jenkins1 · 2017-10-06T15:02:36Z

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

mellanox-github · 2017-10-06T15:46:05Z

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

hoopoepg · 2017-10-06T15:48:07Z

bot:mlx:retest

mellanox-github · 2017-10-06T17:16:07Z

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

swx-jenkins1 · 2017-10-06T18:04:45Z

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

mellanox-github · 2017-10-06T18:48:51Z

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

evgeny-leksikov

I would add ucp_request_send* prefix to all corresponding functions which work with request.send structure and ucp_request_[send]_state* to the functions which work with the state ( #1874 ). BTW, do we plan any mrail infra on recv (request.recv)? it potentially can be needed in case multi protocols over rails.

hoopoepg · 2017-10-07T07:52:22Z

I would add ucp_request_send* prefix to all corresponding functions which work with request.send structure and ucp_request_[send]_state* to the functions which work with the state ( #1874 ).

ok, I will rebase changes to your update

BTW, do we plan any mrail infra on recv (request.recv)? it potentially can be needed in case multi protocols over rails.

may be later

brminich · 2017-10-06T16:19:08Z

src/ucp/core/ucp_ep.inl

+static inline int ucp_ep_is_rndv_mrail_present(ucp_ep_h ep)
+{
+    return (ucp_ep_config(ep)->key.rndv_lanes[0] != UCP_NULL_LANE &&
+            ucp_ep_config(ep)->key.rndv_lanes[1] != UCP_NULL_LANE);


isn't it enough to check just [1] element?

no, on some initialization there is clear only rndv_lanes[0] element (and left all other lanes uninitialized), for such cases we check 2 first lanes to be non-NULL

Do not you want to check: if have more than 1 RNDV lane -> mrail present ?

lane[0] must be initialized (by NULL_LANE or real lane number), lane 1 may be uninitialized, and we have to check both

I guess we have to initialize all RNDV lanes (it's a part of the key and you perform check on it). Then lanes[1] could be either NULL_LANE or some real lane number (which means mrail is used)

will update

brminich · 2017-10-06T16:24:13Z

src/ucp/core/ucp_request.h

@@ -115,6 +115,12 @@ struct ucp_request {
                    uintptr_t     remote_request; /* pointer to the sender's send request */
                    uct_rkey_bundle_t rkey_bundle;
                    ucp_request_t *rreq;    /* receive request on the recv side */
+                    unsigned      use_mrail;


by how much does this increase request size?

8 * (1+sizeof(rkey_byndle))
what is the problem here?

brminich · 2017-10-06T20:16:19Z

src/ucp/core/ucp_request.c

@@ -175,7 +175,8 @@ void ucp_iov_buffer_memh_dereg(uct_md_h uct_md, uct_mem_h *memh,
 UCS_PROFILE_FUNC(ucs_status_t, ucp_request_memory_reg,
                 (context, rsc_index, buffer, length, datatype, state),
                 ucp_context_t *context, ucp_rsc_index_t rsc_index, void *buffer,
-                 size_t length, ucp_datatype_t datatype, ucp_dt_state_t *state)
+                 size_t length, ucp_datatype_t datatype,


why need to split line?

brminich · 2017-10-06T20:17:35Z

src/ucp/core/ucp_request.h

@@ -196,7 +202,8 @@ void ucp_request_send_buffer_dereg(ucp_request_t *req, ucp_lane_index_t lane);

 ucs_status_t ucp_request_memory_reg(ucp_context_t *context, ucp_rsc_index_t rsc_index,
                                    void *buffer, size_t length,
-                                    ucp_datatype_t datatype, ucp_dt_state_t *state);
+                                    ucp_datatype_t datatype,


why need to split line?

brminich · 2017-10-06T20:19:06Z

src/ucp/core/ucp_request.inl

+
+static UCS_F_ALWAYS_INLINE int
+ucp_request_is_empty_rail(ucp_dt_state_t *state, int rail) {
+    return state->dt.mrail[rail].memh == UCT_MEM_HANDLE_NULL ||


is not just check for lane enough?

technically we may have situation when some lanes doesn't require mem-handle, not sure if it is possible in real world, but we should handle such configuration. will look at it after wireup is complete

that's why checking lane for NULL seems to be enough

ok, will fix

brminich · 2017-10-09T07:10:54Z

src/ucp/wireup/select.c

-            ucs_assert(key->rndv_lane == UCP_NULL_LANE);
-            key->rndv_lane = lane;
+            /* TODO: add rndv sort */
+            ucs_assert(key->rndv_lanes[0] == UCP_NULL_LANE);


How will you select what lanes are suitable for RNDV?

don't know for a while, this is in nearest plans :)

brminich · 2017-10-09T07:13:49Z

src/ucp/tag/rndv.c

        /* dereg the original send request since we are going to send on the AM lane next */
        ucp_rndv_rma_request_send_buffer_dereg(sreq);
        sreq->send.state.dt.contig.memh = UCT_MEM_HANDLE_NULL;
+        ucp_request_clear_rails(&sreq->send.state);
+    } else if (ucp_request_have_rails(&sreq->send.state)) {
+        ucp_rndv_rma_request_send_buffer_dereg(sreq);


do not get this. What if am_lane is the same as one of the rails lane? Then you would not need to deregister send buffer, right?

for multirail we have to de-register buffers to avoid resource leaks - the problem is in de-registration: we can't de-register lanes 1+, and not de-register lane 0

why can't we do so?

because it will require deeper refactoring of request/memory registration, which affect non-rndv functionality too

brminich · 2017-10-09T07:14:29Z

src/ucp/tag/rndv.c

-        (ucp_ep_get_am_lane(ep) != ucp_ep_get_rndv_get_lane(ep))) {
+        ucp_request_clear_rails(&sreq->send.state);
+    } else if ((ucp_ep_is_rndv_lane_present(ep, 0)) &&
+        (ucp_ep_get_am_lane(ep) != ucp_ep_get_rndv_get_lane(ep, 0))) {


indentation (thought it was not added by you:)

brminich · 2017-10-09T07:15:49Z

src/ucp/tag/rndv.c

@@ -288,16 +354,33 @@ UCS_PROFILE_FUNC(ucs_status_t, ucp_proto_progress_rndv_get_zcopy, (self),
                   offset, (uintptr_t)rndv_req->send.buffer % align,
                   (void*)rndv_req->send.buffer + offset, length);

+    if ((rndv_req->send.rndv_get.use_mrail) &&
+       ((ucp_request_is_empty_rail(&rndv_req->send.state, rndv_req->send.rndv_get.rail_idx)) ||


indentation

will fix, but current intent is more "beautiful"

brminich · 2017-10-09T07:18:27Z

src/ucp/dt/dt.h

@@ -24,8 +25,14 @@ typedef struct ucp_dt_state {
    size_t                        offset;  /* Total offset in overall payload. */
    union {
        struct {
-            uct_mem_h             memh;
-        } contig;
+            struct {


maybe it is worth to have a single struct and use just one element for contig case?

there is logic to use same lane for am/get and using single element makes processing complicated

swx-jenkins1 · 2017-10-09T08:33:15Z

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

mellanox-github · 2017-10-09T09:16:36Z

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

swx-jenkins1 · 2017-10-09T09:21:40Z

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

mellanox-github · 2017-10-09T10:13:07Z

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

swx-jenkins1 · 2017-10-09T15:01:19Z

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

mellanox-github · 2017-10-09T15:51:23Z

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

swx-jenkins1 · 2017-10-09T17:00:59Z

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

hoopoepg · 2017-10-09T17:55:04Z

bot:retest

mellanox-github · 2017-10-09T17:56:02Z

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

swx-jenkins1 · 2017-10-09T18:36:59Z

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

mellanox-github · 2017-10-09T20:08:17Z

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

hoopoepg · 2017-10-09T20:16:32Z

bot:mlx:retest

mellanox-github · 2017-10-09T21:29:32Z

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

mellanox-github · 2017-10-13T14:26:46Z

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

- added infrastructure to support multi-rail on rndv-RMA: - updated rndv-rma lane to array - added memory handles to store registered buffer and remote keys - added wireup configuration for mrail - added context parameter to enable mrail (disabled by default) UCX_RNDV_RAILS=1

- fixed typo - fixed indentation

- refactoring of datatypes: reduced size of core structures (request & dt)

- unified multi/single rail ways

- renamed rail->lanes

mellanox-github · 2017-11-06T07:02:11Z

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

mellanox-github · 2017-11-06T12:41:54Z

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

yosefe · 2017-11-06T15:06:10Z

src/ucp/core/ucp_request.c

@@ -344,3 +352,52 @@ ucp_request_send_start(ucp_request_t *req, ssize_t max_short,

    return UCS_ERR_NO_PROGRESS;
 }
+
+int ucp_request_rndv_reg(ucp_request_t *req)


rename to ucp_request_rndv_mem_reg(), also i see the return value is not really needed so might as well make it void

yosefe · 2017-11-06T17:08:52Z

src/ucp/core/ucp_request.c

+
+    ucs_assert(UCP_DT_IS_CONTIG(req->send.datatype));
+
+    ucp_dt_clear_rndv_lanes(state);


don't have to clear all lanes - can set memh to NULL as an 'else' clause in line 378

ok, but this infrastructure may be used for AM rndv too, and we have to add complex logic to clean handles

yosefe · 2017-11-06T17:09:25Z

src/ucp/core/ucp_request.c

+    }
+
+    req->send.reg_rsc = UCP_NULL_RESOURCE;
+    ucp_dt_clear_rndv_lanes(state);


can clear only what has been deregistered

yosefe · 2017-11-06T17:12:39Z

src/ucp/core/ucp_request.h

+ */
+typedef struct ucp_rndv_get_rkey {
+    ucp_lane_index_t  lane_num; /* number of rkeys obtained from peer */
+    ucp_lane_index_t  lane_idx;


pls add a comment to describe this field

yosefe · 2017-11-06T17:14:25Z

src/ucp/core/ucp_request.h

+ * available - only one rkey is proceeded
+ */
+typedef struct ucp_rndv_get_rkey {
+    ucp_lane_index_t  lane_num; /* number of rkeys obtained from peer */


why need to keep "lane_num" (which should be renamed to num_lanes) and "lane_idx" in this struct, and not directly in ucp_request.send.rndv_get? this way could avoid this struct altogether:

struct { uint64_t remote_address; /* address of the sender's data buffer */ uintptr_t remote_request; /* pointer to the sender's send request */ ucp_lane_index_t num_lanes; ucp_lane_index_t lane_idx; ucp_request_t *rreq; /* receive request on the recv side */ uct_rkey_bundle_t *rkey_bundle; /* rendezvous-get remote keys */ } rndv_get;

is there free space to add more values?

yosefe · 2017-11-06T17:37:51Z

src/ucp/wireup/select.c

-         ucp_wireup_add_lane_desc(lane_descs, num_lanes_p, rsc_index, addr_index,
-                                 address_list[addr_index].md_index, score,
-                                 UCP_WIREUP_LANE_USAGE_RNDV, 0);
+    if ((params->field_mask & UCP_EP_PARAM_FIELD_ERR_HANDLING_MODE) &&


i thought we disable rndv at all if err handling is enabled.. @evgeny-leksikov ?

in original code rndv lane is configured always. disabled for peer handling error mode

yosefe · 2017-11-06T17:39:05Z

src/ucp/wireup/select.c

+                                             tl_bitmap, remote_md_map, 0,
+                                             &rsc_index, &addr_index, &score);
+        if (status == UCS_OK) {
+            /* Add lane description and remove all occurrences of the remote md */


why is it needed to remote occurrences of remote md?
if you want to avoid multiple rndv lanes on same device, need check device name, rather than the md

yosefe · 2017-11-06T17:40:44Z

src/ucp/wireup/select.c

+                /* In case if selected SHM transport - leave it alone, disable multirail */
+                break;
+            }
+        } else {


i would just do 'if (status!=OK) break' and avoid the 'else'

yosefe · 2017-11-06T17:41:33Z

src/ucp/wireup/select.c

    ucs_qsort_r(key->rma_lanes, UCP_MAX_LANES, sizeof(ucp_lane_index_t),
                ucp_wireup_compare_lane_rma_score, lane_descs);
    ucs_qsort_r(key->amo_lanes, UCP_MAX_LANES, sizeof(ucp_lane_index_t),
                ucp_wireup_compare_lane_amo_score, lane_descs);

+    /* Do not sort rndv lanes to save original sequence */


it seems to contradict the comment '/* Sort RNDV, RMA and AMO lanes according to score */'

comment is updated

yosefe · 2017-11-06T17:41:47Z

src/ucs/sys/compiler.h

@@ -95,6 +95,12 @@

 /**
 * Size of statically-declared array
+ * May be used for arrays in structure values
+ */
+#define ucs_countof(_array) (sizeof(_array) / sizeof((_array)[0]))


not needed anymore?

removed
ucs_static_array_size is not used too, remove it?
BTW, ucs_static_array_size is failed to compile by Intel compiler (due to static assert inside)

yes, let's remove it

jenkinsornl · 2017-11-07T06:32:48Z

Build finished.

jenkinsornl · 2017-11-07T07:46:02Z

Build finished.

swx-jenkins1 · 2017-11-07T07:50:50Z

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

jenkinsornl · 2017-11-07T08:29:58Z

Build finished.

mellanox-github · 2017-11-07T09:08:20Z

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

- functions/values renaming, minor optimization

swx-jenkins1 · 2017-11-07T09:31:52Z

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

jenkinsornl · 2017-11-07T09:38:13Z

Build finished.

swx-jenkins1 · 2017-11-07T09:44:18Z

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

mellanox-github · 2017-11-07T10:23:39Z

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

swx-jenkins1 · 2017-11-07T11:12:23Z

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

mellanox-github · 2017-11-07T11:20:59Z

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

mellanox-github · 2017-11-07T12:23:17Z

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

yosefe · 2017-12-14T16:01:45Z

@hoopoepg can we close this?

hoopoepg · 2017-12-14T16:07:30Z

yes, I will make another PR

hoopoepg force-pushed the multirail-rma-sw branch from 217d8e9 to 3dd33cc Compare October 6, 2017 17:25

evgeny-leksikov reviewed Oct 6, 2017

View reviewed changes

yosefe added the WIP-DNM Work in progress / Do not review label Oct 8, 2017

brminich reviewed Oct 9, 2017

View reviewed changes

hoopoepg force-pushed the multirail-rma-sw branch from 6547897 to bac9ce8 Compare October 9, 2017 08:41

hoopoepg force-pushed the multirail-rma-sw branch from 39bc4fa to 1d6608f Compare October 9, 2017 15:17

hoopoepg force-pushed the multirail-rma-sw branch from 1d6608f to 7cb752d Compare October 9, 2017 17:53

hoopoepg force-pushed the multirail-rma-sw branch 2 times, most recently from 28f9af0 to bdda5df Compare October 13, 2017 14:48

Sergey Oblomov added 5 commits November 6, 2017 06:46

UCP:rndv multirail support infrastructure + protocol (2)

cd83557

- fixed typo - fixed indentation

UCP:rndv multirail support infrastructure + protocol (3)

4f89605

- refactoring of datatypes: reduced size of core structures (request & dt)

UCP:rndv multirail support infrastructure + protocol (4)

d992f41

- unified multi/single rail ways

UCP:rndv multirail support infrastructure + protocol (5)

ab4b3ff

- renamed rail->lanes

yosefe requested changes Nov 6, 2017

View reviewed changes

hoopoepg force-pushed the multirail-rma-sw branch from 8bc7200 to f28651d Compare November 7, 2017 06:02

hoopoepg force-pushed the multirail-rma-sw branch from f28651d to 07e36b9 Compare November 7, 2017 07:16

hoopoepg force-pushed the multirail-rma-sw branch from 07e36b9 to e973ea2 Compare November 7, 2017 08:01

UCP:rndv multirail support infrastructure + protocol (6)

1d7597f

- functions/values renaming, minor optimization

hoopoepg force-pushed the multirail-rma-sw branch from e973ea2 to 1d7597f Compare November 7, 2017 09:09

hoopoepg mentioned this pull request Nov 8, 2017

UCX/RNDV: multirail - updated EP configuration #1981

Merged

yosefe added the WIP-DNM Work in progress / Do not review label Nov 24, 2017

yosefe closed this Dec 14, 2017


		ucs_assert(UCP_DT_IS_CONTIG(req->send.datatype));

		ucp_dt_clear_rndv_lanes(state);

UCP:rndv multirail support infrastructure + protocol #1894

UCP:rndv multirail support infrastructure + protocol #1894

Conversation

hoopoepg commented Oct 6, 2017 • edited Loading

swx-jenkins1 commented Oct 6, 2017

mellanox-github commented Oct 6, 2017

hoopoepg commented Oct 6, 2017

mellanox-github commented Oct 6, 2017

swx-jenkins1 commented Oct 6, 2017

mellanox-github commented Oct 6, 2017

evgeny-leksikov left a comment

Choose a reason for hiding this comment

hoopoepg commented Oct 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brminich Oct 9, 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

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

swx-jenkins1 commented Oct 9, 2017

mellanox-github commented Oct 9, 2017

swx-jenkins1 commented Oct 9, 2017

mellanox-github commented Oct 9, 2017

swx-jenkins1 commented Oct 9, 2017

mellanox-github commented Oct 9, 2017

swx-jenkins1 commented Oct 9, 2017

hoopoepg commented Oct 9, 2017

mellanox-github commented Oct 9, 2017

swx-jenkins1 commented Oct 9, 2017

mellanox-github commented Oct 9, 2017

hoopoepg commented Oct 9, 2017

mellanox-github commented Oct 9, 2017

mellanox-github commented Oct 13, 2017

mellanox-github commented Nov 6, 2017

mellanox-github commented Nov 6, 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

hoopoepg Nov 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jenkinsornl commented Nov 7, 2017

hoopoepg commented Oct 6, 2017 •

edited

Loading

brminich Oct 9, 2017 •

edited

Loading

hoopoepg Nov 7, 2017 •

edited

Loading