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

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

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

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

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

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'

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

src/components/tl/ucp/tl_ucp.h

src/components/tl/ucp/tl_ucp.c

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

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

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

src/components/tl/ucp/tl_ucp.c

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

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

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

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

samnordmann

LGTM!

samnordmann · 2025-02-11T08:46:12Z

test/gtest/coll/test_allgather.cc

@@ -9,7 +9,8 @@

 using Param_0 = std::tuple<int, ucc_datatype_t, ucc_memory_type_t, int, gtest_ucc_inplace_t>;
 using Param_1 = std::tuple<ucc_datatype_t, ucc_memory_type_t, int, gtest_ucc_inplace_t>;
-using Param_2 = std::tuple<ucc_datatype_t, ucc_memory_type_t, int, gtest_ucc_inplace_t, std::string>;
+using Param_2 = std::tuple<ucc_datatype_t, ucc_memory_type_t, int,


Should the new param use_loopback rather be a bool ?

samnordmann · 2025-02-11T09:16:56Z

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

        if (ucc_unlikely(UCC_OK != status)) {
            return status;
        }
    }

+    while (UCC_INPROGRESS == ucc_tl_ucp_test(task)) {


just checking: is this call safe in the "inplace" case ?

samnordmann · 2025-02-11T09:18:05Z

src/components/tl/ucp/tl_ucp.c

+     "If set to 1 performs network loopback for self copy, otherwise uses mc "
+     "cuda copy",


Suggested change

"If set to 1 performs network loopback for self copy, otherwise uses mc "

"cuda copy",

"If set to 1 performs network loopback for self copy, otherwise uses mc",

it is not necessarily cuda

nsarka · 2025-02-12T17:17:27Z

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

+                                ucc_tl_ucp_task_t *task)
+{
+    ucc_status_t status;
+    status = ucc_tl_ucp_send_nb(sbuf, data_size, smem, rank, team, task);


Instead of status = ... ; if (UCC_OK != status) ... you can use UCC_CHECK_GOTO(..., err)

nsarka · 2025-02-12T17:17:47Z

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

+        task->super.status = status;
+        return task->super.status;
+    }
+    return UCC_OK;


err: return status;

nsarka · 2025-02-12T17:19:37Z

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

@@ -54,8 +55,7 @@

 void ucc_tl_ucp_allgather_knomial_progress(ucc_coll_task_t *coll_task)
 {
-    ucc_tl_ucp_task_t     *task      = ucc_derived_of(coll_task,
-                                                      ucc_tl_ucp_task_t);
+    ucc_tl_ucp_task_t *    task = ucc_derived_of(coll_task, ucc_tl_ucp_task_t);


nsarka · 2025-02-12T17:20:55Z

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

-                                       args->root : 0;
-    ucc_rank_t             rank      = VRANK(task->subset.myrank, broot, size);
-    size_t                 local     = GET_LOCAL_COUNT(args, size, rank);
+    ucc_rank_t broot = args->coll_type == UCC_COLL_TYPE_BCAST ? args->root : 0;


nsarka · 2025-02-12T17:40:13Z

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

@@ -67,6 +69,8 @@ void ucc_tl_ucp_allgather_ring_progress(ucc_coll_task_t *coll_task)
        if (UCC_INPROGRESS == ucc_tl_ucp_test(task)) {
            return;
        }
+        step = use_loopback ? task->tagged.send_posted - 1


Should this line check for inplace as well? If use_loopback is true and inplace is false, will step be incorrect?

Similar for neighbor exchange

janjust · 2025-02-12T21:00:22Z

@Sergei-Lebedev @samnordmann @yaeliyac @lappazos

The more I look at this implementation the less I think this is the right approach, and I'm surprised we would allow this in. We're introducing an if-else statement all throughout critical paths of the algorithms to avoid this issues specific to the nccl_plugin and only during inplace.
What if we need to add additional algorithms, then we need to be aware of this limitation and add the same if-else, or if we need to expand this for A2A, A2AV, or AGV? Why is AG the only collective that suffers this deadlock?
Thinking about this in more detail I don't think this is a good approach, we should really consider this carefully, regardless of the deadlock fix. As an immediate alternative, I think at the very least we should provide a loop-back copy or the mc_copy from a higher level.

But on a broader note, what exactly is the deadlock here? It seems to be an issue specifically with nccl_plugin, and why does loopback fix it?

Can we have you guys jump on another devcall and review this with us please.

nsarka · 2025-02-13T16:47:33Z

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

        if (ucc_unlikely(UCC_OK != status)) {
            return status;
        }
    }

