- Make attributes thread safe. Design decision: attributes are so far

away from the critical performance path that we do not attempt to
  provide fancy / fine-grained locking at all.  There is one global
  lock that protects most attribute actions.  Since attribute actions
  are non-blocking in nature, this will not cause deadlock.
- Fix a problem where attributes could get deleted from a communicator
  before the communicator was actually released.

This commit was SVN r3136.

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
  */
 
@@ -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,6 +272,16 @@ 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*) 
@@ -260,12 +289,18 @@ ompi_attr_delete(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;
     }
 
     /* 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
+        /* The "predefined" parameter to ompi_attr_set() is set to 1,
-           for prdefined at all and it just falls off the error
+           so that no comparison is done for prdefined at all and it
-           checking loop in attr_set  */
+           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;
 }

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);
+                     bool predefined, bool need_lock);
 
 
 /** 

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;
     }

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) );
 

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);

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);  
 }

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);  
 }

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);  
 }

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);  
 }

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);  
 }

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);  

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);  
 }

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);  
 }

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 );
-    }
 }

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; 
 
-    if (MPI_PARAM_CHECK) {
+    *ierr = OMPI_INT_2_FINT(MPI_Win_delete_attr(c_win, 
-        if (MPI_WIN_NULL == c_win) {
+                                                OMPI_FINT_2_INT(*win_keyval)
-            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;
-    }
 }