Add osi_Assert()'s around pthread_{cond,mutex}_* calls to make sure

we aren't getting errors anywhere.

Update the documentation/comments about Rx lock ordering.

Fix possible deadlock in asymmetric client detection code.

diff --git a/src/rx/rx.c b/src/rx/rx.c
index c64bb10..4c56995 100644 (file)
--- a/src/rx/rx.c
+++ b/src/rx/rx.c
@@ -263,23 +263,31 @@ void rxi_StartUnlocked();
 struct rx_connection *rxLastConn = 0; 
 
 #ifdef RX_ENABLE_LOCKS
-/* The locking hierarchy for rx fine grain locking is composed of five
+/* The locking hierarchy for rx fine grain locking is composed of these
  * tiers:
+ *
+ * rx_connHashTable_lock - synchronizes conn creation, rx_connHashTable access
  * conn_call_lock - used to synchonize rx_EndCall and rx_NewCall
  * call->lock - locks call data fields.
- * Most any other lock - these are all independent of each other.....
- *     rx_freePktQ_lock
+ * These are independent of each other:
  *     rx_freeCallQueue_lock
- *     freeSQEList_lock
- *     rx_connHashTable_lock
- *     rx_serverPool_lock
  *     rxi_keyCreate_lock
+ * rx_serverPool_lock
+ * freeSQEList_lock
+ *
+ * serverQueueEntry->lock
+ * rx_rpc_stats
  * rx_peerHashTable_lock - locked under rx_connHashTable_lock
-
+ * peer->lock - locks peer data fields.
+ * conn_data_lock - that more than one thread is not updating a conn data
+ *                 field at the same time.
+ * rx_freePktQ_lock
+ *
  * lowest level:
- *     peer_lock - locks peer data fields.
- *     conn_data_lock - that more than one thread is not updating a conn data
- *             field at the same time.
+ *     multi_handle->lock
+ *     rxevent_lock
+ *     rx_stats_mutex
+ *
  * Do we need a lock to protect the peer field in the conn structure?
  *      conn->peer was previously a constant for all intents and so has no
  *      lock protecting this field. The multihomed client delta introduced
@@ -404,9 +412,9 @@ int rx_Init(u_int port)
 #ifdef RX_LOCKS_DB
     rxdb_init();
 #endif /* RX_LOCKS_DB */
-    MUTEX_INIT(&rx_stats_mutex, "rx_stats_mutex",MUTEX_DEFAULT,0);    
-    MUTEX_INIT(&rx_rpc_stats, "rx_rpc_stats",MUTEX_DEFAULT,0);    
-    MUTEX_INIT(&rx_freePktQ_lock, "rx_freePktQ_lock",MUTEX_DEFAULT,0);    
+    MUTEX_INIT(&rx_stats_mutex, "rx_stats_mutex",MUTEX_DEFAULT,0);
+    MUTEX_INIT(&rx_rpc_stats, "rx_rpc_stats",MUTEX_DEFAULT,0);
+    MUTEX_INIT(&rx_freePktQ_lock, "rx_freePktQ_lock",MUTEX_DEFAULT,0);
     MUTEX_INIT(&freeSQEList_lock, "freeSQEList lock",MUTEX_DEFAULT,0);
     MUTEX_INIT(&rx_freeCallQueue_lock, "rx_freeCallQueue_lock",
               MUTEX_DEFAULT,0);
@@ -1425,7 +1433,7 @@ osi_socket *socketp;
     } else {    /* otherwise allocate a new one and return that */
        MUTEX_EXIT(&freeSQEList_lock);
        sq = (struct rx_serverQueueEntry *) rxi_Alloc(sizeof(struct rx_serverQueueEntry));
-       MUTEX_INIT(&sq->lock, "server Queue lock",MUTEX_DEFAULT,0);     
+       MUTEX_INIT(&sq->lock, "server Queue lock",MUTEX_DEFAULT,0);
        CV_INIT(&sq->cv, "server Queue lock", CV_DEFAULT, 0);
     }
 