+    while ((!UCC_IS_INPLACE(TASK_ARGS(task))) && (UCC_INPROGRESS == ucc_tl_ucp_test(task))) {


Instead of setting counter to a different value depending on inplace and use_loopback, should it just decrement send_posted/completed and recv_posted/completed in allgather_copy? That would remove a good bit of complexity

nsarka · 2025-02-13T16:48:46Z

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

+    }
+    return UCC_OK;
+}
+ucc_status_t allgather_copy(void *rbuf, void *sbuf, size_t data_size,


allgather_copy should probably be renamed to copy_sbuf_inplace

nsarka · 2025-02-13T17:05:21Z

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

+            counter = task->tagged.send_posted - 1;
+        } else {
+            counter = task->tagged.send_posted;
+        }


These two if statements can be removed if send_posted is decremented right after the send is completed

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

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

ikryukov reviewed Dec 18, 2024

View reviewed changes

yaeliyac force-pushed the origin/for_plugin branch 2 times, most recently from 5619791 to caa90f8 Compare December 22, 2024 16:08

yaeliyac force-pushed the origin/for_plugin branch from caa90f8 to ee808ea Compare January 6, 2025 13:50

ikryukov reviewed Jan 6, 2025

View reviewed changes

src/components/tl/ucp/allgather/allgather_bruck.c Outdated Show resolved Hide resolved

src/components/tl/ucp/allgather/allgather_knomial.c Outdated Show resolved Hide resolved

yaeliyac force-pushed the origin/for_plugin branch 6 times, most recently from b6d4812 to 54e86f2 Compare January 9, 2025 17:50

nsarka added Code-Review-Completed and removed Code-Review-Required labels Jan 29, 2025

yaeliyac force-pushed the origin/for_plugin branch 7 times, most recently from 4969914 to 3f87beb Compare February 10, 2025 18:20

samnordmann approved these changes Feb 11, 2025

View reviewed changes

nsarka reviewed Feb 12, 2025

View reviewed changes

yaeliyac force-pushed the origin/for_plugin branch from 3f87beb to b9331ee Compare February 12, 2025 18:19

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

c4eb0df

yaeliyac force-pushed the origin/for_plugin branch from b9331ee to c4eb0df Compare February 12, 2025 18:22

janjust added Code-Review-Required and removed Code-Review-Completed labels Feb 12, 2025

nsarka reviewed Feb 13, 2025

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

samnordmann left a comment •

edited

Loading

samnordmann left a comment

samnordmann Feb 11, 2025

samnordmann Feb 11, 2025

samnordmann Feb 11, 2025 •

edited

Loading

nsarka Feb 12, 2025 •

edited

Loading

nsarka Feb 12, 2025

nsarka Feb 12, 2025

nsarka Feb 12, 2025

nsarka Feb 12, 2025

nsarka Feb 12, 2025

janjust commented Feb 12, 2025

nsarka Feb 13, 2025

nsarka Feb 13, 2025

nsarka Feb 13, 2025 •

edited

Loading

		"If set to 1 performs network loopback for self copy, otherwise uses mc "
		"cuda copy",

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

samnordmann left a comment • edited Loading

Choose a reason for hiding this comment

samnordmann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samnordmann Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

nsarka Feb 12, 2025 • 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

janjust commented Feb 12, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nsarka Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

yaeliyac commented Sep 19, 2024 •

edited

Loading

samnordmann left a comment •

edited

Loading

samnordmann Feb 11, 2025 •

edited

Loading

nsarka Feb 12, 2025 •

edited

Loading

nsarka Feb 13, 2025 •

edited

Loading