DRAFT : Register memory by jeffnvidia · Pull Request #1010 · openucx/ucc

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


+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

    }                                                                          \
 } while(0)

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


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

+    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

+                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

+        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

+    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

+    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

+                            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

+
+            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

+        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

+        }
+        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

-    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


    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

-                                             team, task),
+                                             team, task, mh_list[count_mh++]),
                          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

                          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

-                                   team, task),
+                                   team, task, mh_list[count_mh++]),
                task, out);
+            if (count_mh >= max_count){


same as above

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

+        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

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


same as above

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

+        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

+    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

 #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

+    ucp_mem_h      *mh_list;
+    int             count_mh;


add both to allgather

Why ? Is it a bug ?

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

    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

        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

+#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

+            node_ucc_ee_executor_task_t *etask_linked_list_head;
            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

+    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

    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


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


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


    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

+
+    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

initialize p at the right place

01879f9


		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) {

Conversation

jeffnvidia commented Aug 19, 2024

What

Why ?

Uh oh!

swx-jenkins3 commented Aug 19, 2024

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!

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

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

lappazos Aug 22, 2024 •

edited

Loading