Merge pull request #237 from natsys/ab-locks-02

Solution to issues #114, #118, #184.

tempesta_fw/cache.c

@@ -344,8 +344,8 @@ tfw_cache_add(TfwHttpResp *resp, TfwHttpReq *req)
 
 out:
 	/* Now we don't need the request and the reponse anymore. */
-	tfw_http_msg_free((TfwHttpMsg *)req);
-	tfw_http_msg_free((TfwHttpMsg *)resp);
+	tfw_http_conn_msg_free((TfwHttpMsg *)req);
+	tfw_http_conn_msg_free((TfwHttpMsg *)resp);
 }
 
 #define SKB_HDR_SZ	(MAX_HEADER + sizeof(struct ipv6hdr)		\
@@ -453,7 +453,7 @@ __cache_req_process_node(TfwHttpReq *req, unsigned long key,
 	 */
 	action(req, resp, data);
 
-	tfw_http_msg_free((TfwHttpMsg *)req);
+	tfw_http_conn_msg_free((TfwHttpMsg *)req);
 
 	if (ce)
 		tdb_rec_put(ce);

tempesta_fw/client.h

@@ -44,5 +44,6 @@ typedef struct {
 
 TfwClient *tfw_client_obtain(struct sock *sk);
 void tfw_client_put(TfwClient *cli);
+void tfw_cli_conn_release(TfwConnection *conn);
 
 #endif /* __TFW_CLIENT_H__ */

tempesta_fw/connection.c

@@ -33,6 +33,10 @@ static TfwConnHooks *conn_hooks[TFW_CONN_MAX_PROTOS];
 #define TFW_CONN_HOOK_CALL(conn, hook_name) \
 	conn_hooks[TFW_CONN_TYPE2IDX(TFW_CONN_TYPE(conn))]->hook_name(conn)
 
+/*
+ * Initialize the connection structure.
+ * It's not on any list yet, so it's safe to do so without locks.
+ */
 void
 tfw_connection_init(TfwConnection *conn)
 {
@@ -43,33 +47,6 @@ tfw_connection_init(TfwConnection *conn)
 	spin_lock_init(&conn->msg_qlock);
 }
 
-/*
- * It's essential that this function is called before a socket
- * is bound or connected, which guarantees that there are no calls
- * to Tempesta callbacks. No locking is needed under these conditions.
- * Otherwise, it must be called under write lock on sk->sk_callback_lock.
- */
-void
-tfw_connection_link_sk(TfwConnection *conn, struct sock *sk)
-{
-	BUG_ON(conn->sk || sk->sk_user_data);
-	conn->sk = sk;
-	sk->sk_user_data = conn;
-}
-
-/*
- * This function does the opposite to what tfw_connection_link_sk() does.
- * It must be called under write lock on sk->sk_callback_lock to be able
- * to modify sk->sk_user_data, or the socket must not be bound or connected.
- */
-void
-tfw_connection_unlink_sk(TfwConnection *conn)
-{
-	BUG_ON(!conn->sk || !conn->sk->sk_user_data);
-	conn->sk->sk_user_data = NULL;
-	conn->sk = NULL;
-}
-
 void
 tfw_connection_link_peer(TfwConnection *conn, TfwPeer *peer)
 {
@@ -79,15 +56,6 @@ tfw_connection_link_peer(TfwConnection *conn, TfwPeer *peer)
 }
 DEBUG_EXPORT_SYMBOL(tfw_connection_link_peer);
 
-void
-tfw_connection_unlink_peer(TfwConnection *conn)
-{
-	BUG_ON(!conn->peer || list_empty(&conn->list));
-	tfw_peer_del_conn(conn->peer, &conn->list);
-	conn->peer = NULL;
-}
-DEBUG_EXPORT_SYMBOL(tfw_connection_unlink_peer);
-
 /**
  * Publish the "connection is established" event via TfwConnHooks.
  */
@@ -109,6 +77,12 @@ tfw_connection_destruct(TfwConnection *conn)
 	BUG_ON(!list_empty(&conn->msg_queue));
 }
 
+/*
+ * Code architecture decisions ensure that conn->sk remains valid
+ * for the life of @conn instance. The socket itself may have been
+ * closed, but not deleted. ss_send() makes sure that data is sent
+ * only on an active socket.
+ */
 void
 tfw_connection_send(TfwConnection *conn, TfwMsg *msg)
 {

tempesta_fw/connection.h

@@ -53,19 +53,40 @@ enum {
 /**
  * Session/Presentation layer (in OSI terms) handling.
  *
+ * An instance of TfwConnection{} structure links each HTTP message
+ * to the attributes of a connection the message has come on. Some
+ * of those messages may stay longer in Tempesta after they're sent
+ * out to their destinations. Requests are kept until a paired
+ * response comes. By the time there's need to use the request's
+ * connection to send the reponse on, it may already be destroyed.
+ * With that in mind, TfwConnection{} instance is not destroyed
+ * along with the connection so that is can be safely dereferenced.
+ * It's kept around until refcnt permits freeing of the instance,
+ * so it may have longer lifetime than the connection itself.
+ *
+ * @sk is an intrinsic property of TfwConnection{}.
+ * It has exactly the same lifetime as an instance of TfwConnection{}.
+ *
+ * @peer is major property of TfwConnection{}. An instance of @peer
+ * has longer lifetime expectation than a connection. @peer is always
+ * valid while it's referenced from an instance of TfwConnection{}.
+ * That is supported by a separate reference counter in @peer.
+ *
  * @proto	- protocol handler. Base class, must be first;
- * @list	- list of connections with the @peer;
- * @msg_queue	- messages queue to be sent over the connection;
+ * @list	- member in the list of connections with @peer;
+ * @msg_queue	- queue of messages to be sent over the connection;
  * @msg_qlock	- lock for accessing @msg_queue;
- * @msg		- currently processing (receiving) message;
+ * @refcnt	- number of users of the connection structure instance;
+ * @msg		- message that is currently being processed;
  * @peer	- TfwClient or TfwServer handler;
- * @sk		- appropriate sock handler;
+ * @sk		- an appropriate sock handler;
  */
 typedef struct {
 	SsProto			proto;
 	struct list_head	list;
 	struct list_head	msg_queue;
 	spinlock_t		msg_qlock;
+	atomic_t		refcnt;
 	TfwMsg			*msg;
 	TfwPeer 		*peer;
 	struct sock		*sk;
@@ -76,27 +97,127 @@ typedef struct {
 /* Callbacks used by l5-l7 protocols to operate on connection level. */
 typedef struct {
 	/*
-	 * Before servicing a new connection (client or server - connection
-	 * type should be checked in the callback).
-	 * This is a good place to handle Access or GEO modules (block a client
-	 * or bind its descriptor with Geo information).
+	 * Called before servicing a new connection (connection
+	 * type, client or server, is checked in the callback).
+	 * This is a good place to handle Access or GEO modules
+	 * (block a client or bind its descriptor with GEO data).
 	 */
 	int (*conn_init)(TfwConnection *conn);
 
 	/*
-	 * Closing a connection (client or server as for conn_init()).
-	 * This is necessary for modules who account number of established
-	 * client connections.
+	 * Called when closing a connection (client or server,
+	 * as in conn_init()). This is required for modules that
+	 * maintain the number of established client connections.
 	 */
 	void (*conn_destruct)(TfwConnection *conn);
 
-	/**
-	 * High level protocols should be able to allocate messages with all
-	 * required information.
+	/*
+	 * Higher level protocols should be able to allocate
+	 * messages with all required information.
 	 */
 	TfwMsg * (*conn_msg_alloc)(TfwConnection *conn);
 } TfwConnHooks;
 
+static inline void
+tfw_connection_get(TfwConnection *conn)
+{
+	atomic_inc(&conn->refcnt);
+}
+
+static inline bool
+tfw_connection_put(TfwConnection *conn)
+{
+	if (unlikely(!conn))
+		return false;
+	if (likely(atomic_read(&conn->refcnt) == 1))
+		smp_rmb();
+	else if (likely(!atomic_dec_and_test(&conn->refcnt)))
+		return false;
+	return true;
+}
+
+static inline bool
+tfw_connection_hasref(TfwConnection *conn)
+{
+	return atomic_read(&conn->refcnt) > 1;
+}
+
+/*
+ * Link Sync Sockets layer with Tempesta. The socket @sk now carries
+ * a reference to Tempesta's @conn instance. When a Tempesta's socket
+ * callback is called by Sync Sockets on an event in the socket, then
+ * the reference to @conn instance for the socket can be found quickly.
+ */
+static inline void
+tfw_connection_link_from_sk(TfwConnection *conn, struct sock *sk)
+{
+	BUG_ON(sk->sk_user_data);
+	atomic_set(&conn->refcnt, 1);
+	sk->sk_user_data = conn;
+}
+
+/*
+ * The four functions below, link/unlink to/from @sk are called under
+ * sk->sk_callback_lock. The main reason for that was that there's
+ * Tempesta shutdown procedure that runs parallel with Tempesta's main
+ * activity.
+ *
+ * TODO: When the shutdown procedure is changed to run only when main
+ * Tempesta activity has stopped, then sk->sk_callback_lock locks can
+ * be eliminated in Sync Sockets. Also, Tempesta shutdown procedure
+ * need to be verified that runs when Tempesta is unable to start.
+ */
+/*
+ * Link Tempesta with Sync Sockets layer. @conn instance now carries
+ * a reference to @sk. When there's need to send data on a connection,
+ * then the socket for that connection can be found quickly. Also,
+ * get a hold of the socket to avoid premature socket release.
+ */
+static inline void
+tfw_connection_link_to_sk(TfwConnection *conn, struct sock *sk)
+{
+	ss_sock_hold(sk);
+	conn->sk = sk;
+}
+
+/*
+ * Do an oposite to what tfw_connection_link_from_sk() does.
+ * Sync Sockets layer is unlinked from Tempesta, so that Tempesta
+ * callbacks are not called anymore on events in the socket.
+ */
+static inline void
+tfw_connection_unlink_from_sk(struct sock *sk)
+{
+	BUG_ON(sk->sk_user_data == NULL);
+	sk->sk_user_data = NULL;
+}
+
+/*
+ * Do an opposite to what tfw_connection_link_to_sk() does. Tempesta
+ * is unlinked from Sync Sockets layer, so that no data can be sent
+ * anymore on a connection. The previously held socket is released.
+ * Note that it's unnecessary to clear conn->sk as @conn instance is
+ * in the process of being destroyed anyway.
+ */
+static inline void
+tfw_connection_unlink_to_sk(TfwConnection *conn)
+{
+	ss_sock_put(conn->sk);
+}
+
+static inline void
+tfw_connection_unlink_from_peer(TfwConnection *conn)
+{
+	BUG_ON(!conn->peer || list_empty(&conn->list));
+	tfw_peer_del_conn(conn->peer, &conn->list);
+}
+
+static inline bool
+tfw_connection_live(TfwConnection *conn)
+{
+	return conn->sk && ss_sock_live(conn->sk);
+}
+
 /**
  * Check that TfwConnection resources are cleaned up properly.
  */
@@ -106,23 +227,17 @@ tfw_connection_validate_cleanup(TfwConnection *conn)
 	BUG_ON(!conn);
 	BUG_ON(!list_empty(&conn->list));
 	BUG_ON(!list_empty(&conn->msg_queue));
+	BUG_ON(atomic_read(&conn->refcnt) & ~1);
 	BUG_ON(conn->msg);
-	BUG_ON(conn->peer);
-	BUG_ON(conn->sk);
 }
 
-#define TFW_CONN_ALIVE(c)	((c)->sk && SS_SOCK_ALIVE((c)->sk))
-
 void tfw_connection_hooks_register(TfwConnHooks *hooks, int type);
 void tfw_connection_hooks_unregister(int type);
 void tfw_connection_send(TfwConnection *conn, TfwMsg *msg);
 
 /* Generic helpers, used for both client and server connections. */
 void tfw_connection_init(TfwConnection *conn);
-void tfw_connection_link_sk(TfwConnection *conn, struct sock *sk);
-void tfw_connection_unlink_sk(TfwConnection *conn);
 void tfw_connection_link_peer(TfwConnection *conn, TfwPeer *peer);
-void tfw_connection_unlink_peer(TfwConnection *conn);
 
 int tfw_connection_new(TfwConnection *conn);
 void tfw_connection_destruct(TfwConnection *conn);