TL/UCP: Allow self copy in allgather using network loopback #1021

yaeliyac · 2024-09-19T09:49:01Z

What

Add option for self copy loopback in out of place start for allgather algorithms: knomial, ring, neighbor, sparbit

Why ?

When using UCC plugin for NCCL, using cuda_memcpy might cause deadlock

How ?

Add UCC_TL_UCP_ALLGATHER_USE_LOOPBACK flag to control this
Tested on ISRAEL-1 with multiple test cases - showed good results and same performance with and without loopback

swx-jenkins3 · 2024-09-19T10:13:32Z

Can one of the admins verify this patch?

Sergei-Lebedev · 2024-12-12T13:15:15Z

ok to test

src/components/cl/basic/cl_basic_context.c

src/components/ec/cuda/ec_cuda_executor.c

src/components/tl/ucp/allgather/allgather_knomial.c

src/components/tl/ucp/allgather/allgather_neighbor.c

src/components/tl/ucp/tl_ucp.c

janjust · 2024-12-13T15:34:55Z

@Sergei-Lebedev why do we have to keep approving runs when force pushing?? Seems an outlier here

Sergei-Lebedev · 2024-12-16T09:48:21Z

src/components/tl/ucp/allgather/allgather_ring.c

+            }
+        } else {
+            /* Loopback */
+            ucc_rank_t rank = ucc_ep_map_eval(task->subset.map, trank);


declare variables in the beginning of function

Sergei-Lebedev · 2024-12-16T09:49:58Z

src/components/tl/ucp/allgather/allgather_neighbor.c

+            UCPCHECK_GOTO(ucc_tl_ucp_send_nb(sbuf, data_size, smem, trank, team, task),task, err);
+            UCPCHECK_GOTO(ucc_tl_ucp_recv_nb(PTR_OFFSET(rbuf, data_size * trank), data_size, rmem, trank, team, task),task, err);


maybe make a loopback copy function, this pattern repeats in every algorithm

Sergei-Lebedev · 2024-12-16T09:50:53Z

src/components/tl/ucp/allgather/allgather_knomial.c

+        }
+    }
+
+    if (use_cuda) EXEC_TASK_TEST(UCC_KN_PHASE_INIT, "failed during ee task test", task->allgather_kn.etask);


just else for prev if

Sergei-Lebedev · 2024-12-16T09:52:54Z

src/components/tl/ucp/allgather/allgather_ring.c

-        step = task->tagged.send_posted;
+    int iter = use_cuda ? tsize - 1 : tsize; //when using loopback tagged.send_posted has 1 more which will cause non-complete ring algorithm
+    while (task->tagged.send_posted < iter) {
+        step = use_cuda ? task->tagged.send_posted : task->tagged.send_posted - 1; //when using loopback tagged.send_posted has 1 more which will cause wrong calculation of send/recv


since loopback affects algorithm flow I would rather consider checking loopback copy result somehow differently, maybe add custom callback for ucp recv completion

samnordmann

Thank you! It Looks good, I left a bunch of comments mainly on codestyle.

However, please add tests for that feature.

Also please note from the CI:

Commit title is too long: 59
Bad commit title: 'TL/UCP: Allow self copy in allgather using network loopback'

samnordmann · 2024-12-16T09:14:43Z

src/components/tl/ucp/allgather/allgather_knomial.c

@@ -77,9 +77,15 @@ void ucc_tl_ucp_allgather_knomial_progress(ucc_coll_task_t *coll_task)
    size_t                 peer_seg_count, local_seg_count;
    ucc_status_t           status;
    size_t                 extra_count;
+    int                    use_cuda = UCC_TL_UCP_TEAM_LIB(team)->cfg.allgather_use_cuda;


codestyle: initialized variables go first. Please, move this line upwards, to be just after
size_t local = GET_LOCAL_COUNT(args, size, rank);, and please align the datatype, name variable and "=" sign

samnordmann · 2024-12-16T09:17:42Z

src/components/tl/ucp/allgather/allgather_knomial.c

+        }
+    }
+
+    if (use_cuda) EXEC_TASK_TEST(UCC_KN_PHASE_INIT, "failed during ee task test", task->allgather_kn.etask);


