From 07a75ee9c7b0dc79cd9e8cd7c68172c077d5f5b3 Mon Sep 17 00:00:00 2001 From: Hui Zhou Date: Fri, 20 Dec 2024 23:46:40 -0600 Subject: [PATCH] comm: avoid creating invalid intercomm then destroy it In MPIR_Comm_create_inter, we know whether the remote group is empty after the exchange, thus it is unnecessary to create and commit the intercomm then delete it later. Simply don't create it in the first place. The device layer is not necessarily equipped to handle intercomm commit with empty groups. --- src/mpi/comm/comm_impl.c | 86 +++++++++++++++++----------------------- 1 file changed, 37 insertions(+), 49 deletions(-) diff --git a/src/mpi/comm/comm_impl.c b/src/mpi/comm/comm_impl.c index b98e8a46559..a8a1429b8a7 100644 --- a/src/mpi/comm/comm_impl.c +++ b/src/mpi/comm/comm_impl.c @@ -409,31 +409,6 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co mpi_errno = MPII_Comm_create_calculate_mapping(group_ptr, comm_ptr, &mapping, &mapping_comm); MPIR_ERR_CHECK(mpi_errno); - *newcomm_ptr = NULL; - - if (group_ptr->rank != MPI_UNDEFINED) { - /* Get the new communicator structure and context id */ - mpi_errno = MPIR_Comm_create(newcomm_ptr); - if (mpi_errno) - goto fn_fail; - - (*newcomm_ptr)->recvcontext_id = new_context_id; - (*newcomm_ptr)->rank = group_ptr->rank; - (*newcomm_ptr)->comm_kind = comm_ptr->comm_kind; - /* Since the group has been provided, let the new communicator know - * about the group */ - (*newcomm_ptr)->local_comm = 0; - (*newcomm_ptr)->local_group = group_ptr; - MPIR_Group_add_ref(group_ptr); - - (*newcomm_ptr)->local_size = group_ptr->size; - (*newcomm_ptr)->remote_group = 0; - - (*newcomm_ptr)->is_low_group = comm_ptr->is_low_group; - - MPIR_Comm_set_session_ptr(*newcomm_ptr, session_ptr); - } - /* There is an additional step. We must communicate the * information on the local context id and the group members, * given by the ranks so that the remote process can construct the @@ -456,9 +431,6 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co rinfo, 2, MPI_INT, 0, 0, comm_ptr, MPI_STATUS_IGNORE, MPIR_ERR_NONE); MPIR_ERR_CHECK(mpi_errno); - if (*newcomm_ptr != NULL) { - (*newcomm_ptr)->context_id = rinfo[0]; - } remote_size = rinfo[1]; MPIR_CHKLMEM_MALLOC(remote_mapping, int *, @@ -482,9 +454,7 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co /* Broadcast to the other members of the local group */ mpi_errno = MPIR_Bcast(rinfo, 2, MPI_INT, 0, comm_ptr->local_comm, MPIR_ERR_NONE); MPIR_ERR_CHECK(mpi_errno); - if (*newcomm_ptr != NULL) { - (*newcomm_ptr)->context_id = rinfo[0]; - } + remote_size = rinfo[1]; MPIR_CHKLMEM_MALLOC(remote_mapping, int *, remote_size * sizeof(int), @@ -495,10 +465,45 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co } MPIR_Assert(remote_size >= 0); + if (group_ptr->rank == MPI_UNDEFINED || remote_size <= 0) { + /* If we are not part of the group, or - + * It's possible that no members of the other side of comm were + * members of the group that they passed, which we only know after + * receiving/bcasting the remote_size above. We must return + * MPI_COMM_NULL in this case. + */ + MPIR_Free_contextid(new_context_id); + *newcomm_ptr = NULL; + goto fn_exit; + } + + /* FIXME: the branch was kept to minimize line changes. Remove the if-check. */ + if (group_ptr->rank != MPI_UNDEFINED) { + /* Get the new communicator structure and context id */ + mpi_errno = MPIR_Comm_create(newcomm_ptr); + if (mpi_errno) + goto fn_fail; + (*newcomm_ptr)->context_id = rinfo[0]; + (*newcomm_ptr)->remote_size = rinfo[1]; + (*newcomm_ptr)->recvcontext_id = new_context_id; + (*newcomm_ptr)->rank = group_ptr->rank; + (*newcomm_ptr)->comm_kind = comm_ptr->comm_kind; + /* Since the group has been provided, let the new communicator know + * about the group */ + (*newcomm_ptr)->local_comm = 0; + (*newcomm_ptr)->local_group = group_ptr; + MPIR_Group_add_ref(group_ptr); + + (*newcomm_ptr)->local_size = group_ptr->size; + (*newcomm_ptr)->remote_group = 0; + + (*newcomm_ptr)->is_low_group = comm_ptr->is_low_group; + + MPIR_Comm_set_session_ptr(*newcomm_ptr, session_ptr); + } if (group_ptr->rank != MPI_UNDEFINED) { - (*newcomm_ptr)->remote_size = remote_size; /* Now, everyone has the remote_mapping, and can apply that to * the network address mapping. */ @@ -528,23 +533,6 @@ int MPIR_Comm_create_inter(MPIR_Comm * comm_ptr, MPIR_Group * group_ptr, MPIR_Co (*newcomm_ptr)->tainted = comm_ptr->tainted; mpi_errno = MPIR_Comm_commit(*newcomm_ptr); MPIR_ERR_CHECK(mpi_errno); - - if (remote_size <= 0) { - /* It's possible that no members of the other side of comm were - * members of the group that they passed, which we only know after - * receiving/bcasting the remote_size above. We must return - * MPI_COMM_NULL in this case, but we can't free the newcomm_ptr - * immediately after the communication above because - * MPIR_Comm_release won't work correctly with a half-constructed - * comm. */ - mpi_errno = MPIR_Comm_release(*newcomm_ptr); - MPIR_ERR_CHECK(mpi_errno); - *newcomm_ptr = NULL; - } - } else { - /* This process is not in the group */ - MPIR_Free_contextid(new_context_id); - *newcomm_ptr = NULL; } fn_exit: