From 0607e8a4a19183798c656bf29aaa3f8aa75ac078 Mon Sep 17 00:00:00 2001 From: Jeffrey Altman Date: Wed, 14 Apr 2010 23:21:37 -0400 Subject: [PATCH] Rx: restore thread safety to rx_NewCall Thread safety in rx_NewCall requires that only one thread be actively allocating or recycling a call at a time. Since we are no longer holding the conn_call_lock across the entire transaction we need to have another synchronization mechanism. Add a new rx_connection flag, RX_CONN_MAKECALL_ACTIVE, which when set indicates that a thread is actively obtaining a call. If any other threads see this flag set, they will wait until being signalled that the thread has completed its activity. In addition, because the call->lock may be dropped when processing rxi_ResetCall(), we must hold a reference to the call once we begin using it. Otherwise, the call may be garbage collected behind our back. Change-Id: Ie97757f812d7043203ffcaf399400789cda39da1 Reviewed-on: http://gerrit.openafs.org/1755 Reviewed-by: Derrick Brashear Tested-by: Derrick Brashear --- src/rx/rx.c | 41 +++++++++++++++++++++++++++-------------- src/rx/rx.h | 3 ++- 2 files changed, 29 insertions(+), 15 deletions(-) diff --git a/src/rx/rx.c b/src/rx/rx.c index 4ccd96e0b..9ff827fa3 100644 --- a/src/rx/rx.c +++ b/src/rx/rx.c @@ -982,7 +982,7 @@ rxi_DestroyConnectionNoLock(struct rx_connection *conn) * waiting, treat this as a running call, and wait to destroy the * connection later when the call completes. */ if ((conn->type == RX_CLIENT_CONNECTION) - && (conn->flags & RX_CONN_MAKECALL_WAITING)) { + && (conn->flags & (RX_CONN_MAKECALL_WAITING|RX_CONN_MAKECALL_ACTIVE))) { conn->flags |= RX_CONN_DESTROY_ME; MUTEX_EXIT(&conn->conn_data_lock); USERPRI; @@ -1166,7 +1166,8 @@ rx_NewCall(struct rx_connection *conn) */ MUTEX_ENTER(&conn->conn_call_lock); MUTEX_ENTER(&conn->conn_data_lock); - if (conn->makeCallWaiters) { + while (conn->flags & RX_CONN_MAKECALL_ACTIVE) { + conn->flags |= RX_CONN_MAKECALL_WAITING; conn->makeCallWaiters++; MUTEX_EXIT(&conn->conn_data_lock); @@ -1180,6 +1181,9 @@ rx_NewCall(struct rx_connection *conn) if (conn->makeCallWaiters == 0) conn->flags &= ~RX_CONN_MAKECALL_WAITING; } + + /* We are now the active thread in rx_NewCall */ + conn->flags |= RX_CONN_MAKECALL_ACTIVE; MUTEX_EXIT(&conn->conn_data_lock); for (;;) { @@ -1204,6 +1208,7 @@ rx_NewCall(struct rx_connection *conn) * effect on overall system performance. */ call->state = RX_STATE_RESET; + CALL_HOLD(call, RX_CALL_REFCOUNT_BEGIN); MUTEX_EXIT(&conn->conn_call_lock); rxi_ResetCall(call, 0); (*call->callNumber)++; @@ -1221,6 +1226,7 @@ rx_NewCall(struct rx_connection *conn) MUTEX_EXIT(&call->lock); MUTEX_ENTER(&conn->conn_call_lock); MUTEX_ENTER(&call->lock); + if (call->state == RX_STATE_RESET) break; @@ -1235,6 +1241,7 @@ rx_NewCall(struct rx_connection *conn) * Instead, cycle through one more time to see if * we can find a call that can call our own. */ + CALL_RELE(call, RX_CALL_REFCOUNT_BEGIN); wait = 0; } MUTEX_EXIT(&call->lock); @@ -1242,6 +1249,7 @@ rx_NewCall(struct rx_connection *conn) } else { /* rxi_NewCall returns with mutex locked */ call = rxi_NewCall(conn, i); + CALL_HOLD(call, RX_CALL_REFCOUNT_BEGIN); break; } } @@ -1267,18 +1275,6 @@ rx_NewCall(struct rx_connection *conn) conn->flags &= ~RX_CONN_MAKECALL_WAITING; MUTEX_EXIT(&conn->conn_data_lock); } - /* - * Wake up anyone else who might be giving us a chance to - * run (see code above that avoids resource starvation). - */ -#ifdef RX_ENABLE_LOCKS - CV_BROADCAST(&conn->conn_call_cv); -#else - osi_rxWakeup(conn); -#endif - - CALL_HOLD(call, RX_CALL_REFCOUNT_BEGIN); - /* Client is initially in send mode */ call->state = RX_STATE_ACTIVE; call->error = conn->error; @@ -1295,6 +1291,23 @@ rx_NewCall(struct rx_connection *conn) /* Turn on busy protocol. */ rxi_KeepAliveOn(call); + + /* + * We are no longer the active thread in rx_NewCall + */ + MUTEX_ENTER(&conn->conn_data_lock); + conn->flags &= ~RX_CONN_MAKECALL_ACTIVE; + MUTEX_EXIT(&conn->conn_data_lock); + + /* + * Wake up anyone else who might be giving us a chance to + * run (see code above that avoids resource starvation). + */ +#ifdef RX_ENABLE_LOCKS + CV_BROADCAST(&conn->conn_call_cv); +#else + osi_rxWakeup(conn); +#endif MUTEX_EXIT(&conn->conn_call_lock); #ifdef AFS_GLOBAL_RXLOCK_KERNEL diff --git a/src/rx/rx.h b/src/rx/rx.h index 4d12f5464..343a18e29 100644 --- a/src/rx/rx.h +++ b/src/rx/rx.h @@ -423,13 +423,14 @@ struct rx_peer { #ifndef KDUMP_RX_LOCK /* Flag bits for connection structure */ -#define RX_CONN_MAKECALL_WAITING 1 /* rx_MakeCall is waiting for a channel */ +#define RX_CONN_MAKECALL_WAITING 1 /* rx_NewCall is waiting for a channel */ #define RX_CONN_DESTROY_ME 2 /* Destroy *client* connection after last call */ #define RX_CONN_USING_PACKET_CKSUM 4 /* non-zero header.spare field seen */ #define RX_CONN_KNOW_WINDOW 8 /* window size negotiation works */ #define RX_CONN_RESET 16 /* connection is reset, remove */ #define RX_CONN_BUSY 32 /* connection is busy; don't delete */ #define RX_CONN_ATTACHWAIT 64 /* attach waiting for peer->lastReach */ +#define RX_CONN_MAKECALL_ACTIVE 128 /* a thread is actively in rx_NewCall */ /* Type of connection, client or server */ #define RX_CLIENT_CONNECTION 0 -- 2.39.5