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 <shadow@dementia.org>
Tested-by: Derrick Brashear <shadow@dementia.org>

diff --git a/src/rx/rx.c b/src/rx/rx.c
index 4ccd96e0bcb85fe9fce9b43c471f0fcd873ca884..9ff827fa3da8f3fc9e26ce3cabb8f343e3aa3dd0 100644 (file)
--- 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 4d12f5464fe03d3fdbccea743863802121438023..343a18e299fe00ee872279c52b48f41de6bb5fbe 100644 (file)
--- 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