ompi/info: introduce support for the mpi_memory_alloc_kinds info object (II) #13055

edgargabriel · 2025-01-27T15:53:06Z

This PR introduces support for the mpi_memory_alloc_kinds and mpi_assert_memory_alloc_kinds info objects as defined in the MPI 4.1 document and in the side document specifying the values for the three supported accelerator kinds.
The logic is surprisingly twisted and I would not be surprised if some of the code here would have to be revised over time.

There are two ways on how the user can specify the mpi_memory_alloc_kinds info object: either as a runtime argument to mpiexec, namely -memory-alloc-kinds, or as part of the info-object passed to MPI_Session_init. There are two predefined memor-alloc-kinds, namely system and mpi, the latter having three potential restrictors. When user retrieves the info object e.g. using MPI_Comm_get_info on MPI_COMM_WORLD, the MPI library is allowed to return more memory-kinds than requested by the user.

This freedom has been applied in the following manner here:

if the user does not request the system and/or mpi memory-alloc-kinds, we add them to the list of provided memkinds
if the user does not request one of the GPU memory kinds (cuda, ze, rocm), we do not add them to the list of supported memkinds, even if the library has been compiled with GPU support and it would be available. The goal is in a second commit to allow to use this mechanism to turn off GPU support in the library if support for GPU memory has not been requested by the user. This commit doesn't do that yet however.
All unknown/unsupported memkinds are removed and not returned when retrieving the info object.

This default provided memkind is applied to all new {Comm/File/Window} objects, unless the user sets mpi_assert_memory_alloc_kinds during object creation (e.g. MPI_Comm_dup_with_info or MPI_File_open). The user can restrict the memory kinds supported by the object with mpi_assert_memory_alloc_kinds, i.e. setting this info object will influence mpi_memory_alloc_kinds on that object. (I am not sure whether there is another example in the MPI spec where providing one info object influences the value of another info object, but the MPI 4.1 specification is pretty clear in my understanding that this is what is expected to happen.)

Another difference in the handling of mpi_assert_memory_alloc_kinds is that if a provided value is not recognized, the entire mpi_assert_memory_alloc_kinds is dropped/ignored, not just the unrecognized memkind itself. (This is my reading of the specification on what is expected to happen).

Lastly, there could probably be some discussion whether the location chosen for the majority of the code is appropriate (i.e. ompi/info), a directory which so-far has contained the code to manage the MPI_Info object in general. After some discussion I thought this is the most appropriate locations, since

this is MPI related know-how, i.e. it cannot go into opal
the code is general and has to be used by Communicators, Windows, Files, and Sessions/Instances

edgargabriel · 2025-01-27T18:51:21Z

As a side note, I do have a bunch of tests that I developed as part of the project, and we might need to find a good spot for where to put them. The tests do require some human interaction and the output is platform dependent, but its a good starting point in my opinion.

For the World model:

egabriel@amdhome:~/memkind-tests$ mpirun --mca coll ^hcoll --memory-alloc-kinds system,x:1,y:1,y:2,rocm:device,x:asd  -np 2 ./memkind_test
Comm: MPI_COMM_SELF rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_COMM_WORLD rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_COMM_SELF rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_Comm_dup rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_Comm_dup after Comm_set_info rank: 0 Value of memory_alloc_kind info object is rocm,system,mpi
Comm: MPI_Comm_dup_with_info & INFO_NULL rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_Comm_dup_with_info & info_assert rank: 0 Value of memory_alloc_kind info object is mpi
Comm: MPI_Comm_dup_with_info & info_assert rank: 0 Value of assert_memory_alloc_kind info object is mpi
Comm: MPI_Comm_dup of comm_dup'ed with info_assert rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_Comm_dup_with_info & info_invalid rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device

Comm: MPI_Comm_create rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device

Comm: MPI_Intercomm_create rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_Intercomm_create rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
Comm: MPI_Intercomm_merge rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device

