From 229ae1982f6be18e72a7ca85333c5d27c3589b8b Mon Sep 17 00:00:00 2001 From: Nickolai Zeldovich Date: Thu, 22 Aug 2002 03:02:09 +0000 Subject: [PATCH] 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. (cherry picked from commit b08f021fc924f24fe82dae79fa9ff30ff0a17572) --- src/rx/rx.c | 60 ++++++++++++++++++++++++++------------------- src/rx/rx_pthread.h | 21 ++++++++-------- 2 files changed, 46 insertions(+), 35 deletions(-) diff --git a/src/rx/rx.c b/src/rx/rx.c index 1ca01a048..9058d7d8d 100644 --- 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; icall[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 850c3b04e..575175d18 100644 --- 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 -#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 */ -- 2.39.5