diff --git a/src/attribute/attribute.c b/src/attribute/attribute.c index 0f6abb4559..81c1fe9b24 100644 --- a/src/attribute/attribute.c +++ b/src/attribute/attribute.c @@ -1,12 +1,15 @@ -/** +/* * $HEADER$ */ +#include "ompi_config.h" + #include "attribute/attribute.h" #include "communicator/communicator.h" +#include "threads/mutex.h" #include "include/constants.h" -/** +/* * Macros */ @@ -61,7 +64,7 @@ /* - * Static + * Static */ static void ompi_attrkey_item_construct(ompi_attrkey_item_t *item); static void ompi_attrkey_item_destruct(ompi_attrkey_item_t *item); @@ -84,6 +87,16 @@ OBJ_CLASS_INSTANCE(ompi_attrkey_item_t, static ompi_hash_table_t *keyval_hash; static ompi_bitmap_t *key_bitmap; +/* + * Have one lock protect all access to any attribute stuff (keyval + * hash, key bitmap, attribute hashes on MPI objects, etc.). + * Arguably, we would have a finer-grained scheme (e.g., 2 locks) that + * would allow at least *some* concurrency, but these are attributes + * -- they're not in the performance-critical portions of the code. + * So why bother? + */ +static ompi_mutex_t alock; + /* * ompi_attrkey_item_t interface functions @@ -102,8 +115,10 @@ ompi_attrkey_item_destruct(ompi_attrkey_item_t *item) { /* Remove the key entry from the hash and free the key */ + OMPI_THREAD_LOCK(&alock); ompi_hash_table_remove_value_uint32(keyval_hash, item->key); FREE_KEY(item->key); + OMPI_THREAD_UNLOCK(&alock); } @@ -176,8 +191,10 @@ ompi_attr_create_keyval(ompi_attribute_type_t type, /* Create a new unique key and fill the hash */ + OMPI_THREAD_LOCK(&alock); *key = CREATE_KEY(); ret = ompi_hash_table_set_value_uint32(keyval_hash, *key, attr); + OMPI_THREAD_UNLOCK(&alock); if (OMPI_SUCCESS != ret) { return ret; } @@ -204,7 +221,7 @@ ompi_attr_create_keyval(ompi_attribute_type_t type, int -ompi_attr_free_keyval(ompi_attribute_type_t type, int *key, int predefined) +ompi_attr_free_keyval(ompi_attribute_type_t type, int *key, bool predefined) { ompi_attrkey_item_t *key_item; @@ -216,15 +233,17 @@ ompi_attr_free_keyval(ompi_attribute_type_t type, int *key, int predefined) /* Find the key-value pair */ + OMPI_THREAD_LOCK(&alock); key_item = (ompi_attrkey_item_t*) ompi_hash_table_get_value_uint32(keyval_hash, *key); + OMPI_THREAD_UNLOCK(&alock); if ((NULL == key_item) || (key_item->attr_type != type) || ((!predefined) && (key_item->attr_flag & OMPI_KEYVAL_PREDEFINED))) { return OMPI_ERR_BAD_PARAM; } - /* Not releasing the object here, it will be done in MPI_*_attr_delete */ + /* MPI says to set the returned value to MPI_KEYVAL_INVALID */ *key = MPI_KEYVAL_INVALID; @@ -241,7 +260,7 @@ ompi_attr_free_keyval(ompi_attribute_type_t type, int *key, int predefined) int ompi_attr_delete(ompi_attribute_type_t type, void *object, ompi_hash_table_t *keyhash, int key, - int predefined) + bool predefined, bool need_lock) { ompi_attrkey_item_t *key_item; int ret, err; @@ -253,19 +272,35 @@ ompi_attr_delete(ompi_attribute_type_t type, void *object, return MPI_ERR_INTERN; } + /* Note that this function can be invoked by + ompi_attr_delete_all() to set attributes on the new object (in + addition to the top-level MPI_* functions that set attributes). + In these cases, ompi_attr_delete_all() has already locked the + keyval_lock, so we should not try to lock it again. */ + + if (need_lock) { + OMPI_THREAD_LOCK(&alock); + } + /* Check if the key is valid in the key-attribute hash */ key_item = (ompi_attrkey_item_t*) ompi_hash_table_get_value_uint32(keyval_hash, key); - + if ((NULL == key_item) || (key_item->attr_type!= type) || ((!predefined) && (key_item->attr_flag & OMPI_KEYVAL_PREDEFINED))) { + if (need_lock) { + OMPI_THREAD_UNLOCK(&alock); + } return OMPI_ERR_BAD_PARAM; } /* Ensure that we don't have an empty keyhash */ if (NULL == keyhash) { + if (need_lock) { + OMPI_THREAD_UNLOCK(&alock); + } return OMPI_ERR_BAD_PARAM; } @@ -273,7 +308,6 @@ ompi_attr_delete(ompi_attribute_type_t type, void *object, yes, then delete the attribute and key entry from the CWD hash */ attr = ompi_hash_table_get_value_uint32(keyhash, key); - switch(type) { case COMM_ATTR: DELETE_ATTR_OBJECT(communicator, attr); @@ -293,6 +327,9 @@ ompi_attr_delete(ompi_attribute_type_t type, void *object, } ret = ompi_hash_table_remove_value_uint32(keyhash, key); + if (need_lock) { + OMPI_THREAD_UNLOCK(&alock); + } if (OMPI_SUCCESS != ret) { return ret; } @@ -309,7 +346,7 @@ ompi_attr_delete(ompi_attribute_type_t type, void *object, int ompi_attr_set(ompi_attribute_type_t type, void *object, ompi_hash_table_t **keyhash, int key, void *attribute, - int predefined) + bool predefined, bool need_lock) { ompi_attrkey_item_t *key_item; int ret, err; @@ -325,6 +362,15 @@ ompi_attr_set(ompi_attribute_type_t type, void *object, return MPI_ERR_INTERN; } + /* Note that this function can be invoked by ompi_attr_copy_all() + to set attributes on the new object (in addition to the + top-level MPI_* functions that set attributes). In these + cases, ompi_attr_copy_all() has already locked the keyval_lock, + so we should not try to lock it again. */ + + if (need_lock) { + OMPI_THREAD_LOCK(&alock); + } key_item = (ompi_attrkey_item_t *) ompi_hash_table_get_value_uint32(keyval_hash, key); @@ -332,6 +378,9 @@ ompi_attr_set(ompi_attribute_type_t type, void *object, if ((NULL == key_item) || (key_item->attr_type != type) || ((!predefined) && (key_item->attr_flag & OMPI_KEYVAL_PREDEFINED))) { + if (need_lock) { + OMPI_THREAD_UNLOCK(&alock); + } return OMPI_ERR_BAD_PARAM; } @@ -343,8 +392,8 @@ ompi_attr_set(ompi_attribute_type_t type, void *object, /* Now see if the key is present in the CWD object. If so, delete the old attribute in the key */ - oldattr = ompi_hash_table_get_value_uint32(*keyhash, key); + oldattr = ompi_hash_table_get_value_uint32(*keyhash, key); if (oldattr != NULL) { switch(type) { case COMM_ATTR: @@ -367,6 +416,9 @@ ompi_attr_set(ompi_attribute_type_t type, void *object, } ret = ompi_hash_table_set_value_uint32(*keyhash, key, attribute); + if (need_lock) { + OMPI_THREAD_UNLOCK(&alock); + } if (OMPI_SUCCESS != ret) { return ret; } @@ -394,10 +446,12 @@ ompi_attr_get(ompi_hash_table_t *keyhash, int key, void *attribute, flag argument */ *flag = 0; + OMPI_THREAD_LOCK(&alock); key_item = (ompi_attrkey_item_t *) ompi_hash_table_get_value_uint32(keyval_hash, key); if (NULL == key_item) { + OMPI_THREAD_UNLOCK(&alock); return MPI_KEYVAL_INVALID; } @@ -405,10 +459,12 @@ ompi_attr_get(ompi_hash_table_t *keyhash, int key, void *attribute, been cached on this object yet. So just return *flag = 0. */ if (NULL == keyhash) { + OMPI_THREAD_UNLOCK(&alock); return MPI_SUCCESS; } attr = ompi_hash_table_get_value_uint32(keyhash, key); + OMPI_THREAD_UNLOCK(&alock); if (NULL != attr) { *((void **) attribute) = attr; *flag = 1; @@ -443,6 +499,12 @@ ompi_attr_copy_all(ompi_attribute_type_t type, void *old_object, return MPI_SUCCESS; } + /* Lock this whole sequence of events -- don't let any other + thread modify the structure of the keyval hash or bitmap while + we're traversing it */ + + OMPI_THREAD_LOCK(&alock); + /* Get the first key-attr in the CWD hash */ ret = ompi_hash_table_get_first_key_uint32(oldkeyhash, &key, &old_attr, &node); @@ -478,23 +540,27 @@ ompi_attr_copy_all(ompi_attribute_type_t type, void *old_object, /* Hang this off the new CWD object */ - /* VPS: predefined is set to 1, so that no comparison is done - for prdefined at all and it just falls off the error - checking loop in attr_set */ + /* The "predefined" parameter to ompi_attr_set() is set to 1, + so that no comparison is done for prdefined at all and it + just falls off the error checking loop in attr_set */ /* VPS: we pass the address of new_attr in here, I am assuming that new_attr should have actually been a double pointer in the copy fn, but since its a pointer in that MPI specs, we need to pass *new_attr here */ - if (flag == 1) { + if (1 == flag) { ompi_attr_set(type, new_object, &newkeyhash, key, - new_attr, 1); + new_attr, true, false); } ret = ompi_hash_table_get_next_key_uint32(oldkeyhash, &key, &old_attr, in_node, &node); } + + /* All done */ + + OMPI_THREAD_UNLOCK(&alock); return MPI_SUCCESS; } @@ -515,10 +581,16 @@ ompi_attr_delete_all(ompi_attribute_type_t type, void *object, /* Ensure that the table is not empty */ - if (NULL != keyhash) { + if (NULL == keyhash) { return MPI_SUCCESS; } + /* Lock this whole sequence of events -- don't let any other + thread modify the structure of the keyval hash or bitmap while + we're traversing it */ + + OMPI_THREAD_LOCK(&alock); + /* Get the first key in local CWD hash */ ret = ompi_hash_table_get_first_key_uint32(keyhash, &key, &old_attr, @@ -538,8 +610,11 @@ ompi_attr_delete_all(ompi_attribute_type_t type, void *object, in_node, &node); /* Now delete this attribute */ - ompi_attr_delete(type, object, keyhash, oldkey, 1); + ompi_attr_delete(type, object, keyhash, oldkey, true, false); } - + + /* All done */ + + OMPI_THREAD_UNLOCK(&alock); return MPI_SUCCESS; } diff --git a/src/attribute/attribute.h b/src/attribute/attribute.h index d8a2cdfef8..c954485632 100644 --- a/src/attribute/attribute.h +++ b/src/attribute/attribute.h @@ -171,7 +171,8 @@ int ompi_attr_create_keyval(ompi_attribute_type_t type, * @return OMPI error code */ -int ompi_attr_free_keyval(ompi_attribute_type_t type, int *key, int predefined); +int ompi_attr_free_keyval(ompi_attribute_type_t type, int *key, + bool predefined); /** * Set an attribute on the comm/win/datatype @@ -181,16 +182,22 @@ int ompi_attr_free_keyval(ompi_attribute_type_t type, int *key, int predefined); * @param key Key val for the attribute (IN) * @param attribute The actual attribute pointer (IN) * @param predefined Whether the key is predefined or not 0/1 (IN) + * @param need_lock Whether we need to need to lock the keyval_lock or not * @return OMPI error code * * If (*keyhash) == NULL, a new keyhash will be created and * initialized. * + * Note that need_lock should *always* be true when this function is + * invoked from an top-level MPI function. It is only false when this + * function is invoked internally (i.e., when we already hold the + * relevant locks, and we don't want to try to lock them again, + * recursively). */ int ompi_attr_set(ompi_attribute_type_t type, void *object, ompi_hash_table_t **keyhash, - int key, void *attribute, int predefined); + int key, void *attribute, bool predefined, bool need_lock); /** * Get an attribute on the comm/win/datatype @@ -214,13 +221,19 @@ int ompi_attr_get(ompi_hash_table_t *keyhash, int key, * @param keyhash The attribute hash table hanging on the object(IN) * @param key Key val for the attribute (IN) * @param predefined Whether the key is predefined or not 0/1 (IN) + * @param need_lock Whether we need to need to lock the keyval_lock or not * @return OMPI error code * + * Note that need_lock should *always* be true when this function is + * invoked from an top-level MPI function. It is only false when this + * function is invoked internally (i.e., when we already hold the + * relevant locks, and we don't want to try to lock them again, + * recursively). */ int ompi_attr_delete(ompi_attribute_type_t type, void *object, - ompi_hash_table_t *keyhash , int key, - int predefined); + ompi_hash_table_t *keyhash , int key, + bool predefined, bool need_lock); /** diff --git a/src/attribute/attribute_predefined.c b/src/attribute/attribute_predefined.c index d917229892..46914dbe37 100644 --- a/src/attribute/attribute_predefined.c +++ b/src/attribute/attribute_predefined.c @@ -167,7 +167,7 @@ static int set(int target_keyval, void *value) return err; } err = ompi_attr_set(COMM_ATTR, MPI_COMM_WORLD, - &MPI_COMM_WORLD->c_keyhash, keyval, value, 1); + &MPI_COMM_WORLD->c_keyhash, keyval, value, true, true); if (OMPI_SUCCESS != err) { return err; } diff --git a/src/communicator/comm.c b/src/communicator/comm.c index 3421039164..1f53dfc0fb 100644 --- a/src/communicator/comm.c +++ b/src/communicator/comm.c @@ -678,13 +678,6 @@ static int ompi_comm_allgather_emulate_intra( void *inbuf, int incount, */ int ompi_comm_free ( ompi_communicator_t **comm ) { - - /* Release attributes */ - if (NULL != (*comm)->c_keyhash) { - ompi_attr_delete_all ( COMM_ATTR, (*comm), (*comm)->c_keyhash ); - OBJ_RELEASE((*comm)->c_keyhash); - } - /* Release the communicator */ OBJ_RELEASE ( (*comm) ); diff --git a/src/communicator/comm_init.c b/src/communicator/comm_init.c index cb37b3a24a..4d0bffdd3c 100644 --- a/src/communicator/comm_init.c +++ b/src/communicator/comm_init.c @@ -276,6 +276,13 @@ static void ompi_comm_construct(ompi_communicator_t* comm) static void ompi_comm_destruct(ompi_communicator_t* comm) { + /* Release attributes */ + + if (NULL != comm->c_keyhash) { + ompi_attr_delete_all(COMM_ATTR, comm, comm->c_keyhash); + OBJ_RELEASE(comm->c_keyhash); + } + /* Release the collective module */ mca_coll_base_comm_unselect(comm); diff --git a/src/mpi/c/attr_delete.c b/src/mpi/c/attr_delete.c index 370c295ea9..3667c244a8 100644 --- a/src/mpi/c/attr_delete.c +++ b/src/mpi/c/attr_delete.c @@ -32,7 +32,8 @@ int MPI_Attr_delete(MPI_Comm comm, int keyval) } } - ret = ompi_attr_delete(COMM_ATTR, comm, comm->c_keyhash, keyval, 0); + ret = ompi_attr_delete(COMM_ATTR, comm, comm->c_keyhash, keyval, + false, true); OMPI_ERRHANDLER_RETURN(ret, comm, MPI_ERR_OTHER, FUNC_NAME); } diff --git a/src/mpi/c/attr_put.c b/src/mpi/c/attr_put.c index 5b1b09a32c..6b6f139fb6 100644 --- a/src/mpi/c/attr_put.c +++ b/src/mpi/c/attr_put.c @@ -32,7 +32,7 @@ int MPI_Attr_put(MPI_Comm comm, int keyval, void *attribute_val) } ret = ompi_attr_set(COMM_ATTR, comm, &comm->c_keyhash, - keyval, attribute_val, 0); + keyval, attribute_val, false, true); OMPI_ERRHANDLER_RETURN(ret, comm, MPI_ERR_OTHER, FUNC_NAME); } diff --git a/src/mpi/c/comm_delete_attr.c b/src/mpi/c/comm_delete_attr.c index b22664c11f..9ca01fc207 100644 --- a/src/mpi/c/comm_delete_attr.c +++ b/src/mpi/c/comm_delete_attr.c @@ -31,7 +31,8 @@ int MPI_Comm_delete_attr(MPI_Comm comm, int comm_keyval) } } - ret = ompi_attr_delete(COMM_ATTR, comm, comm->c_keyhash, comm_keyval, 0); + ret = ompi_attr_delete(COMM_ATTR, comm, comm->c_keyhash, comm_keyval, + false, true); OMPI_ERRHANDLER_RETURN(ret, comm, MPI_ERR_OTHER, FUNC_NAME); } diff --git a/src/mpi/c/comm_set_attr.c b/src/mpi/c/comm_set_attr.c index 7dbdfa3ae1..64dafb29d9 100644 --- a/src/mpi/c/comm_set_attr.c +++ b/src/mpi/c/comm_set_attr.c @@ -32,6 +32,6 @@ int MPI_Comm_set_attr(MPI_Comm comm, int comm_keyval, void *attribute_val) } ret = ompi_attr_set(COMM_ATTR, comm, &comm->c_keyhash, - comm_keyval, attribute_val, 0); + comm_keyval, attribute_val, false, true); OMPI_ERRHANDLER_RETURN(ret, comm, MPI_ERR_OTHER, FUNC_NAME); } diff --git a/src/mpi/c/type_delete_attr.c b/src/mpi/c/type_delete_attr.c index fb037a895b..f13ed57664 100644 --- a/src/mpi/c/type_delete_attr.c +++ b/src/mpi/c/type_delete_attr.c @@ -32,7 +32,8 @@ int MPI_Type_delete_attr (MPI_Datatype type, int type_keyval) } } - ret = ompi_attr_delete(TYPE_ATTR, type, type->d_keyhash, type_keyval, 0); + ret = ompi_attr_delete(TYPE_ATTR, type, type->d_keyhash, type_keyval, + false, true); OMPI_ERRHANDLER_RETURN(ret, MPI_COMM_WORLD, MPI_ERR_OTHER, FUNC_NAME); } diff --git a/src/mpi/c/type_set_attr.c b/src/mpi/c/type_set_attr.c index cc0215e1a8..524218069e 100644 --- a/src/mpi/c/type_set_attr.c +++ b/src/mpi/c/type_set_attr.c @@ -35,7 +35,7 @@ int MPI_Type_set_attr (MPI_Datatype type, } ret = ompi_attr_set(TYPE_ATTR, type, &type->d_keyhash, - type_keyval, attribute_val, 0); + type_keyval, attribute_val, false, true); OMPI_ERRHANDLER_RETURN(ret, MPI_COMM_WORLD, MPI_ERR_OTHER, FUNC_NAME); diff --git a/src/mpi/c/win_delete_attr.c b/src/mpi/c/win_delete_attr.c index 7e75d186e8..75fd3aa5e3 100644 --- a/src/mpi/c/win_delete_attr.c +++ b/src/mpi/c/win_delete_attr.c @@ -31,6 +31,7 @@ int MPI_Win_delete_attr(MPI_Win win, int win_keyval) } } - ret = ompi_attr_delete(WIN_ATTR, win, win->w_keyhash, win_keyval, 0); + ret = ompi_attr_delete(WIN_ATTR, win, win->w_keyhash, win_keyval, + false, true); OMPI_ERRHANDLER_RETURN(ret, win, MPI_ERR_OTHER, FUNC_NAME); } diff --git a/src/mpi/c/win_set_attr.c b/src/mpi/c/win_set_attr.c index c9c1ea2df1..4ca3c2e710 100644 --- a/src/mpi/c/win_set_attr.c +++ b/src/mpi/c/win_set_attr.c @@ -34,6 +34,6 @@ int MPI_Win_set_attr(MPI_Win win, int win_keyval, void *attribute_val) } ret = ompi_attr_set(WIN_ATTR, win, &win->w_keyhash, - win_keyval, attribute_val, 0); + win_keyval, attribute_val, false, true); OMPI_ERRHANDLER_RETURN(ret, win, MPI_ERR_OTHER, FUNC_NAME); } diff --git a/src/mpi/f77/type_delete_attr_f.c b/src/mpi/f77/type_delete_attr_f.c index 9de949cc3f..a79adccce3 100644 --- a/src/mpi/f77/type_delete_attr_f.c +++ b/src/mpi/f77/type_delete_attr_f.c @@ -54,8 +54,4 @@ void mpi_type_delete_attr_f(MPI_Fint *type, MPI_Fint *type_keyval, *ierr = OMPI_INT_2_FINT(MPI_Type_delete_attr(c_type, OMPI_FINT_2_INT(*type_keyval) )); - - if (MPI_SUCCESS == *ierr) { - *type = MPI_Type_c2f( c_type ); - } } diff --git a/src/mpi/f77/win_delete_attr_f.c b/src/mpi/f77/win_delete_attr_f.c index 71bcfda807..a00711d471 100644 --- a/src/mpi/f77/win_delete_attr_f.c +++ b/src/mpi/f77/win_delete_attr_f.c @@ -46,27 +46,13 @@ OMPI_GENERATE_F77_BINDINGS (MPI_WIN_DELETE_ATTR, #if OMPI_PROFILE_LAYER && ! OMPI_HAVE_WEAK_SYMBOLS #include "mpi/f77/profile/defines.h" #endif -static const char FUNC_NAME[] = "MPI_Win_delete_attr_f"; + void mpi_win_delete_attr_f(MPI_Fint *win, MPI_Fint *win_keyval, MPI_Fint *ierr) { - MPI_Win c_win = MPI_Win_f2c( *win ); - int ret, c_err; + MPI_Win c_win = MPI_Win_f2c(*win); - if (MPI_PARAM_CHECK) { - if (MPI_WIN_NULL == c_win) { - c_err = OMPI_ERRHANDLER_INVOKE(c_win, MPI_ERR_WIN, - FUNC_NAME); - *ierr = OMPI_INT_2_FINT(c_err); - } - } - - ret = ompi_attr_delete(WIN_ATTR, c_win, c_win->w_keyhash, *win_keyval, - OMPI_KEYVAL_F77); - - if (MPI_SUCCESS != ret) { - OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_OTHER, FUNC_NAME); - } else { - *ierr = MPI_SUCCESS; - } + *ierr = OMPI_INT_2_FINT(MPI_Win_delete_attr(c_win, + OMPI_FINT_2_INT(*win_keyval) + )); }