This commit fixes a bug on how to deal with the potential if a 'dependent'
communicator that we created has a lower CID than the parent comm. This can happen when using the hierarch collective communication module or for inter-communicators (since we make a duplicate of the original communicator). This is not a problem as long as the user calls MPI_Comm_free on the parent communicator. However, if the communicators are not freed by the user but released by Open MPI in MPI_Finalize, we walk through the list of still available communicators and free them one by one. Thus, local_comm is freed before the actual inter-communicator. However, the local_comm pointer in the inter communicator will still contain the 'previous' address of the local_comm and thus this will lead to a segmentation violation. In order to prevent that from happening, we increase the reference counter local_comm by one if its CID is lower than the parent. We cannot increase however its reference counter if the CID of local_comm is larger than the CID of the inter communicators, since a regular MPI_Comm_free would leave in that the case the local_comm hanging around and thus we would not recycle CID's properly, which was the reason and the cause for this trouble. This commit fixes tickets 2094 and 2166. Note however, that I want to close them manually, since a slightly different patch is required for the 1.4 series. This commit will have to be applied for the 1.5 series. And I will need a volunteer to review it. This commit was SVN r22671.
Этот коммит содержится в:
Edgar Gabriel
родитель
548d6f7c61
Коммит
61dee816db
@ -9,7 +9,7 @@
|
||||
* University of Stuttgart. All rights reserved.
|
||||
* Copyright (c) 2004-2005 The Regents of the University of California.
|
||||
* All rights reserved.
|
||||
* Copyright (c) 2007-2009 University of Houston. All rights reserved.
|
||||
* Copyright (c) 2007-2010 University of Houston. All rights reserved.
|
||||
* Copyright (c) 2007-2008 Cisco Systems, Inc. All rights reserved.
|
||||
* Copyright (c) 2009 Sun Microsystems, Inc. All rights reserved.
|
||||
* $COPYRIGHT$
|
||||
@ -578,6 +578,15 @@ int ompi_comm_split ( ompi_communicator_t* comm, int color, int key,
|
||||
snprintf(newcomp->c_name, MPI_MAX_OBJECT_NAME, "MPI COMMUNICATOR %d SPLIT FROM %d",
|
||||
newcomp->c_contextid, comm->c_contextid );
|
||||
|
||||
/* set the rank to MPI_UNDEFINED. This prevents in comm_activate
|
||||
* the collective module selection for a communicator that will
|
||||
* be freed anyway.
|
||||
*/
|
||||
if ( MPI_UNDEFINED == color ) {
|
||||
newcomp->c_local_group->grp_my_rank = MPI_UNDEFINED;
|
||||
}
|
||||
|
||||
|
||||
/* Activate the communicator and init coll-component */
|
||||
rc = ompi_comm_activate( &newcomp, /* new communicator */
|
||||
comm,
|
||||
@ -942,6 +951,9 @@ static int ompi_comm_allgather_emulate_intra( void *inbuf, int incount,
|
||||
int ompi_comm_free ( ompi_communicator_t **comm )
|
||||
{
|
||||
int ret;
|
||||
int cid = (*comm)->c_contextid;
|
||||
int is_internal = OMPI_COMM_IS_INTERNAL(*comm);
|
||||
|
||||
|
||||
/* Release attributes. We do this now instead of during the
|
||||
communicator destructor for 2 reasons:
|
||||
@ -987,6 +999,30 @@ int ompi_comm_free ( ompi_communicator_t **comm )
|
||||
}
|
||||
OBJ_RELEASE ( (*comm) );
|
||||
|
||||
if ( is_internal) {
|
||||
/* This communicator has been marked as an internal
|
||||
* communicator. This can happen if a communicator creates
|
||||
* 'dependent' subcommunicators (e.g. for inter
|
||||
* communicators or when using hierarch collective
|
||||
* module *and* the cid of the dependent communicator
|
||||
* turned out to be lower than of the parent one.
|
||||
* In that case, the reference counter has been increased
|
||||
* by one more, in order to handle the scenario,
|
||||
* that the user did not free the communicator.
|
||||
* Note, that if we enter this routine, we can
|
||||
* decrease the counter by one more therefore. However,
|
||||
* in ompi_comm_finalize, we only used OBJ_RELEASE instead
|
||||
* of ompi_comm_free(), and the increased reference counter
|
||||
* makes sure that the pointer to the dependent communicator
|
||||
* still contains a valid object.
|
||||
*/
|
||||
ompi_communicator_t *tmpcomm = opal_pointer_array_get_item(&ompi_mpi_communicators, cid);
|
||||
if ( NULL != tmpcomm ){
|
||||
OBJ_RELEASE (tmpcomm);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
*comm = MPI_COMM_NULL;
|
||||
return OMPI_SUCCESS;
|
||||
}
|
||||
|
||||
@ -469,6 +469,32 @@ int ompi_comm_activate ( ompi_communicator_t** newcomm,
|
||||
if (OMPI_SUCCESS != (ret = mca_coll_base_comm_select(*newcomm))) {
|
||||
goto bail_on_error;
|
||||
}
|
||||
|
||||
/* For an inter communicator, we have to deal with the potential
|
||||
* problem of what is happening if the local_comm that we created
|
||||
* has a lower CID than the parent comm. This is not a problem
|
||||
* as long as the user calls MPI_Comm_free on the inter communicator.
|
||||
* However, if the communicators are not freed by the user but released
|
||||
* by Open MPI in MPI_Finalize, we walk through the list of still available
|
||||
* communicators and free them one by one. Thus, local_comm is freed before
|
||||
* the actual inter-communicator. However, the local_comm pointer in the
|
||||
* inter communicator will still contain the 'previous' address of the local_comm
|
||||
* and thus this will lead to a segmentation violation. In order to prevent
|
||||
* that from happening, we increase the reference counter local_comm
|
||||
* by one if its CID is lower than the parent. We cannot increase however
|
||||
* its reference counter if the CID of local_comm is larger than
|
||||
* the CID of the inter communicators, since a regular MPI_Comm_free would
|
||||
* leave in that the case the local_comm hanging around and thus we would not
|
||||
* recycle CID's properly, which was the reason and the cause for this trouble.
|
||||
*/
|
||||
if ( OMPI_COMM_IS_INTER(*newcomm)) {
|
||||
if ( OMPI_COMM_CID_IS_LOWER(*newcomm, comm)) {
|
||||
OMPI_COMM_SET_INTERNAL (*newcomm);
|
||||
OBJ_RETAIN (*newcomm);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
return OMPI_SUCCESS;
|
||||
|
||||
bail_on_error:
|
||||
|
||||
@ -256,11 +256,23 @@ int ompi_comm_finalize(void)
|
||||
comm=(ompi_communicator_t *)opal_pointer_array_get_item(&ompi_mpi_communicators, i);
|
||||
if ( NULL != comm ) {
|
||||
/* Still here ? */
|
||||
if ( ompi_debug_show_handle_leaks && !(OMPI_COMM_IS_FREED(comm)) ){
|
||||
opal_output(0,"WARNING: MPI_Comm still allocated in MPI_Finalize\n");
|
||||
ompi_comm_dump ( comm);
|
||||
OBJ_RELEASE(comm);
|
||||
}
|
||||
if ( !OMPI_COMM_IS_INTERNAL(comm)) {
|
||||
|
||||
/* For communicator that have been marked as internal, we do not further
|
||||
* enforce to decrease the reference counter once more. These internal
|
||||
* communicators created e.g. by the hierarch or inter module did increase
|
||||
* the reference count by one more than other communicators, on order to
|
||||
* allow for deallocation with the parent communicator. Note, that
|
||||
* this only occurs if the cid of the local_comm is lower than of its
|
||||
* parent communicator. Read the comment in comm_activate for
|
||||
* a full explanation.
|
||||
*/
|
||||
if ( ompi_debug_show_handle_leaks && !(OMPI_COMM_IS_FREED(comm)) ){
|
||||
opal_output(0,"WARNING: MPI_Comm still allocated in MPI_Finalize\n");
|
||||
ompi_comm_dump ( comm);
|
||||
OBJ_RELEASE(comm);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@ -47,6 +47,7 @@ OMPI_DECLSPEC OBJ_CLASS_DECLARATION(ompi_communicator_t);
|
||||
#define OMPI_COMM_DYNAMIC 0x00000040
|
||||
#define OMPI_COMM_INVALID 0x00000080
|
||||
#define OMPI_COMM_PML_ADDED 0x00000100
|
||||
#define OMPI_COMM_INTERNAL 0x00000200
|
||||
|
||||
/* some utility #defines */
|
||||
#define OMPI_COMM_IS_INTER(comm) ((comm)->c_flags & OMPI_COMM_INTER)
|
||||
@ -58,11 +59,13 @@ OMPI_DECLSPEC OBJ_CLASS_DECLARATION(ompi_communicator_t);
|
||||
#define OMPI_COMM_IS_DYNAMIC(comm) ((comm)->c_flags & OMPI_COMM_DYNAMIC)
|
||||
#define OMPI_COMM_IS_INVALID(comm) ((comm)->c_flags & OMPI_COMM_INVALID)
|
||||
#define OMPI_COMM_IS_PML_ADDED(comm) ((comm)->c_flags & OMPI_COMM_PML_ADDED)
|
||||
#define OMPI_COMM_IS_INTERNAL(comm) ((comm)->c_flags & OMPI_COMM_INTERNAL)
|
||||
|
||||
#define OMPI_COMM_SET_DYNAMIC(comm) ((comm)->c_flags |= OMPI_COMM_DYNAMIC)
|
||||
#define OMPI_COMM_SET_INVALID(comm) ((comm)->c_flags |= OMPI_COMM_INVALID)
|
||||
|
||||
#define OMPI_COMM_SET_PML_ADDED(comm) ((comm)->c_flags |= OMPI_COMM_PML_ADDED)
|
||||
#define OMPI_COMM_SET_INTERNAL(comm) ((comm)->c_flags |= OMPI_COMM_INTERNAL)
|
||||
|
||||
/* a set of special tags: */
|
||||
|
||||
@ -91,6 +94,10 @@ OMPI_DECLSPEC OBJ_CLASS_DECLARATION(ompi_communicator_t);
|
||||
#define OMPI_COMM_BLOCK_WORLD 16
|
||||
#define OMPI_COMM_BLOCK_OTHERS 8
|
||||
|
||||
/* A macro comparing two CIDs */
|
||||
#define OMPI_COMM_CID_IS_LOWER(comm1,comm2) ( ((comm1)->c_contextid < (comm2)->c_contextid)? 1:0)
|
||||
|
||||
|
||||
OMPI_DECLSPEC extern opal_pointer_array_t ompi_mpi_communicators;
|
||||
|
||||
struct ompi_communicator_t {
|
||||
|
||||
@ -268,6 +268,14 @@ int mca_coll_hierarch_module_enable (mca_coll_base_module_t *module,
|
||||
if ( OMPI_SUCCESS != ret ) {
|
||||
goto exit;
|
||||
}
|
||||
if ( OMPI_COMM_CID_IS_LOWER ( lcomm, comm ) ) {
|
||||
/* Mark the communicator as 'interna' and increase the
|
||||
reference count by one more. See ompi_comm_activate
|
||||
for detailed comments
|
||||
*/
|
||||
OMPI_COMM_SET_INTERNAL (lcomm);
|
||||
OBJ_RETAIN(lcomm);
|
||||
}
|
||||
|
||||
hierarch_module->hier_comm = comm;
|
||||
hierarch_module->hier_lcomm = lcomm;
|
||||
@ -298,10 +306,24 @@ int mca_coll_hierarch_module_enable (mca_coll_base_module_t *module,
|
||||
/* Generate the lleader communicator assuming that all lleaders are the first
|
||||
process in the list of processes with the same color. A function generating
|
||||
other lleader-comms will follow soon. */
|
||||
ret = ompi_comm_split ( comm, llead->am_lleader, rank, &llcomm, 0);
|
||||
color = MPI_UNDEFINED;
|
||||
if ( llead->am_lleader ) {
|
||||
color = 1;
|
||||
}
|
||||
ret = ompi_comm_split ( comm, color, rank, &llcomm, 0);
|
||||
if ( OMPI_SUCCESS != ret ) {
|
||||
goto exit;
|
||||
}
|
||||
if ( OMPI_COMM_CID_IS_LOWER ( llcomm, comm ) ) {
|
||||
/* Mark the communicator as 'internal' and increase the
|
||||
reference count by one more. See ompi_comm_activate
|
||||
for detailed explanation.
|
||||
*/
|
||||
OMPI_COMM_SET_INTERNAL (llcomm);
|
||||
OBJ_RETAIN(llcomm);
|
||||
}
|
||||
|
||||
|
||||
llead->llcomm = llcomm;
|
||||
|
||||
/* Store it now on the data structure */
|
||||
@ -375,7 +397,7 @@ int mca_coll_hierarch_get_all_lleaders ( int rank, mca_coll_hierarch_module_t *h
|
||||
llead->my_lleader = MPI_UNDEFINED;
|
||||
}
|
||||
else {
|
||||
llead->am_lleader = MPI_UNDEFINED;
|
||||
llead->am_lleader = 0;
|
||||
for ( i=0; i< hierarch_module->hier_num_lleaders; i++ ) {
|
||||
if ( hierarch_module->hier_llr[i] == mycolor ) {
|
||||
llead->my_lleader = cntarr[i]-1;
|
||||
@ -446,6 +468,7 @@ struct ompi_communicator_t* mca_coll_hierarch_get_llcomm (int root,
|
||||
struct mca_coll_hierarch_llead_t *llead=NULL;
|
||||
int found, i, rc, num_llead, offset;
|
||||
int rank = ompi_comm_rank (hierarch_module->hier_comm);
|
||||
int color;
|
||||
|
||||
/* determine what our offset of root is in the colorarr */
|
||||
offset = mca_coll_hierarch_get_offset ( root,
|
||||
@ -484,12 +507,25 @@ struct ompi_communicator_t* mca_coll_hierarch_get_llcomm (int root,
|
||||
|
||||
/* generate the list of lleaders with this offset */
|
||||
mca_coll_hierarch_get_all_lleaders ( rank, hierarch_module, llead, offset );
|
||||
|
||||
color = MPI_UNDEFINED;
|
||||
if ( llead->am_lleader ) {
|
||||
color = 1;
|
||||
}
|
||||
|
||||
/* create new lleader subcommunicator */
|
||||
rc = ompi_comm_split ( hierarch_module->hier_comm, llead->am_lleader, root, &llcomm, 0);
|
||||
rc = ompi_comm_split ( hierarch_module->hier_comm, color, root, &llcomm, 0);
|
||||
if ( OMPI_SUCCESS != rc ) {
|
||||
return NULL;
|
||||
}
|
||||
if ( OMPI_COMM_CID_IS_LOWER ( llcomm, hierarch_module->hier_comm ) ) {
|
||||
/* Mark the communicator as 'interna' and increase the
|
||||
reference count by one more. See ompi_comm_activate
|
||||
for detailed explanation. */
|
||||
OMPI_COMM_SET_INTERNAL (llcomm);
|
||||
OBJ_RETAIN(llcomm);
|
||||
}
|
||||
|
||||
|
||||
llead->llcomm = llcomm;
|
||||
|
||||
/* Store the new element on the hierarch_module struct */
|
||||
|
||||
Загрузка…
x
Ссылка в новой задаче
Block a user