DRAFT : Register memory #1010

jeffnvidia · 2024-08-19T10:03:09Z

What

add callback function to replace cuda copy

Why ?

There is a deadlock when executing collectives in AI workload

swx-jenkins3 · 2024-08-19T10:10:17Z

Can one of the admins verify this patch?

lappazos · 2024-08-21T08:51:34Z

src/components/ec/base/ucc_ec_base.h

 } ucc_ee_executor_task_t;

+typedef struct node_ucc_ee_executor_task node_ucc_ee_executor_task_t;
+typedef struct node_ucc_ee_executor_task {
+    ucc_ee_executor_task_t      *val;


change val to etask

lappazos · 2024-08-21T08:51:55Z

src/components/tl/ucp/tl_ucp_coll.h

@@ -57,6 +57,23 @@ void ucc_tl_ucp_team_default_score_str_free(
    }                                                                          \
 } while(0)

+// #define EXEC_TASK_TEST_2(_errmsg, _etask) do {                                 


lappazos · 2024-08-21T08:53:47Z

src/components/tl/ucp/tl_ucp_coll.h

+    while (polls++ < task->n_polls) {
+        node_ucc_ee_executor_task_t *current_node;
+        current_node = task->allgather_kn.etask_linked_list_head;
+        while(current_node != NULL) {


Ask Sergey should this be put inside / outside of loop

what are you talking about ? I don't remember

performance wise, should we iterate the list of etasks inside the while loop, or outside (1 TIME OR N_POLL TIME)

almost no performance difference according to Sergey

lappazos · 2024-08-21T08:57:54Z

src/components/tl/ucp/tl_ucp_coll.h

+                status = ucc_ee_executor_task_test(current_node->val);                            \
+                if (status > 0) {                                                      \
+                    ucc_ee_executor_task_finalize(current_node->val);                                 \
+                    ucp_memcpy_device_complete(current_node->val->completion, status);


have to come before finalize

lappazos · 2024-08-21T08:58:17Z

src/components/tl/ucp/tl_ucp_coll.h

+        node_ucc_ee_executor_task_t *current_node;
+        current_node = task->allgather_kn.etask_linked_list_head;
+        while(current_node != NULL) {
+            if (current_node->val != NULL) {


can be removed

lappazos · 2024-08-21T09:29:27Z

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

+    task->allgather_kn.etask_linked_list_head = (node_ucc_ee_executor_task_t *)malloc(sizeof(node_ucc_ee_executor_task_t));
+    task->allgather_kn.etask_linked_list_head->val = NULL;


task->allgather_kn.etask_linked_list_head = NULL

lappazos · 2024-08-21T09:34:53Z

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

+    ucc_tl_ucp_context_t *ctx = UCC_TL_UCP_TEAM_CTX(team);
+    ucp_mem_map_params_t mmap_params;
+    ucp_mem_h            mh;
+    int size_of_list = 6;


maybe start with 1;

lappazos · 2024-08-21T09:35:59Z

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

+                            UCP_MEM_MAP_PARAM_FIELD_MEMORY_TYPE;
+    mmap_params.memory_type = ucc_memtype_to_ucs[mem_type];
+
+    // EXEC_TASK_TEST(UCC_KN_PHASE_INIT, "failed during ee task test",


could be removed

lappazos · 2024-08-21T09:39:24Z

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

+
+            mmap_params.address     = task->allgather_kn.sbuf;
+            mmap_params.length      = local * dt_size;
+            UCPCHECK_GOTO(ucs_status_to_ucc_status(ucp_mem_map(ctx->worker.ucp_context, &mmap_params, &mh)),


dst is not important? ask Sergey
ucc_tl_ucp_send_nb(void *buffer, size_t msglen, ucc_memory_type_t mtype,
ucc_rank_t dest_group_rank, ucc_tl_ucp_team_t *team,
ucc_tl_ucp_task_t *task)

dest isnt important

lappazos · 2024-08-21T09:50:59Z

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

+        mmap_params.address     = rbuf;
+        mmap_params.length      = data_size;
+        UCPCHECK_GOTO(ucs_status_to_ucc_status(ucp_mem_map(ctx->worker.ucp_context, &mmap_params, &mh)),
+                        task, out);
+        if (count_mh == size_of_list){
+            size_of_list *= 2;
+            mh_list = (ucp_mem_h *)realloc(mh_list, size_of_list * sizeof(ucp_mem_h));
+        }
+        mh_list[count_mh++] = mh;


make sure all cases are perfectly identical logic (double list size etc), try to create a macro if possible

what isnt identical, I'm not sure to understand

lappazos · 2024-08-22T14:19:15Z

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

+        }
+        mh_list[count_mh++] = mh;
+    }
+


https://github.com/openucx/ucc/blob/master/src/components/tl/ucp/allgather/allgather_knomial.c#L97

lappazos · 2024-08-25T11:09:27Z

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

-    task->allgather_kn.etask = NULL;
+                   task->allgather_kn.etask_linked_list_head->etask);
+
+    // task->allgather_kn.etask_linked_list_head = NULL;


lappazos · 2024-08-25T11:17:02Z

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

@@ -59,38 +63,51 @@ void ucc_tl_ucp_allgather_knomial_progress(ucc_coll_task_t *coll_task)
    size_t                 extra_count;

    EXEC_TASK_TEST(UCC_KN_PHASE_INIT, "failed during ee task test",
-                   task->allgather_kn.etask);
-    task->allgather_kn.etask = NULL;


a BUG - you can't remove it. its needed

lappazos · 2024-08-25T11:18:45Z

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

                          task, out);
+            if (count_mh >= max_count){


there is a function ucc_assert for that look what its doing

why u chose to out that check here?

should I just raise an error maybe ? How ?

if I use ucc_assert, I can't add my print right ?

lappazos · 2024-08-25T11:19:02Z

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

                          task, out);
+            if (count_mh >= max_count){
+                printf("count_mh is bigger than it should (%d >= %d).", count_mh, max_count);
+                goto out;


not good here

you mean even with ucc_assert ?

lappazos · 2024-08-25T11:19:50Z

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

                task, out);
+            if (count_mh >= max_count){


same as above

lappazos · 2024-08-25T11:26:37Z

src/components/tl/ucp/tl_ucp_context.c

+        status = ucc_coll_task_get_executor(&task->super, &exec);
+        if (ucc_unlikely(status != UCC_OK)) {
+            task->super.status = status;
+            return -1;


maybe u can simply return uuc_status? check its values, it's enum so it's int eventually

lappazos · 2024-08-25T11:26:53Z

src/components/tl/ucp/tl_ucp_context.c

+
+        if (ucc_unlikely(status != UCC_OK)) {
+            task->super.status = status;
+            return -1;


same as above

lappazos · 2024-08-25T11:26:58Z

src/components/tl/ucp/tl_ucp_context.c

+        task->allgather_kn.etask_linked_list_head = new_node;
+
+        task->allgather_kn.etask_linked_list_head->etask->completion = completion;
+        return 1;


same as above

lappazos · 2024-08-25T11:27:39Z

src/components/tl/ucp/tl_ucp_context.c

+    status = ucc_ee_executor_task_test(etask);
+    while (status>0) {
+        status = ucc_ee_executor_task_test(etask);
+        // if (ucc_unlikely(status < 0)) {                                        


why not remove?

I added it now

lappazos · 2024-08-25T11:28:33Z

src/schedule/ucc_schedule.h

@@ -7,6 +7,7 @@
 #ifndef UCC_SCHEDULE_H_
 #define UCC_SCHEDULE_H_

+#include <ucp/api/ucp.h>


is it really needed?

I think so for the ucp_mem_map

check again

lappazos · 2024-08-27T06:59:36Z

src/components/tl/ucp/tl_ucp_coll.h

+    ucp_mem_h      *mh_list;
+    int             count_mh;


add both to allgather

Why ? Is it a bug ?

lappazos · 2024-08-27T07:14:01Z

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

@@ -243,12 +371,18 @@ ucc_status_t ucc_tl_ucp_allgather_knomial_init_r(
    ucc_sbgp_t        *sbgp;

    task = ucc_tl_ucp_init_task(coll_args, team);
+    ucc_mpool_init(&task->allgather_kn.etask_node_mpool, 0, sizeof(node_ucc_ee_executor_task_t),


u didn't check for status

what should I do if status<0 ?

lappazos · 2024-08-27T07:14:25Z

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

    if (tl_team->cfg.use_reordering &&
        coll_args->args.coll_type == UCC_COLL_TYPE_ALLREDUCE) {
        sbgp = ucc_topo_get_sbgp(tl_team->topo, UCC_SBGP_FULL_HOST_ORDERED);
        task->subset.myrank = sbgp->group_rank;
        task->subset.map    = sbgp->map;
    }
+    register_memory(&task->super);


u didn't check for status

lappazos · 2024-08-27T07:14:59Z

src/components/tl/ucp/tl_ucp_coll.h

+#define MEM_MAP() do {                                                              \
+    status = ucp_mem_map(ctx->worker.ucp_context, &mmap_params, &mh);               \
+    if (UCC_OK != status) {                                                         \
+        task->super.status = status;                                                \


no, just return status and error

ucc/src/components/tl/nccl/tl_nccl_context.c

Line 196 in 2359644

tl_error(self->super.super.lib,

I should keep this self->super.super.lib or it's something else for here ?

lappazos · 2024-08-27T07:15:38Z

src/components/tl/ucp/tl_ucp_coll.h

            ucc_rank_t              recv_dist;
+            ucc_mpool_t             etask_node_mpool;
+            ucc_ee_executor_task_t *etask;


move back to original place

lappazos · 2024-08-27T08:07:45Z

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

    }
 UCC_KN_PHASE_PROXY:
-    if (UCC_INPROGRESS == ucc_tl_ucp_test(task)) {
+    if (UCC_INPROGRESS == ucc_tl_ucp_test_with_etasks(task)) {
        SAVE_STATE(UCC_KN_PHASE_PROXY);
        return;
    }


add in out
|ucc_assert(count_mh == max_count) or smth similar

lappazos · 2024-08-27T08:10:03Z

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

@@ -50,6 +51,9 @@ void ucc_tl_ucp_allgather_knomial_progress(ucc_coll_task_t *coll_task)
                                       args->root : 0;
    ucc_rank_t             rank      = VRANK(task->subset.myrank, broot, size);
    size_t                 local     = GET_LOCAL_COUNT(args, size, rank);
+    ucp_mem_h             *mh_list   = task->mh_list;
+    int                    max_count = task->count_mh;
+    int                    count_mh     = 0;


BUG - this is a mistake, because u want to keep it when u go in and out from progress

right but so how do I initialize it ?

lappazos · 2024-08-27T08:11:09Z

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

@@ -60,37 +64,42 @@ void ucc_tl_ucp_allgather_knomial_progress(ucc_coll_task_t *coll_task)

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


lappazos · 2024-08-27T08:12:00Z

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

@@ -193,7 +205,6 @@ ucc_status_t ucc_tl_ucp_allgather_knomial_start(ucc_coll_task_t *coll_task)

    UCC_TL_UCP_PROFILE_REQUEST_EVENT(coll_task, "ucp_allgather_kn_start", 0);
    ucc_tl_ucp_task_reset(task, UCC_INPROGRESS);
-    task->allgather_kn.etask = NULL;


BUG - why removed?

lappazos · 2024-08-27T08:12:33Z

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

+
+    if (KN_NODE_EXTRA == node_type) {
+        if (p->type != KN_PATTERN_ALLGATHERX) {
+


no need for all of the line spaces

jeffnvidia added 7 commits June 26, 2024 14:54

create function register memory

c6d49ef

before review of eyal

43faffd

first fixes of building errors

0441b48

sketch of the callback (missing the way to transmit task)

a22f47c

for loop over tasks for memcpy_device_start

44d21ea

first full compiling version

e14d2b1

rename ambiguous variables

8ef02c3

change name for better understanding and fixed first running errors

3417737

lappazos reviewed Aug 21, 2024

View reviewed changes

lappazos reviewed Aug 22, 2024

View reviewed changes

fix all comments, still on same bug

6589983

lappazos reviewed Aug 25, 2024

View reviewed changes

jeffnvidia changed the title ~~Register memory~~ DRAFT : Register memory Aug 25, 2024

fix comments, still bug on ucc_tl_ucp_send_nb_with_mem

4cbdc7e

lappazos suggested changes Aug 27, 2024

View reviewed changes

jeffnvidia added 2 commits August 27, 2024 18:13

fix all of the comments, still same bug

00a9097

working version on IL1 with Cuda

3ca742c

jeffnvidia force-pushed the register_memory branch from 6142008 to 3ca742c Compare September 19, 2024 14:45

		@@ -60,37 +64,42 @@ void ucc_tl_ucp_allgather_knomial_progress(ucc_coll_task_t *coll_task)

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


		if (KN_NODE_EXTRA == node_type) {
		if (p->type != KN_PATTERN_ALLGATHERX) {

DRAFT : Register memory #1010

Are you sure you want to change the base?

DRAFT : Register memory #1010

Conversation

jeffnvidia commented Aug 19, 2024

What

Why ?

swx-jenkins3 commented Aug 19, 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

lappazos Aug 22, 2024 • 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

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

lappazos Aug 22, 2024 •

edited

Loading