@@ -1583,7 +1591,7 @@ rx_GetCall(tno, cur_service, socketp)
     } else {    /* otherwise allocate a new one and return that */
        MUTEX_EXIT(&freeSQEList_lock);
        sq = (struct rx_serverQueueEntry *) rxi_Alloc(sizeof(struct rx_serverQueueEntry));
-       MUTEX_INIT(&sq->lock, "server Queue lock",MUTEX_DEFAULT,0);     
+       MUTEX_INIT(&sq->lock, "server Queue lock",MUTEX_DEFAULT,0);
        CV_INIT(&sq->cv, "server Queue lock", CV_DEFAULT, 0);
     }
     MUTEX_ENTER(&sq->lock);
@@ -2878,7 +2886,6 @@ static void rxi_CheckReachEvent(event, conn, acall)
     struct clock when;
     int i, waiting;
 
-    MUTEX_ENTER(&conn->conn_call_lock);
     MUTEX_ENTER(&conn->conn_data_lock);
     conn->checkReachEvent = (struct rxevent *) 0;
     waiting = conn->flags & RX_CONN_ATTACHWAIT;
@@ -2886,7 +2893,8 @@ static void rxi_CheckReachEvent(event, conn, acall)
     MUTEX_EXIT(&conn->conn_data_lock);
 
     if (waiting) {
-       if (!call)
+       if (!call) {
+           MUTEX_ENTER(&conn->conn_call_lock);
            for (i=0; i<RX_MAXCALLS; i++) {
                struct rx_call *tc = conn->call[i];
                if (tc && tc->state == RX_STATE_PRECALL) {
@@ -2894,22 +2902,25 @@ static void rxi_CheckReachEvent(event, conn, acall)
                    break;
                }
            }
+           MUTEX_EXIT(&conn->conn_call_lock);
+       }
 
        if (call) {
            if (call != acall) MUTEX_ENTER(&call->lock);
            rxi_SendAck(call, NULL, 0, 0, 0, RX_ACK_PING, 0);
            if (call != acall) MUTEX_EXIT(&call->lock);
 
-           MUTEX_ENTER(&conn->conn_data_lock);
-           conn->refCount++;
-           MUTEX_EXIT(&conn->conn_data_lock);
            clock_GetTime(&when);
            when.sec += RX_CHECKREACH_TIMEOUT;
-           conn->checkReachEvent =
-               rxevent_Post(&when, rxi_CheckReachEvent, conn, NULL);
+           MUTEX_ENTER(&conn->conn_data_lock);
+           if (!conn->checkReachEvent) {
+               conn->refCount++;
+               conn->checkReachEvent =
+                   rxevent_Post(&when, rxi_CheckReachEvent, conn, NULL);
+           }
+           MUTEX_EXIT(&conn->conn_data_lock);
        }
     }
-    MUTEX_EXIT(&conn->conn_call_lock);
 }
 
 static int rxi_CheckConnReach(conn, call)
@@ -3333,7 +3344,6 @@ static void rxi_UpdatePeerReach(conn, acall)
     peer->lastReachTime = clock_Sec();
     MUTEX_EXIT(&peer->peer_lock);
 
-    MUTEX_ENTER(&conn->conn_call_lock);
     MUTEX_ENTER(&conn->conn_data_lock);
     if (conn->flags & RX_CONN_ATTACHWAIT) {
        int i;
@@ -3351,7 +3361,6 @@ static void rxi_UpdatePeerReach(conn, acall)
        }
     } else
        MUTEX_EXIT(&conn->conn_data_lock);
-    MUTEX_EXIT(&conn->conn_call_lock);
 }
 
 /* The real smarts of the whole thing.  */
@@ -4288,6 +4297,7 @@ void rxi_ConnectionError(conn, error)
        if (conn->checkReachEvent) {
            rxevent_Cancel(conn->checkReachEvent, (struct rx_call*)0, 0);
            conn->checkReachEvent = 0;
+           conn->flags &= ~RX_CONN_ATTACHWAIT;
            conn->refCount--;
        }
        MUTEX_EXIT(&conn->conn_data_lock);

