UCX plugin: Add multi-receive posting to avoid excessive flush #125

tvegas1 · 2023-10-06T15:15:21Z

What?

Add to UCX plugin the capability to post multiple receives at once.

Why?

Posting 8 receives at once reduces the flushing by 8 times, which fixes perf on alltoall_perf (nccl-test).

Selene luna, alltoall_perf, 4 nodes, 32 gpus, tested 1 gpu x 8 processes on 4 nodes, tested 8 gpus x 1 process on 4 nodes

Internal IB / UCX Before / UCX After: 31.84 / 24.27 / 31.89

How?

Add multi-request handling
Remove support for UCX <1.10 (was not building since 1+ year)
Merge comm descriptor

Tested

nccl-test alltoall, allreduce

Versions:

UCX 96dcb9fc20353cc59478256e33eba152751fa4b4
nccl-rdma-sharp-plugins d387b8a
nccl 4365458757e4107ecbf629b2fd6e0e19a5d237c2 (~v2.18.5-1)

Command:

NCCL_PLUGIN_P2P=ucx
NCCL_UCX_TLS=rc,cuda_copy,cuda_ipc
all_reduce_perf -b 8 -e 8G -f 2 -g 8 -t 1 -w 100

tvegas1 · 2023-10-06T15:19:34Z

@bureddy, @brminich, @yosefe
please look and instruct for regs to run

yosefe · 2023-10-09T15:55:12Z

src/ucx_plugin.c

+                        UCP_OP_ATTR_FLAG_NO_IMM_CMPL;
+  params.cb.recv      = recv_handler_nbx;
+  params.user_data    = &pending;
+  params.datatype     = ucp_dt_make_contig(1);


no need to pass datatype

fixed for all

yosefe · 2023-10-09T15:55:48Z

src/ucx_plugin.c

+  params.cb.recv      = recv_handler_nbx;
+  params.user_data    = &pending;


why need a callback - can't we just wait for the request?

yosefe · 2023-10-09T15:59:28Z

src/ucx_plugin.c

+                          UCP_OP_ATTR_FIELD_DATATYPE;
+    params.cb.recv      = recv_handler_nbx;
+    params.user_data    = &req->pending;
+    params.datatype     = ucp_dt_make_contig(1);


no need to pass datatype

yosefe · 2023-10-09T15:59:53Z

src/ucx_plugin.c

+    params.op_attr_mask = UCP_OP_ATTR_FIELD_CALLBACK |
+                          UCP_OP_ATTR_FIELD_USER_DATA |
+                          UCP_OP_ATTR_FIELD_DATATYPE;
+    params.cb.recv      = recv_handler_nbx;
+    params.user_data    = &req->pending;


could be initialized outside the loop

yosefe · 2023-10-09T16:03:46Z

src/ucx_plugin.c

