OSHMEM/MCA/SSHMEM/UCX: DEVICE_NIC_MEM hint - implementation should use RDMA memory type #11866

roiedanino · 2023-08-21T08:08:42Z

Updated the implementation for using the DEVICE_NIC_MEM to use ucp_mem_map with the new RDMA memory type.
Cleanup for old code which used uct api

github-actions · 2023-08-21T08:09:24Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

33a0760: Removed device mem

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

oshmem/mca/sshmem/ucx/sshmem_ucx.h

yosefe · 2023-08-21T14:34:04Z

oshmem/mca/sshmem/ucx/configure.m4

+                #include <ucp/api/ucp.h>
+          ]],
+          [[
+                ucs_memory_type_t mem_type = UCS_MEMORY_TYPE_RDMA;


can just AC_CHECK_DECLS([UCS_MEMORY_TYPE_RDMA], ...)

yosefe · 2023-08-21T14:34:52Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

@@ -176,19 +171,39 @@ segment_create_internal(map_segment_t *ds_buf, void *address, size_t size,
    return rc;
 }

+static int 
+segment_create_host_mem(map_segment_t *ds_buf, size_t size, unsigned flags) {


IMO we don't need this wrapper

yosefe · 2023-08-21T14:35:10Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

+        status = segment_create_internal(ds_buf, NULL, size, flags, UCS_MEMORY_TYPE_RDMA);
+        if (status == OSHMEM_SUCCESS) {
+            ds_buf->alloc_hints   = hint;
+            if (hint) {


why needed?

oshmem/mca/sshmem/ucx/configure.m4

yosefe · 2023-08-21T15:14:13Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

    }
+    return segment_create_internal(ds_buf, mca_sshmem_base_start_address, size, flags, UCS_MEMORY_TYPE_HOST);


line too long

yosefe · 2023-08-21T15:14:20Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

-                uct_ib_md_release_device_mem(dev_mem);
-                /* fallback to regular allocation */
-            }
+        status = segment_create_internal(ds_buf, NULL, size, flags, UCS_MEMORY_TYPE_RDMA);


line seems too long

github-actions · 2023-08-22T09:49:16Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

f1671c2: Fixed CR - using DECLS auto generated macros

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

yosefe

pls squash

yosefe

@gleon99 can you pls take a look as well?

gleon99 · 2023-08-23T14:00:58Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c


-#if HAVE_UCX_DEVICE_MEM
-    int ret = OSHMEM_ERROR;
+#if HAVE_DECL_UCS_MEMORY_TYPE_RDMA


Please add an indicative debug message for the user, in case one had passed a hint which was not applied eventually. It can happen in 3 scenarios:

NIC_MEM passed but the #if is false.

NIC_MEM passed but create_internal failed -> fallback to host memory..

Unknown / invalid "hint" was passed.

github-actions · 2023-08-24T08:33:10Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

2fe0dcd: MCA/SSHMEM/UCX: Added warning messages for fallbac...

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

gleon99 · 2023-08-24T09:26:10Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

        /* Fallback - Try again using host memory*/
    }
+#else
+    SSHMEM_WARN("UCX version is too old and won't support "


Are you sure? It would be printed independent of the hint, even if 0..

gleon99 · 2023-08-24T09:27:06Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

@@ -189,8 +189,18 @@ segment_create(map_segment_t *ds_buf,
            ds_buf->allocator   = &sshmem_ucx_allocator;
            return OSHMEM_SUCCESS;
        }
+        else if(hint) {


If includes the possibility of hint == SHMEM_HINT_DEVICE_NIC_MEM. What's the reasoning?

gleon99 · 2023-08-24T15:49:18Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

-        SSHMEM_WARN("UCX version is too old and won't support "
-                    "SHMEM_HINT_DEVICE_NIC_MEM hint, fallback to host memory");
+#else 
+        SSHMEM_WARN("UCX is not supporting SHMEM_HINT_DEVICE_NIC_MEM "


does not support

SSHMEM_VERBOSE(1, "DEVICE_NIC_MEM hint ignored since UCX does not support MEMORY_TYPE_RDMA")

gleon99 · 2023-08-24T15:50:56Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

 static int
 segment_create(map_segment_t *ds_buf,
               const char *file_name,
               size_t size, long hint)
 {
    mca_spml_ucx_t *spml = (mca_spml_ucx_t*)mca_spml.self;
-    unsigned flags;
+    unsigned flags       = UCP_MEM_MAP_ALLOCATE;
+    int status           = OSHMEM_SUCCESS;


Seems this init value is not used in any scenario?

gleon99 · 2023-08-24T15:53:22Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

    }
+    if (hint & ~SHMEM_HINT_DEVICE_NIC_MEM) {
+        SSHMEM_WARN("Hint was not recognized therefore ignored, "


Better used unknown/invalid instead of "was not recognized.

Print the passed hint and the "allowed" hints.

IMO we don't need to print any error in this case

yosefe · 2023-08-26T07:51:31Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

-        SSHMEM_WARN("UCX version is too old and won't support "
-                    "SHMEM_HINT_DEVICE_NIC_MEM hint, fallback to host memory");
+#else 
+        SSHMEM_WARN("UCX is not supporting SHMEM_HINT_DEVICE_NIC_MEM "


SSHMEM_VERBOSE(1, "DEVICE_NIC_MEM hint ignored since UCX does not support MEMORY_TYPE_RDMA")

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

yosefe · 2023-08-26T07:54:00Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

    }
+    if (hint & ~SHMEM_HINT_DEVICE_NIC_MEM) {
+        SSHMEM_WARN("Hint was not recognized therefore ignored, "


IMO we don't need to print any error in this case

github-actions · 2023-08-29T11:16:40Z

Hello! The Git Commit Checker CI bot found a few problems with this PR:

1f8f040: SSHMEM/UCX: fixed logging

check_signed_off: does not contain a valid Signed-off-by line

Please fix these problems and, if necessary, force-push new commits back up to the PR branch. Thanks!

yosefe · 2023-08-29T11:28:54Z

opal/mca/common/ucx/common_ucx.h

-                    MCA_COMMON_UCX_VERBOSE(1, "%s failed: %d, %s", (_msg) ? (_msg) : __func__, \
-                                           UCS_PTR_STATUS(_request),                           \
-                                           ucs_status_string(UCS_PTR_STATUS(_request)));       \
+                    MCA_COMMON_UCX_VERBOSE(1, "%s failed2: %d, %s", (_msg) ? (_msg) : __func__,\


yosefe · 2023-08-29T15:54:38Z

@roiedanino next time pls don't force push

gleon99 · 2023-09-03T16:31:40Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c


    status = ucp_mem_map(spml->ucp_context, &mem_map_params, &mem_h);
    if (UCS_OK != status) {
-        SSHMEM_ERROR("ucp_mem_map() failed: %s\n", ucs_status_string(status));
+        SSHMEM_ERROR("Failed to allocate DEVICE_NIC_MEM"


I'm not sure if this error msg is indicative.
In this particular context, (in theory) it might not necessarily be only about DEVICE_NIC_MEM...

Suggested change

SSHMEM_ERROR("Failed to allocate DEVICE_NIC_MEM"

SSHMEM_ERROR("ucp_mem_map() failed: %s %s\n",

ucs_status_string(status),

mem_type == UCS_MEMORY_TYPE_RDMA ?

"failed to allocate DEVICE_NIC_MEM, falls back to host memory" : "");

Maybe that way?

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

yosefe · 2023-09-07T08:53:15Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

+        } else {
+            SSHMEM_WARN("Failed to allocate DEVICE_NIC_MEM: %s, "
+                        "fallback to host memory", ucs_status_string(status));


remove this

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

yosefe · 2023-09-07T10:08:42Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

@@ -123,7 +123,9 @@ segment_create_internal(map_segment_t *ds_buf, void *address, size_t size,

    status = ucp_mem_map(spml->ucp_context, &mem_map_params, &mem_h);
    if (UCS_OK != status) {
-        SSHMEM_ERROR("ucp_mem_map() failed: %s\n", ucs_status_string(status));
+        SSHMEM_VERBOSE(err_level, "ucp_mem_map(%s) failed: %s\n", 


memory_type=%s

yosefe

pls squash

yosefe · 2023-09-12T11:51:04Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

        if (status == OSHMEM_SUCCESS) {
            ds_buf->alloc_hints = hint;
            ds_buf->allocator   = &sshmem_ucx_allocator;
            return OSHMEM_SUCCESS;
        }
 #else 
-       SSHMEM_VERBOSE(1, "DEVICE_NIC_MEM hint ignored since UCX does not "
+       SSHMEM_VERBOSE(20, "DEVICE_NIC_MEM hint ignored since UCX does not "


use level 3

yosefe · 2023-09-12T11:52:02Z

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c

-            return OSHMEM_ERR_BAD_PARAM;
+#if HAVE_DECL_UCS_MEMORY_TYPE_RDMA
+        status = segment_create_internal(ds_buf, NULL, size, flags,
+                                         UCS_MEMORY_TYPE_RDMA, 20);


use level 3 instead of 20

type Signed-off-by: Roie Danino <[email protected]> Added a fallback for rdma allocation failure - allocating host memory instead Signed-off-by: Roie Danino <[email protected]>

gleon99

@yosefe can we merge this one?

github-actions bot added the Target: main label Aug 21, 2023

roiedanino force-pushed the minor-fixes-for-memic-support branch from 33a0760 to 946a131 Compare August 21, 2023 08:17

yosefe reviewed Aug 21, 2023

View reviewed changes

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c Outdated Show resolved Hide resolved

oshmem/mca/sshmem/ucx/sshmem_ucx_module.c Outdated Show resolved Hide resolved

oshmem/mca/sshmem/ucx/sshmem_ucx.h Outdated Show resolved Hide resolved

roiedanino force-pushed the minor-fixes-for-memic-support branch from 5dffffe to e853c37 Compare August 21, 2023 11:17

yosefe reviewed Aug 21, 2023

View reviewed changes

roiedanino force-pushed the minor-fixes-for-memic-support branch from f1671c2 to 4d16347 Compare August 22, 2023 09:58

roiedanino requested a review from yosefe August 22, 2023 10:15

roiedanino force-pushed the minor-fixes-for-memic-support branch from 4d16347 to 64e931b Compare August 22, 2023 12:28

yosefe approved these changes Aug 23, 2023

View reviewed changes

roiedanino force-pushed the minor-fixes-for-memic-support branch from 64e931b to 2858a93 Compare August 23, 2023 08:22

yosefe approved these changes Aug 23, 2023

View reviewed changes

roiedanino mentioned this pull request Aug 23, 2023

UCT/IB: Added support for device memory allocation (memic/rdma) openucx/ucx#9308

Merged

gleon99 reviewed Aug 23, 2023

View reviewed changes

roiedanino force-pushed the minor-fixes-for-memic-support branch 2 times, most recently from 5456bf4 to e37a850 Compare August 24, 2023 09:09

gleon99 reviewed Aug 24, 2023

View reviewed changes

roiedanino requested a review from gleon99 August 24, 2023 12:59

gleon99 reviewed Aug 24, 2023

View reviewed changes

yosefe reviewed Aug 26, 2023

View reviewed changes

yosefe reviewed Aug 29, 2023

View reviewed changes

roiedanino force-pushed the minor-fixes-for-memic-support branch from 1f8f040 to ce61ab5 Compare August 29, 2023 14:58

roiedanino requested a review from gleon99 August 29, 2023 14:59

yosefe approved these changes Aug 29, 2023

View reviewed changes

gleon99 reviewed Sep 3, 2023

View reviewed changes

roiedanino requested review from gleon99 and yosefe September 7, 2023 07:55

roiedanino force-pushed the minor-fixes-for-memic-support branch from eab79be to 1577cec Compare September 7, 2023 08:50

yosefe reviewed Sep 7, 2023

View reviewed changes

yosefe approved these changes Sep 7, 2023

View reviewed changes

gleon99 approved these changes Sep 7, 2023

View reviewed changes

roiedanino force-pushed the minor-fixes-for-memic-support branch from 0f18386 to 7aa1f28 Compare September 7, 2023 15:15

yosefe reviewed Sep 12, 2023

View reviewed changes

yosefe approved these changes Sep 12, 2023

View reviewed changes

SHMEM/MCA/SSHMEM/UCX: Fixing DEVICE_NIC_MEM support to use RDMA memory

b192a78

type Signed-off-by: Roie Danino <[email protected]> Added a fallback for rdma allocation failure - allocating host memory instead Signed-off-by: Roie Danino <[email protected]>

roiedanino force-pushed the minor-fixes-for-memic-support branch from 6131002 to b192a78 Compare September 12, 2023 12:40

gleon99 approved these changes Oct 1, 2023

View reviewed changes

yosefe merged commit 3de90b1 into open-mpi:main Oct 8, 2023
8 checks passed

roiedanino mentioned this pull request Oct 8, 2023

v4.1.x - SSHMEM/UCX: DEVICE_NIC_MEM hint - implementation should use RDMA memory type #11972

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSHMEM/MCA/SSHMEM/UCX: DEVICE_NIC_MEM hint - implementation should use RDMA memory type #11866

OSHMEM/MCA/SSHMEM/UCX: DEVICE_NIC_MEM hint - implementation should use RDMA memory type #11866

roiedanino commented Aug 21, 2023

github-actions bot commented Aug 21, 2023

yosefe Aug 21, 2023

yosefe Aug 21, 2023

yosefe Aug 21, 2023

yosefe Aug 21, 2023

yosefe Aug 21, 2023

github-actions bot commented Aug 22, 2023

yosefe left a comment

yosefe left a comment

gleon99 Aug 23, 2023

github-actions bot commented Aug 24, 2023

gleon99 Aug 24, 2023

gleon99 Aug 24, 2023

gleon99 Aug 24, 2023

yosefe Aug 26, 2023

gleon99 Aug 24, 2023

gleon99 Aug 24, 2023

yosefe Aug 26, 2023

yosefe Aug 26, 2023

yosefe Aug 26, 2023

github-actions bot commented Aug 29, 2023

yosefe Aug 29, 2023

yosefe commented Aug 29, 2023

gleon99 Sep 3, 2023 •

edited

Loading

roiedanino Sep 6, 2023

yosefe Sep 7, 2023

yosefe Sep 7, 2023

yosefe left a comment

yosefe Sep 12, 2023

yosefe Sep 12, 2023

gleon99 left a comment

		}
		return segment_create_internal(ds_buf, mca_sshmem_base_start_address, size, flags, UCS_MEMORY_TYPE_HOST);

-        SSHMEM_ERROR("Failed to allocate DEVICE_NIC_MEM"
+        SSHMEM_ERROR("ucp_mem_map() failed: %s %s\n",
+                     ucs_status_string(status),
+                     mem_type == UCS_MEMORY_TYPE_RDMA ?
+                     "failed to allocate DEVICE_NIC_MEM, falls back to host memory" : "");

OSHMEM/MCA/SSHMEM/UCX: DEVICE_NIC_MEM hint - implementation should use RDMA memory type #11866

OSHMEM/MCA/SSHMEM/UCX: DEVICE_NIC_MEM hint - implementation should use RDMA memory type #11866

Conversation

roiedanino commented Aug 21, 2023

github-actions bot commented Aug 21, 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

github-actions bot commented Aug 22, 2023

yosefe left a comment

Choose a reason for hiding this comment

yosefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Aug 24, 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

github-actions bot commented Aug 29, 2023

Choose a reason for hiding this comment

yosefe commented Aug 29, 2023

gleon99 Sep 3, 2023 • 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

yosefe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gleon99 left a comment

Choose a reason for hiding this comment

gleon99 Sep 3, 2023 •

edited

Loading