codestyle: please use curly brakcets and new line:

for (condition) { [body] }

samnordmann · 2024-12-16T09:24:39Z

src/components/tl/ucp/tl_ucp.h

@@ -45,6 +45,7 @@ extern ucc_tl_ucp_iface_t ucc_tl_ucp;
 typedef struct ucc_tl_ucp_lib_config {
    ucc_tl_lib_config_t      super;
    uint32_t                 kn_radix;
+    uint32_t                 allgather_use_cuda;


we usually use int for booleans, rather than uint32_t. Btw it needs to be consistent with int use_cuda = UCC_TL_UCP_TEAM_LIB(team)->cfg.allgather_use_cuda; from allgather_knomial.c

samnordmann · 2024-12-16T09:25:14Z

src/components/tl/ucp/tl_ucp.c

@@ -140,6 +140,10 @@ ucc_config_field_t ucc_tl_ucp_lib_config_table[] = {
     ucc_offsetof(ucc_tl_ucp_lib_config_t, allgather_kn_radix),
     UCC_CONFIG_TYPE_UINT},

+     {"ALLGATHER_USE_CUDA", "1", "If set to 1 uses mc cuda copy, otherwise performs loopback for self copy",
+     ucc_offsetof(ucc_tl_ucp_lib_config_t, allgather_use_cuda),
+     UCC_CONFIG_TYPE_UINT},


Suggested change

UCC_CONFIG_TYPE_UINT},

UCC_CONFIG_TYPE_BOOL},

src/components/tl/ucp/tl_ucp.c

samnordmann · 2024-12-16T10:03:02Z

src/components/tl/ucp/allgather/allgather_sparbit.c

+        if (!use_loopback) {
+            status = ucc_mc_memcpy(PTR_OFFSET(rbuf, data_size * trank), sbuf,
                               data_size, rmem, smem);
-        if (ucc_unlikely(UCC_OK != status)) {
-            return status;
+            if (ucc_unlikely(UCC_OK != status)) {
+                return status;
+            }
+        } else {
+            /* Loopback */
+            UCPCHECK_GOTO(ucc_tl_ucp_send_nb(sbuf, data_size, smem, trank, team, task),task, out);
+            UCPCHECK_GOTO(ucc_tl_ucp_recv_nb(PTR_OFFSET(rbuf, data_size * trank), data_size, rmem, trank, team, task),task, out);


this whole code block is duplicated in neighbor, ring and sparbit. It should be refactored into a helper function and called there.

Ideally, this could also be refactored with the call from knomial, with an additional control about whether to use ucc_mc_memcpy or the executor.

samnordmann · 2024-12-16T10:04:15Z

src/components/tl/ucp/allgather/allgather_sparbit.c

        }
    }

    return ucc_progress_queue_enqueue(UCC_TL_CORE_CTX(team)->pq, &task->super);
+out:


a goto redirection is not needed here. We could simply check status when it is returned, and return it if it is in error state.

Apply in other places of this PR where this pattern is used

samnordmann · 2024-12-16T10:07:22Z

src/components/tl/ucp/allgather/allgather_ring.c


    if (UCC_INPROGRESS == ucc_tl_ucp_test(task)) {
        return;
    }
    sendto   = ucc_ep_map_eval(task->subset.map, (trank + 1) % tsize);
    recvfrom = ucc_ep_map_eval(task->subset.map, (trank - 1 + tsize) % tsize);