+    if (req->used == 0) {
+      req->worker  = comm->worker;
+      req->pending = 0;
+      req->count   = 0;
+      req->used    = 1;
+      return req;


seems too slow to iterate over the array like that
can we use a freelist? or bitmap (FFS) if num of requests is <=64 ?

added freelist

yosefe · 2023-10-09T16:06:46Z

src/ucx_plugin.c

+  req = ucx_request_get(comm);
+  if (req == NULL) {
+    return ncclInternalError;
+  }


maybe for send flow we don't need this since there is always a single buffer to send

let me know if this is what you meant but i think we need it as-is because:

we want to return *request = req with always same type as other callbacks

api does not seem to guarantee that we have only one req instance in-flight, so we still need to allocate one

our callback ->test() that does progress will need to be called with a valid request

yosefe · 2023-10-09T16:08:06Z

src/ucx_plugin.c

+  if (size != NULL) {
+    /* Posted receives have completed */
+    for (int i = 0; i < req->count; i++) {
+      size[i] = req->size[i];


can we use memcpy?

bureddy · 2023-10-10T15:28:44Z

src/ucx_plugin.c

-  ucp_params.request_size = sizeof(ucx_request_t);
-  ucp_params.request_init = request_init;
+  ucp_params.field_mask = UCP_PARAM_FIELD_FEATURES;
+  ucp_params.features   = UCP_FEATURE_TAG | UCP_FEATURE_RMA;


we are not using RMA in this plugin. is it better to drop this?

@brminich reminded me that we still need it for iflush/ucp_get

brminich · 2023-10-10T16:38:34Z

src/ucx_plugin.c

@@ -1,5 +1,5 @@
 /*************************************************************************
- * Copyright (c) 2016-2020, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
+ * Copyright (c) 2016-2023, NVIDIA CORPORATION & AFFILIATES. All rights reserved.


minor: worth updating in none or all files

brminich · 2023-10-10T16:54:13Z

src/ucx_plugin.c

+                        UCP_OP_ATTR_FIELD_DATATYPE;
+  params.cb.send      = check_handler;
+  params.user_data    = comm;
+  params.datatype     = ucp_dt_make_contig(1);


we can avoid passing datatype here as well
(ucp_dt_make_contig(1) is a default option when UCP_OP_ATTR_FIELD_DATATYPE is not passed)

Sergei-Lebedev · 2023-10-12T08:53:55Z

src/ucx_plugin.c

    params.op_attr_mask |= UCP_OP_ATTR_FIELD_MEMORY_TYPE;
+    params.memory_type   = mh->mem_type;


if we anyway use "modern" UCP API here why not to pass memory handle for preregistered buffers?

let's postpone this change, as it may imply additional limitations

plan is to check that independently from multi-recv fix. but for my understanding, currently, the memh is found internally through rcache from passed addr/len, is that correct?

yes, it should be found by rcache

src/ucx_plugin.c

tvegas1 added 8 commits October 3, 2023 11:41

Advertize multiple operations per call for UCX

6fcabab

Multiple receive for UCX plugin

1f237ef

Only use one comm

abf2795

Code cleanup

3259585

Code cleanup (2)

b5c3e3a

Remove UCX <1.10 support

bb76a10

Rename ucx_nccl_request to ucx_request

9cd0407

Fix codestyle, and cleanup code

5650fbd

yosefe reviewed Oct 9, 2023

View reviewed changes

bureddy reviewed Oct 10, 2023

View reviewed changes

brminich reviewed Oct 10, 2023

View reviewed changes

tvegas1 added 2 commits October 11, 2023 10:27

Address review: remove datatype parameter

90a623b

Address review: add freelist and various code fixes

d6ff08f

brminich approved these changes Oct 11, 2023

View reviewed changes

Sergei-Lebedev reviewed Oct 12, 2023

View reviewed changes

tvegas1 commented Oct 17, 2023

View reviewed changes

src/ucx_plugin.c Show resolved Hide resolved

Add missing ucp_request_free

264e50b

tvegas1 force-pushed the dev/multi_recv branch from 6884b03 to 264e50b Compare October 18, 2023 14:52

bureddy approved these changes Oct 18, 2023

View reviewed changes

bureddy merged commit 7845726 into Mellanox:master Oct 18, 2023
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UCX plugin: Add multi-receive posting to avoid excessive flush #125

UCX plugin: Add multi-receive posting to avoid excessive flush #125

tvegas1 commented Oct 6, 2023

tvegas1 commented Oct 6, 2023

yosefe Oct 9, 2023

tvegas1 Oct 11, 2023

yosefe Oct 9, 2023

tvegas1 Oct 11, 2023

yosefe Oct 9, 2023

tvegas1 Oct 11, 2023

yosefe Oct 9, 2023

tvegas1 Oct 11, 2023

yosefe Oct 9, 2023

tvegas1 Oct 11, 2023

yosefe Oct 9, 2023

tvegas1 Oct 11, 2023

yosefe Oct 9, 2023

tvegas1 Oct 11, 2023

bureddy Oct 10, 2023

tvegas1 Oct 11, 2023

brminich Oct 10, 2023

tvegas1 Oct 11, 2023

brminich Oct 10, 2023

tvegas1 Oct 11, 2023

Sergei-Lebedev Oct 12, 2023

brminich Oct 12, 2023

tvegas1 Oct 12, 2023

brminich Oct 12, 2023

		params.cb.recv = recv_handler_nbx;
		params.user_data = &pending;

		params.op_attr_mask \|= UCP_OP_ATTR_FIELD_MEMORY_TYPE;
		params.memory_type = mh->mem_type;

UCX plugin: Add multi-receive posting to avoid excessive flush #125

UCX plugin: Add multi-receive posting to avoid excessive flush #125

Conversation

tvegas1 commented Oct 6, 2023

What?

Why?

How?

Tested

tvegas1 commented Oct 6, 2023

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