File: MPI_File_open with INFO_NULL rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device
File: MPI_File_open with info_assert rank: 0 Value of memory_alloc_kind info object is mpi
File: MPI_File_open with info_invalid rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device

Win: MPI_Win_create rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm:device

and for Sessions:

egabriel@amdhome:~/memkind-tests$ mpirun --mca coll ^hcoll  -np 2 ./memkind_test_sessions
Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm
Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm
Comm: Comm_create_from_group with INFO_NULL rank: 0 Value of memory_alloc_kind info object is system,mpi:alloc_mem,mpi:win_allocate,mpi:win_allocate_shared,rocm
Comm: Comm_create_from_group with info_assert rank: 0 Value of memory_alloc_kind info object is mpi

hppritcha · 2025-01-27T18:53:04Z

Why does HCOLL need to be excluded?

edgargabriel · 2025-01-27T18:56:24Z

Why does HCOLL need to be excluded?

hcoll needs to be excluded if you compile with ROCm support, they don't seem to like each other

hppritcha · 2025-01-27T22:27:09Z

ompi/info/info_memkind.c

+    return num_unique;
+}
+
+#if 0


do we want to leave this debug code in the PR?

I would like to keep the code in there, I am 99.9% sure that we will have use for that in the near future. Furthermore, there are other components/code parts that also have dump functionality in the code base. The #if 0 is just to get rid of compiler warning about unused static function

edgargabriel · 2025-02-10T17:49:03Z

I have updated the PR based on the feedback that I received on the hybrid WG mailing list.
Specifically:

we now return all the memkinds requested by the user, even if they overlap. For example, before this updated patch, if the user requested "mpi,mpi:alloc_mem", we only returned "mpi" (since it was the superset that includes the second memkind requested). Now we return both strings requested by the user, since it seems like the language in MPI 4.1 does request this behavior explicitely.
the memkind value on derived communicators is now not derived from the parent communicator/object anymore, but from the instance. This change impacts communicators that were created on a parent communicator that had its memkind values changed freom the default one, e.g. MPI_Comm_dup_with_info + mpi_assert_memory_alloc_kind

The third item, namely clarification of the behavior of info objects passed to MPI_Comm_spawn and friends is still pending with the MPI Forum. I would suggest to not hold up this pr for that purpose, we can fix that retro-actively.

devreal · 2025-02-10T18:24:19Z

ompi/communicator/comm.c