-    while (task->tagged.send_posted < tsize - 1) {
-        step = task->tagged.send_posted;
+    int iter = use_loopback ? tsize : tsize - 1; //when using loopback tagged.send_posted has 1 more which will cause non-complete ring algorithm


declaration of variable goes to the beginning of the function

I am not sure to understand the comment. What do you mean by "will cause non-complete ring algorithm"?
Can you use a more explicit name than iter for that variable?

samnordmann · 2024-12-16T10:07:32Z

src/components/tl/ucp/allgather/allgather_ring.c

@@ -43,15 +43,18 @@ void ucc_tl_ucp_allgather_ring_progress(ucc_coll_task_t *coll_task)
    ucc_rank_t         sendto, recvfrom, sblock, rblock;
    int                step;
    void              *buf;
+    int                use_loopback = UCC_TL_UCP_TEAM_LIB(team)->cfg.allgather_use_loopback;


initialized variables go first

yaeliyac force-pushed the origin/for_plugin branch from 5e852a7 to 7e6c55d Compare October 3, 2024 08:55

manjugv added the Don't Merge label Oct 30, 2024

yaeliyac closed this Nov 6, 2024

yaeliyac reopened this Nov 6, 2024

yaeliyac force-pushed the origin/for_plugin branch from d13b885 to b3c2e80 Compare November 10, 2024 14:02

yaeliyac marked this pull request as ready for review November 18, 2024 15:39

yaeliyac changed the title ~~UCC plugin for NCCL - adjustments~~ Allow self copy in allgather using network loopback Dec 11, 2024

janjust added the Code-Review-Required label Dec 11, 2024

janjust requested review from nsarka and janjust December 11, 2024 16:08

nsarka approved these changes Dec 12, 2024

View reviewed changes

samnordmann self-requested a review December 13, 2024 12:35

yaeliyac force-pushed the origin/for_plugin branch 3 times, most recently from 482006f to eecdebf Compare December 13, 2024 15:28

janjust removed the Don't Merge label Dec 13, 2024

yaeliyac force-pushed the origin/for_plugin branch from 1d88c9b to 820794d Compare December 13, 2024 15:31

yaeliyac force-pushed the origin/for_plugin branch 3 times, most recently from 32b3ccd to b1c875b Compare December 15, 2024 11:49

yaeliyac changed the title ~~Allow self copy in allgather using network loopback~~ TL/UCP: Allow self copy in allgather using network loopback Dec 15, 2024

yaeliyac force-pushed the origin/for_plugin branch 3 times, most recently from 2dc74f3 to a2c5fee Compare December 15, 2024 16:18

Sergei-Lebedev reviewed Dec 16, 2024

View reviewed changes

TL/UCP: Allow self copy in allgather using network loopback

5940100

yaeliyac force-pushed the origin/for_plugin branch from a2c5fee to 5940100 Compare December 16, 2024 10:01

samnordmann reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TL/UCP: Allow self copy in allgather using network loopback #1021

TL/UCP: Allow self copy in allgather using network loopback #1021

yaeliyac commented Sep 19, 2024 •

edited

Loading

swx-jenkins3 commented Sep 19, 2024

Sergei-Lebedev commented Dec 12, 2024

janjust commented Dec 13, 2024

Sergei-Lebedev Dec 16, 2024

Sergei-Lebedev Dec 16, 2024

Sergei-Lebedev Dec 16, 2024

Sergei-Lebedev Dec 16, 2024

samnordmann left a comment •

edited

Loading

samnordmann Dec 16, 2024

samnordmann Dec 16, 2024

samnordmann Dec 16, 2024

samnordmann Dec 16, 2024

samnordmann Dec 16, 2024

samnordmann Dec 16, 2024

samnordmann Dec 16, 2024 •

edited

Loading

samnordmann Dec 16, 2024

samnordmann Dec 16, 2024

		UCPCHECK_GOTO(ucc_tl_ucp_send_nb(sbuf, data_size, smem, trank, team, task),task, err);
		UCPCHECK_GOTO(ucc_tl_ucp_recv_nb(PTR_OFFSET(rbuf, data_size * trank), data_size, rmem, trank, team, task),task, err);

TL/UCP: Allow self copy in allgather using network loopback #1021

Are you sure you want to change the base?

TL/UCP: Allow self copy in allgather using network loopback #1021

Conversation

yaeliyac commented Sep 19, 2024 • edited Loading

What

Why ?

How ?

swx-jenkins3 commented Sep 19, 2024

Sergei-Lebedev commented Dec 12, 2024

janjust commented Dec 13, 2024

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

samnordmann left a comment • 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

samnordmann Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yaeliyac commented Sep 19, 2024 •

edited

Loading

samnordmann left a comment •

edited

Loading

samnordmann Dec 16, 2024 •

edited

Loading