comm/cid: fix for edge case with intercomm creation #12465

hppritcha · 2024-04-11T22:21:45Z

A barrier was removed from the cid allocation algorithm for intercommunicators that leads to random hangs when doing MPI_Intercomm_create or moral equivalent.

See issue #12367

the cid allocation algorithm and comm from cid lookup was modified to use a pointer to ompi_mpi_comm_null as the placeholder in the list of pointers to ompi commucators.

A convenience debug function was added to comm_cid.c to dump out this table of communicators.

bosilca

ompi_mpi_comm_null as a sentinel should work better than using the parent communicator. If you remove the ompi_comm_dump_communicators function this should be ready to go.

bosilca · 2024-04-11T22:58:52Z

ompi/communicator/comm_cid.c

@@ -1589,3 +1589,16 @@ static int ompi_comm_ft_allreduce_intra_pmix_nb(int *inbuf, int *outbuf, int cou

 #endif /* OPAL_ENABLE_FT_MPI */

+void ompi_comm_dump_communicators(void)


We have specialized mechanisms to dump information, especially communicators. This function does not follow any of OMPI coding standards, such as not using printf and friends but relying on OPAL_OUTPUT_VERBOSE.

np i'll remove from the pr

bosilca · 2024-04-11T23:34:27Z

Conceptually this is not correct because ompi_mpi_comm_null can legitimately be added to the ompi_mpi_communicators list. In the current code it will work because the only time we extract it from the list we use opal_pointer_array_get_item instead of ompi_comm_lookup.

good point. i'll see if there's a not-to-painful way to address this.

hppritcha · 2024-04-11T23:54:44Z

Conceptually this is not correct because ompi_mpi_comm_null can legitimately be added to the ompi_mpi_communicators list. In the current code it will work because the only time we extract it from the list we use opal_pointer_array_get_item instead of ompi_comm_lookup.

good point. i'll see if there's a not-to-painful way to address this.

okay so if &ompi_mpi_comm_null is added to the list - which we actually do by default to support sessions - how is this relevant to the logic in the ob1 pml? The whole issue revolves around ob1 pml getting a comm back from doing an lookup on the incoming hdr's ctx and then thinking - oh I have a valid comm to use! we don't want that.

bosilca · 2024-04-12T05:16:56Z

I understand the problem, we need a sentinel to put in the ompi_mpi_communicators. What I'm arguing here is that ompi_mpi_comm_null is a valid communicator and already belongs in that list, but the proposed code cannot identify such cases because it willingly replace a return of ompi_mpi_comm_null by NULL, even in the valid case.

We could solve this by defining OMPI_COMM_SENTINEL to 0x00001, and using this instead of ompi_mpi_comm_null. This makes the code much clear and also allows the ompi_comm_lookup function to correctly return ompi_mpi_comm_null.

hppritcha · 2024-04-12T16:19:27Z

I see. I think the sentinel idea should work. I'll update the PR with that approach

bosilca · 2024-04-13T16:39:50Z

I was going over this this morning and I realized this code had and has a big flaw. The original code would have break the the resilience support because we go over the complete list of communicators to identify those that contains the failed processes to update their status and set in motion the PROC_FAILED and the automatic REVOKE. That code is not set to encounter the same communicator twice, which means that any errors that would have triggered while a communicator creation was underway would have lead to badness.

The current code has now a different flaw. We use the ompi_mpi_communicators list via two accessors: opal_pointer_array_get_item and ompi_comm_lookup. The PR protects the ompi_comm_lookup path by checking for the special value, but there is no protection for the 6 uses of opal_pointer_array_get_item and opal_pointer_array_test_and_set_item.

hppritcha · 2024-04-13T23:23:55Z

I will look into a fix for this next week.

hppritcha · 2024-04-15T21:50:17Z

@wenduwan @bosilca some updates.

wenduwan · 2024-04-15T21:55:28Z

Run AWS CI again.

wenduwan · 2024-04-15T22:23:08Z

ompi/communicator/communicator.h

@@ -140,6 +140,12 @@ OMPI_DECLSPEC OBJ_CLASS_DECLARATION(ompi_communicator_t);
 #define OMPI_COMM_BLOCK_WORLD      16
 #define OMPI_COMM_BLOCK_OTHERS     8

+/**
+ * Placeholder to use in array of ompi communicators during CID allocation 
+ *


nit extra line

wenduwan · 2024-04-15T22:23:23Z

ompi/communicator/comm_init.c

        if ( NULL != comm ) {
            /* Communicator has not been freed before finalize */
            OBJ_RELEASE(comm);
-            comm=(ompi_communicator_t *)opal_pointer_array_get_item(&ompi_mpi_communicators, i);
+            comm= ompi_comm_lookup(i);


nit space around =

A barrier was removed from the cid allocation algorithm for intercommunicators that leads to random hangs when doing MPI_Intercomm_create or moral equivalent. See issue open-mpi#12367 the cid allocation algorithm and comm from cid lookup was modified to use OMPI_COMM_SENTINEL as the placeholder in the list of pointers to ompi commucators. Signed-off-by: Howard Pritchard <[email protected]>

PR open-mpi#12465 gave Intel OneAPI a headache and threw an error trying to compile the code. This patch fixes that. Signed-off-by: Howard Pritchard <[email protected]>

dalcinl · 2024-04-18T10:29:29Z

I believe that the fixes from here solved some of the issues related to dynamic process management, in particular accept/connect.

PR open-mpi#12465 gave Intel OneAPI a headache and threw an error trying to compile the code. This patch fixes that. Signed-off-by: Howard Pritchard <[email protected]> (cherry picked from commit 574778d)

github-actions bot added the Target: main label Apr 11, 2024

hppritcha requested a review from wenduwan April 11, 2024 23:25

wenduwan added the mpi4py-all Run the optional mpi4py CI tests label Apr 11, 2024

wenduwan force-pushed the fix_for_intercomm_cid_alloc branch from 9399be3 to c31f714 Compare April 11, 2024 23:28

bosilca reviewed Apr 11, 2024

View reviewed changes

wenduwan removed the mpi4py-all Run the optional mpi4py CI tests label Apr 11, 2024

hppritcha force-pushed the fix_for_intercomm_cid_alloc branch 2 times, most recently from 0725601 to 85993fe Compare April 12, 2024 17:38

hppritcha force-pushed the fix_for_intercomm_cid_alloc branch from 85993fe to 7dcdbd2 Compare April 15, 2024 21:47

bosilca approved these changes Apr 15, 2024

View reviewed changes

wenduwan approved these changes Apr 15, 2024

View reviewed changes

hppritcha force-pushed the fix_for_intercomm_cid_alloc branch from 7dcdbd2 to 15a3d26 Compare April 16, 2024 15:31

hppritcha merged commit ae7fe9a into open-mpi:main Apr 16, 2024
14 checks passed

wenduwan mentioned this pull request Apr 17, 2024

comm/cid: keep Intel OneAPI compiler happy #12474

Merged

hppritcha mentioned this pull request Apr 17, 2024

mpi4py: Regression in alltoall with intercommunicators #12367

Closed

dalcinl mentioned this pull request Apr 22, 2024

v5.0.x: mpi4py: Run accept/connect tests unconditionally #12486

Merged

hppritcha mentioned this pull request Jun 25, 2024

[v5.0.x] 3rd-party: bump prrte,pmix submodules #12626

Merged

		@@ -1589,3 +1589,16 @@ static int ompi_comm_ft_allreduce_intra_pmix_nb(int inbuf, int outbuf, int cou

		#endif /* OPAL_ENABLE_FT_MPI */

		void ompi_comm_dump_communicators(void)

comm/cid: fix for edge case with intercomm creation #12465

comm/cid: fix for edge case with intercomm creation #12465

Uh oh!

Conversation

hppritcha commented Apr 11, 2024

Uh oh!

bosilca left a comment

Choose a reason for hiding this comment

Uh oh!

bosilca Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

hppritcha Apr 11, 2024

Choose a reason for hiding this comment

Uh oh!

bosilca commented Apr 11, 2024 • edited by hppritcha Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hppritcha commented Apr 11, 2024

Uh oh!

bosilca commented Apr 12, 2024

Uh oh!

hppritcha commented Apr 12, 2024

Uh oh!

bosilca commented Apr 13, 2024

Uh oh!

hppritcha commented Apr 13, 2024

Uh oh!

hppritcha commented Apr 15, 2024

Uh oh!

wenduwan commented Apr 15, 2024

Uh oh!

wenduwan Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

hppritcha Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

wenduwan Apr 15, 2024

Choose a reason for hiding this comment

Uh oh!

hppritcha Apr 16, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dalcinl commented Apr 18, 2024

Uh oh!

Uh oh!

bosilca commented Apr 11, 2024 •

edited by hppritcha

Loading