+    newcomp->super.s_info = OBJ_NEW(opal_info_t);
+    ompi_info_memkind_copy_or_set (&comm->instance->super, &newcomp->super, info);
    if (info) {
-        newcomp->super.s_info = OBJ_NEW(opal_info_t);
        opal_info_dup(info, &(newcomp->super.s_info));


Should we dup the provided info before handling the memkind? Aren't we risking to overwrite the memkind info?

you are correct, I reverted the order

ompi/instance/instance.c

devreal · 2025-02-10T19:03:49Z

opal/mca/accelerator/null/accelerator_null_component.c

+static void mca_accelerator_cuda_ze_memkind (ompi_memkind_t *memkind)
+{
+  memkind->im_name = NULL;
+  memkind->im_no_restrictors = false;
+  memkind->im_num_restrictors = 0;
+
+  return;
+}


Suggested change

static void mca_accelerator_cuda_ze_memkind (ompi_memkind_t *memkind)

{

memkind->im_name = NULL;

memkind->im_no_restrictors = false;

memkind->im_num_restrictors = 0;

return;

}

static void mca_accelerator_null_memkind (ompi_memkind_t *memkind)

{

memkind->im_name = NULL;

memkind->im_no_restrictors = false;

memkind->im_num_restrictors = 0;

return;

}

We don't need to return system here, right?

I also don't see where this callback is used

argh, this (and your comment below for the ze component) were originally fixed in the version before I did the rebase on the wrong machine and the force-push lost that work. Anyway, it is fixed now again, thanks!

You are btw. correct that we register the get_memkind() method with the null component although it is not actually invoked from anyway, its just for symmetry/completeness

the 'system' and 'mpi' memkinds are set in the info_memkind.c, independent of the accelerator component used.

opal/mca/accelerator/ze/accelerator_ze_module.c

ompi/info/info_memkind.c

edgargabriel · 2025-02-10T22:06:35Z

@devreal thank you for your review, I think I incorporated all your requests/suggestions. Whenever you have a few minutes, can you please re-review?

devreal

Sorry, some more things I didn't notice earlier. I'm ok with not having an MCA parameter for now but I find having the undocumented environment variable strange.

devreal · 2025-02-11T01:54:46Z

ompi/info/info_memkind.c

+const char *ompi_info_memkind_cb (opal_infosubscriber_t *obj, const char *key, const char *value)
+{
+    return value;
+}


I think we should return NULL here so signal that the info value stored in the object has not changed. Returning value signals that we accepted the new value and that the subscriber mechanism will change the stored value.

I tried changing it to return NULL instead of value, in that case however it stops working, it looks like the callback is also invoked in the initial setting of the value (which doesn't seem to occur when I return NULL instead of the value)

Ahh yes, I forgot about the first time a value is set. How about testing for whether the value is set and rejecting any subsequent attempts? We don't want users to be able to change the key AFAIR.

Suggested change

const char *ompi_info_memkind_cb (opal_infosubscriber_t *obj, const char *key, const char *value)

{

return value;

}

/**

* Callback invoked by the info subscriber mechanism.

* Accepts only the first value set.

*/

const char *ompi_info_memkind_cb (opal_infosubscriber_t *obj, const char *key, const char *value)

{

int flag, valuelen;

opal_info_get_valuelen(obj->s_info, key, &valuelen, &flag);

return (flag && valuelen) ? NULL : value;

}

I am not sure this is correct either. If the user provided mpi-assert-memory-alloc-kinds, we calculate a new string 'value' for mpi-memory-alloc-kinds (e.g. at communicator creation) and want that value to be set, not what was provided (and would be available after the info_dup).

We had a chat with @devreal and I think we came up with the correct solution for this problem, change has been pushed and integrated into the PR

ompi/info/info_memkind.c

ompi/communicator/comm_init.c

add code to handle the memkind info objects defined in MPI 4.1 Signed-off-by: Edgar Gabriel <[email protected]>

devreal

LGTM, thanks!

edgargabriel · 2025-02-11T18:46:49Z

bot:retest

edgargabriel · 2025-02-11T18:52:36Z

/azp run

add an API to the accelerator component to retrieve the memory_alloc_kind information that is supported by the component. The values stored/returned are based on the side document that is about to be ratified, see https://github.com/mpi-forum/mem-alloc/blob/main/mem_alloc.tex Signed-off-by: Edgar Gabriel <[email protected]>

in intercomm_create_from_group, we need to set the grp_instance value for the leader_group. This issue was exposed with the new mmemkind code and the mpi4py testsuite. Signed-off-by: Edgar Gabriel <[email protected]>

edgargabriel added the ⚠️ WIP-DNM! label Jan 27, 2025

github-actions bot added the Target: main label Jan 27, 2025

edgargabriel force-pushed the topic/memkind-support branch 2 times, most recently from e91f38b to 9fe1afe Compare January 27, 2025 17:42

edgargabriel removed the ⚠️ WIP-DNM! label Jan 27, 2025

edgargabriel requested review from devreal and hppritcha January 27, 2025 18:06

hppritcha approved these changes Jan 27, 2025

View reviewed changes

edgargabriel force-pushed the topic/memkind-support branch 4 times, most recently from b495610 to 3314f1d Compare February 10, 2025 16:58

edgargabriel force-pushed the topic/memkind-support branch 2 times, most recently from f1d9f85 to 3f7fef1 Compare February 10, 2025 18:09

devreal reviewed Feb 10, 2025

View reviewed changes

edgargabriel force-pushed the topic/memkind-support branch 3 times, most recently from 084c609 to cdba5b2 Compare February 10, 2025 21:24

devreal reviewed Feb 11, 2025

View reviewed changes

edgargabriel force-pushed the topic/memkind-support branch 2 times, most recently from 9052fda to c2c408d Compare February 11, 2025 15:43

info/info_memkind: add code to handle memkind info

ddd0c2c

add code to handle the memkind info objects defined in MPI 4.1 Signed-off-by: Edgar Gabriel <[email protected]>

edgargabriel force-pushed the topic/memkind-support branch from c2c408d to 61f4b55 Compare February 11, 2025 16:04

devreal approved these changes Feb 11, 2025

View reviewed changes

edgargabriel and others added 2 commits February 11, 2025 13:21

ompi/communicator: set grp_instance for leader_group

e901e6a

in intercomm_create_from_group, we need to set the grp_instance value for the leader_group. This issue was exposed with the new mmemkind code and the mpi4py testsuite. Signed-off-by: Edgar Gabriel <[email protected]>

edgargabriel force-pushed the topic/memkind-support branch from 61f4b55 to e901e6a Compare February 11, 2025 19:22

edgargabriel merged commit 4f87c6b into open-mpi:main Feb 11, 2025
15 checks passed

jiaxiyan mentioned this pull request Feb 13, 2025

osu_latency tests with CUDA segfault after mpi_memory_alloc_kinds is introduced #13096

Closed

github-actions bot mentioned this pull request May 8, 2025

fortran: fix common symbol sizes and alignments #13230

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ompi/info: introduce support for the mpi_memory_alloc_kinds info object (II) #13055

ompi/info: introduce support for the mpi_memory_alloc_kinds info object (II) #13055

edgargabriel commented Jan 27, 2025 •

edited

Loading

edgargabriel commented Jan 27, 2025 •

edited

Loading

hppritcha commented Jan 27, 2025

edgargabriel commented Jan 27, 2025

hppritcha Jan 27, 2025

edgargabriel Feb 11, 2025

edgargabriel commented Feb 10, 2025

devreal Feb 10, 2025

edgargabriel Feb 10, 2025

devreal Feb 10, 2025

devreal Feb 10, 2025

edgargabriel Feb 10, 2025

edgargabriel Feb 10, 2025

edgargabriel commented Feb 10, 2025

devreal left a comment

devreal Feb 11, 2025

edgargabriel Feb 11, 2025

devreal Feb 11, 2025

edgargabriel Feb 11, 2025 •

edited

Loading

edgargabriel Feb 11, 2025

devreal left a comment

edgargabriel commented Feb 11, 2025

edgargabriel commented Feb 11, 2025

-const char *ompi_info_memkind_cb (opal_infosubscriber_t *obj, const char *key, const char *value)
-{
-    return value;
-}
+/**
+ * Callback invoked by the info subscriber mechanism.
+ * Accepts only the first value set.
+ */
+const char *ompi_info_memkind_cb (opal_infosubscriber_t *obj, const char *key, const char *value)
+{
+    int flag, valuelen;
+    opal_info_get_valuelen(obj->s_info, key, &valuelen, &flag);
+    return (flag && valuelen) ? NULL : value;
+}

ompi/info: introduce support for the mpi_memory_alloc_kinds info object (II) #13055

ompi/info: introduce support for the mpi_memory_alloc_kinds info object (II) #13055

Conversation

edgargabriel commented Jan 27, 2025 • edited Loading

edgargabriel commented Jan 27, 2025 • edited Loading

hppritcha commented Jan 27, 2025

edgargabriel commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edgargabriel commented Feb 10, 2025

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

edgargabriel commented Feb 10, 2025

devreal left a 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

edgargabriel Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devreal left a comment

Choose a reason for hiding this comment

edgargabriel commented Feb 11, 2025

edgargabriel commented Feb 11, 2025

edgargabriel commented Jan 27, 2025 •

edited

Loading

edgargabriel commented Jan 27, 2025 •

edited

Loading

edgargabriel Feb 11, 2025 •

edited

Loading