UCP RMA: Fix put-based plugin by tvegas1 · Pull Request #167 · Mellanox/nccl-rdma-sharp-plugins

tvegas1 · 2024-10-04T07:31:46Z

Fix UCP RMA plugin.

brminich

first bunch of comments

brminich · 2024-11-04T16:20:24Z

+NCCL_PARAM(UCXAckDelay, "UCXPUT_ACK_DELAY", 1);
+NCCL_PARAM(UCXAckSkip, "UCXPUT_ACK_SKIP", 0);


why not

Suggested change

NCCL_PARAM(UCXAckDelay, "UCXPUT_ACK_DELAY", 1);

NCCL_PARAM(UCXAckSkip, "UCXPUT_ACK_SKIP", 0);

NCCL_PARAM(UCXAckDelay, "UCX_PUT_ACK_DELAY", 1);

NCCL_PARAM(UCXAckSkip, "UCX_PUT_ACK_SKIP", 0);

brminich · 2024-11-04T16:25:50Z

+  char           inflight; /* Chunk still being sent */
+  char           reqs;     /* Count request alive */
+  int            sizes[NCCL_UCP_MAX_RECV];
+  unsigned short id; /* Id of the origin RTR again */


why need two ids?

in general, to check if data has fully arrived (although in this case the struct occupies only 1 cacheline)

brminich · 2024-11-08T13:37:34Z

-}
+static ncclResult_t nccl_ucx_rma_connect(int dev, void *listen_handle,
+                                         void **send_comm,
+                                         ncclNetDeviceHandle_t **sendDevComm) {


why not ncclNetDeviceHandle_v7_t?

keep main function with latest struct definition (also matches implementation in the ib_plugin.c file).

anyways we have in net_device.h:

27 typedef ncclNetDeviceHandle_v7_t ncclNetDeviceHandle_v8_t; 28 typedef ncclNetDeviceHandle_v8_t ncclNetDeviceHandle_t;

brminich · 2024-11-08T14:32:33Z

+  w->address        = malloc(attr.address_length);
+  if (w->address == NULL) {
+    ucp_worker_release_address(w->ucp_worker, attr.address);
+    return ncclSystemError;


need to destroy worker

brminich · 2024-11-08T14:32:48Z

+  UCXCHECK(ucp_worker_query(w->ucp_worker, &attr));
+
+  if (attr.thread_mode != UCS_THREAD_MODE_MULTI) {
+    INFO(NCCL_NET, "Thread mode multi is not supported");


should it be error?

brminich · 2024-11-14T09:07:11Z

+  req->comm      = comm;
+  req->type      = NCCL_UCP_TYPE_ISEND;
+  req->rtr_id    = rtr->id_start;
+  req->inflight  = (UCS_PTR_STATUS(status_ptr) == UCS_INPROGRESS);


Suggested change

req->inflight = (UCS_PTR_STATUS(status_ptr) == UCS_INPROGRESS);

req->inflight = (UCS_PTR_IS_PTR(status_ptr));

brminich · 2024-11-14T09:08:09Z

-    if (UCS_PTR_IS_ERR(req->st)) {
-      WARN("NET/UCX_RMA: am_send_nb failed");
-      return ncclInternalError;
+  assert(tag != INT_MAX);


we use this value to mark entries as consumed later

brminich · 2024-11-14T09:09:28Z

-  comm->rem_fifo.tail++;
+out:
+  if ((*request == NULL) && (comm->total == 0)) {
+    ucp_worker_progress(comm->worker->ucp_worker);


why needed?

depending on the transport, it could be that we need to progress to receive the first remote puts.
(could there also be timeouts to service?).

brminich · 2024-11-14T09:11:34Z

+  assert((UCS_PTR_STATUS(status_ptr) == UCS_INPROGRESS) ||
+         (UCS_PTR_STATUS(status_ptr) == UCS_OK));


it is already checked 2 lines above

brminich · 2024-11-14T09:16:54Z

+        status         = nccl_ucp_shared_put(comm, atp, sizeof(*atp), remote,
+                                             &req->inflight);
+        req->inflight += (status == UCS_INPROGRESS);
+        rtr->avail    -= (status == UCS_INPROGRESS) || (status == UCS_OK);


Suggested change

rtr->avail -= (status == UCS_INPROGRESS) || (status == UCS_OK);

rtr->avail -= !UCS_STATUS_IS_ERR(status);

tvegas1 added 2 commits October 3, 2024 16:09

UCP RMA: Fix put-based plugin

0a245b7

UCP RMA: Fix put-based plugin

95c140e

brminich reviewed Nov 14, 2024

View reviewed changes

tvegas1 added 11 commits November 18, 2024 13:48

UCP RMA: Check for null pointer before copying sizes

0671c1d

UCP RMA: Address review comments

3cd4a68

UCP RMA: Address review comments

5bddc71

UCP RMA: Address review comments

a6b9270

UCP RMA: Address review comments

3cf2f96

UCP RMA: Address review comments

b0222cb

UCP RMA: Address review comments

bc83b44

UCP RMA: Address review comments

e582c5d

Merge remote-tracking branch 'upstream/master' into HEAD

b9d94a3

UCP RMA: Merge remote tracking branch

149c3e2

UCP RMA: Add v9 plugin API

730e90d

		NCCL_PARAM(UCXAckDelay, "UCXPUT_ACK_DELAY", 1);
		NCCL_PARAM(UCXAckSkip, "UCXPUT_ACK_SKIP", 0);

	req->inflight = (UCS_PTR_STATUS(status_ptr) == UCS_INPROGRESS);
	req->inflight = (UCS_PTR_IS_PTR(status_ptr));

		assert((UCS_PTR_STATUS(status_ptr) == UCS_INPROGRESS) \|\|
		(UCS_PTR_STATUS(status_ptr) == UCS_OK));

	rtr->avail -= (status == UCS_INPROGRESS) \|\| (status == UCS_OK);
	rtr->avail -= !UCS_STATUS_IS_ERR(status);

Uh oh!

Conversation

tvegas1 commented Oct 4, 2024

Uh oh!

brminich left a comment

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!

tvegas1 Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tvegas1 Nov 18, 2024 •

edited

Loading