diff --git a/src/rx/rx_pthread.h b/src/rx/rx_pthread.h
index 850c3b0..575175d 100644 (file)
--- a/src/rx/rx_pthread.h
+++ b/src/rx/rx_pthread.h
@@ -57,8 +57,9 @@ typedef pthread_cond_t afs_kcondvar_t;
 #ifndef MUTEX_ISMINE
 /* Only used for debugging. */
 #ifdef AFS_SUN5_ENV
+/* synch.h says mutex_t and pthread_mutex_t are always the same */
 #include <synch.h>
-#define MUTEX_ISMINE(l) MUTEX_HELD(l)
+#define MUTEX_ISMINE(l) MUTEX_HELD((mutex_t *) l)
 #else /* AFS_SUN5_ENV */
 #define MUTEX_ISMINE(l) (1)
 #endif /* AFS_SUN5_ENV */
@@ -71,17 +72,17 @@ extern void osirx_AssertMine(afs_kmutex_t *lockaddr, char *msg);
 #ifdef MUTEX_INIT
 #undef MUTEX_INIT
 #endif
-#define MUTEX_INIT(a, b, c, d) pthread_mutex_init(a, NULL)
+#define MUTEX_INIT(a, b, c, d) osi_Assert(pthread_mutex_init(a, NULL) == 0)
 
 #ifdef MUTEX_DESTROY
 #undef MUTEX_DESTROY
 #endif
-#define MUTEX_DESTROY(l) pthread_mutex_destroy(l)
+#define MUTEX_DESTROY(l) osi_Assert(pthread_mutex_destroy(l) == 0)
 
 #ifdef MUTEX_ENTER
 #undef MUTEX_ENTER
 #endif
-#define MUTEX_ENTER(l) pthread_mutex_lock(l)
+#define MUTEX_ENTER(l) osi_Assert(pthread_mutex_lock(l) == 0)
 
 #ifdef MUTEX_TRYENTER
 #undef MUTEX_TRYENTER
@@ -91,7 +92,7 @@ extern void osirx_AssertMine(afs_kmutex_t *lockaddr, char *msg);
 #ifdef MUTEX_EXIT
 #undef MUTEX_EXIT
 #endif
-#define MUTEX_EXIT(l) pthread_mutex_unlock(l)
+#define MUTEX_EXIT(l) osi_Assert(pthread_mutex_unlock(l) == 0)
 
 #ifdef RXObtainWriteLock
 #undef RXObtainWriteLock
@@ -106,27 +107,27 @@ extern void osirx_AssertMine(afs_kmutex_t *lockaddr, char *msg);
 #ifdef CV_INIT
 #undef CV_INIT
 #endif
-#define CV_INIT(cv, a, b, c) pthread_cond_init(cv, NULL)
+#define CV_INIT(cv, a, b, c) osi_Assert(pthread_cond_init(cv, NULL) == 0)
 
 #ifdef CV_DESTROY
 #undef CV_DESTROY
 #endif
-#define CV_DESTROY(cv) pthread_cond_destroy(cv)
+#define CV_DESTROY(cv) osi_Assert(pthread_cond_destroy(cv) == 0)
 
 #ifdef CV_WAIT
 #undef CV_WAIT
 #endif
-#define CV_WAIT(cv, l) pthread_cond_wait(cv, l)
+#define CV_WAIT(cv, l) osi_Assert(pthread_cond_wait(cv, l) == 0)
 
 #ifdef CV_SIGNAL
 #undef CV_SIGNAL
 #endif
-#define CV_SIGNAL(cv) pthread_cond_signal(cv)
+#define CV_SIGNAL(cv) osi_Assert(pthread_cond_signal(cv) == 0)
 
 #ifdef CV_BROADCAST
 #undef CV_BROADCAST
 #endif
-#define CV_BROADCAST(cv) pthread_cond_broadcast(cv)
+#define CV_BROADCAST(cv) osi_Assert(pthread_cond_broadcast(cv) == 0)
 
 #endif /* AFS_PTHREAD_ENV */