MINOR: connection: align toremove_{lock,connections} and cleanup into idle_conns

We used to have 3 thread-based arrays for toremove_lock, idle_cleanup,
and toremove_connections. The problem is that these items are small,
and that this creates false sharing between threads since it's possible
to pack up to 8-16 of these values into a single cache line. This can
cause real damage where there is contention on the lock.

This patch creates a new array of struct "idle_conns" that is aligned
on a cache line and which contains all three members above. This way
each thread has access to its variables without hindering the other
ones. Just doing this increased the HTTP/1 request rate by 5% on a
16-thread machine.

The definition was moved to connection.{c,h} since it appeared a more
natural evolution of the ongoing changes given that there was already
one of them declared in connection.h previously.

diff --git a/include/haproxy/connection-t.h b/include/haproxy/connection-t.h
index ff0ba92e76bef6fbc1504cd4356c33413d6d9849..1ff1c93818ba93fc1f2976133943f50fd2c93763 100644 (file)
--- a/include/haproxy/connection-t.h
+++ b/include/haproxy/connection-t.h
@@ -600,6 +600,16 @@ struct tlv_ssl {
 }__attribute__((packed));
 
 
+/* This structure is used to manage idle connections, their locking, and the
+ * list of such idle connections to be removed. It is per-thread and must be
+ * accessible from foreign threads.
+ */
+struct idle_conns {
+       struct mt_list toremove_conns;
+       __decl_thread(HA_SPINLOCK_T toremove_lock);
+       struct task *cleanup_task;
+} THREAD_ALIGNED(64);
+
 #endif /* _HAPROXY_CONNECTION_T_H */
 
 /*

diff --git a/include/haproxy/connection.h b/include/haproxy/connection.h
index 435d109421f42a42c1288e84fdc7b5c453148102..a7b942cebc4e6153fa43508f48ad2c748d1433c7 100644 (file)
--- a/include/haproxy/connection.h
+++ b/include/haproxy/connection.h
@@ -77,7 +77,7 @@ int conn_recv_socks4_proxy_response(struct connection *conn);
 /* If we delayed the mux creation because we were waiting for the handshake, do it now */
 int conn_create_mux(struct connection *conn);
 
-__decl_thread(extern HA_SPINLOCK_T toremove_lock[MAX_THREADS]);
+extern struct idle_conns idle_conns[MAX_THREADS];
 
 /* returns true is the transport layer is ready */
 static inline int conn_xprt_ready(const struct connection *conn)
@@ -494,9 +494,9 @@ static inline void conn_free(struct connection *conn)
        }
 
        conn_force_unsubscribe(conn);
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        MT_LIST_DEL((struct mt_list *)&conn->list);
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        pool_free(pool_head_connection, conn);
 }
 

diff --git a/include/haproxy/server.h b/include/haproxy/server.h
index 8d89b97e12b410f7428ab3d1c8a029e24c61b390..9c1672a7b43a8d6a196a48427c6759f07fcceb7d 100644 (file)
--- a/include/haproxy/server.h
+++ b/include/haproxy/server.h
@@ -38,8 +38,6 @@
 __decl_thread(extern HA_SPINLOCK_T idle_conn_srv_lock);
 extern struct eb_root idle_conn_srv;
 extern struct task *idle_conn_task;
-extern struct task *idle_conn_cleanup[MAX_THREADS];
-extern struct mt_list toremove_connections[MAX_THREADS];
 extern struct dict server_name_dict;
 
 int srv_downtime(const struct server *s);

diff --git a/src/backend.c b/src/backend.c
index 915adc5b74c5fa1d199f184acf89ea0ac9626394..952e37197d5e05e6dd3679e629d345bd6db0835f 100644 (file)
--- a/src/backend.c
+++ b/src/backend.c
@@ -1083,9 +1083,9 @@ static struct connection *conn_backend_get(struct server *srv, int is_safe)
         * thread may be trying to migrate that connection, and we don't want
         * to end up with two threads using the same connection.
         */
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        conn = MT_LIST_POP(&mt_list[tid], struct connection *, list);
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        /* If we found a connection in our own list, and we don't have to
         * steal one from another thread, then we're done.
@@ -1099,7 +1099,7 @@ static struct connection *conn_backend_get(struct server *srv, int is_safe)
        for (i = tid; !found && (i = ((i + 1 == global.nbthread) ? 0 : i + 1)) != tid;) {
                struct mt_list *elt1, elt2;
 
-               HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[i]);
+               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[i].toremove_lock);
                mt_list_for_each_entry_safe(conn, &mt_list[i], list, elt1, elt2) {
                        if (conn->mux->takeover && conn->mux->takeover(conn) == 0) {
                                MT_LIST_DEL_SAFE(elt1);
@@ -1107,7 +1107,7 @@ static struct connection *conn_backend_get(struct server *srv, int is_safe)
                                break;
                        }
                }
-               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[i]);
+               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[i].toremove_lock);
        }
 
        if (!found)
@@ -1279,7 +1279,7 @@ int connect_server(struct stream *s)
                                // see it possibly larger.
                                ALREADY_CHECKED(i);
 
-                               HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+                               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                                tokill_conn = MT_LIST_POP(&srv->idle_conns[i],
                                    struct connection *, list);
                                if (!tokill_conn)
@@ -1288,13 +1288,13 @@ int connect_server(struct stream *s)
                                if (tokill_conn) {
                                        /* We got one, put it into the concerned thread's to kill list, and wake it's kill task */
 
-                                       MT_LIST_ADDQ(&toremove_connections[i],
+                                       MT_LIST_ADDQ(&idle_conns[i].toremove_conns,
                                            (struct mt_list *)&tokill_conn->list);
-                                       task_wakeup(idle_conn_cleanup[i], TASK_WOKEN_OTHER);
-                                       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+                                       task_wakeup(idle_conns[i].cleanup_task, TASK_WOKEN_OTHER);
+                                       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                                        break;
                                }
-                               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+                               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                        }
                }
 

diff --git a/src/cfgparse.c b/src/cfgparse.c
index 63efc9114ec7a0c58ba40ce4c9e01a873c35958c..4b0dc9e51aecafb09468e04837d7a4cc9ecf5724 100644 (file)
--- a/src/cfgparse.c
+++ b/src/cfgparse.c
@@ -3566,12 +3566,12 @@ out_uri_auth_compat:
                                        idle_conn_task->context = NULL;
 
                                        for (i = 0; i < global.nbthread; i++) {
-                                               idle_conn_cleanup[i] = task_new(1UL << i);
-                                               if (!idle_conn_cleanup[i])
+                                               idle_conns[i].cleanup_task = task_new(1UL << i);
+                                               if (!idle_conns[i].cleanup_task)
                                                        goto err;
-                                               idle_conn_cleanup[i]->process = srv_cleanup_toremove_connections;
-                                               idle_conn_cleanup[i]->context = NULL;
-                                               MT_LIST_INIT(&toremove_connections[i]);
+                                               idle_conns[i].cleanup_task->process = srv_cleanup_toremove_connections;
+                                               idle_conns[i].cleanup_task->context = NULL;
+                                               MT_LIST_INIT(&idle_conns[i].toremove_conns);
                                        }
                                }
 

diff --git a/src/connection.c b/src/connection.c
index 1267416990d3f236058ac1137b527095c3338e53..da0d406cb9afb8b2dc69deeeacefb20123e6b354 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -32,6 +32,7 @@ DECLARE_POOL(pool_head_connstream, "conn_stream", sizeof(struct conn_stream));
 DECLARE_POOL(pool_head_sockaddr,   "sockaddr",    sizeof(struct sockaddr_storage));
 DECLARE_POOL(pool_head_authority,  "authority",   PP2_AUTHORITY_MAX);
 
+struct idle_conns idle_conns[MAX_THREADS] = { };
 struct xprt_ops *registered_xprt[XPRT_ENTRIES] = { NULL, };
 
 /* List head of all known muxes for PROTO */

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index b11612179ea447740e2ca8088d01d85c8878b351..0e3ab3f1ad112591f6e9e66ba51541228db06f0a 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -2918,13 +2918,13 @@ static struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned short status)
        int ret = 0;
 
 
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        if (tl->context == NULL) {
                /* The connection has been taken over by another thread,
                 * we're no longer responsible for it, so just free the
                 * tasklet, and do nothing.
                 */
-               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                tasklet_free(tl);
                return NULL;
 
@@ -2938,7 +2938,7 @@ static struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned short status)
        if (conn_in_list)
                MT_LIST_DEL(&conn->list);
 
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        if (!(fconn->wait_event.events & SUB_RETRY_SEND))
                ret = fcgi_send(fconn);
@@ -3092,7 +3092,7 @@ static struct task *fcgi_timeout_task(struct task *t, void *context, unsigned sh
        /* We're about to destroy the connection, so make sure nobody attempts
         * to steal it from us.
         */
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        if (fconn && fconn->conn->flags & CO_FL_LIST_MASK)
                MT_LIST_DEL(&fconn->conn->list);
@@ -3103,7 +3103,7 @@ static struct task *fcgi_timeout_task(struct task *t, void *context, unsigned sh
        if (!t->context)
                fconn = NULL;
 
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        task_destroy(t);
 

diff --git a/src/mux_h1.c b/src/mux_h1.c
index 776dee40329a9d86ce44e9f54402e691db99a830..8c7b21c494e1b5cdbb976636c5929a5717d45a24 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -2219,13 +2219,13 @@ static struct task *h1_io_cb(struct task *t, void *ctx, unsigned short status)
        int ret = 0;
 
 
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        if (tl->context == NULL) {
                /* The connection has been taken over by another thread,
                 * we're no longer responsible for it, so just free the
                 * tasklet, and do nothing.
                 */
-               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                tasklet_free(tl);
                return NULL;
        }
@@ -2241,7 +2241,7 @@ static struct task *h1_io_cb(struct task *t, void *ctx, unsigned short status)
        if (conn_in_list)
                MT_LIST_DEL(&conn->list);
 
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        if (!(h1c->wait_event.events & SUB_RETRY_SEND))
                ret = h1_send(h1c);
@@ -2308,7 +2308,7 @@ static struct task *h1_timeout_task(struct task *t, void *context, unsigned shor
        /* We're about to destroy the connection, so make sure nobody attempts
         * to steal it from us.
         */
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        if (h1c && h1c->conn->flags & CO_FL_LIST_MASK)
                MT_LIST_DEL(&h1c->conn->list);
@@ -2319,7 +2319,7 @@ static struct task *h1_timeout_task(struct task *t, void *context, unsigned shor
        if (!t->context)
                h1c = NULL;
 
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        task_destroy(t);
 

diff --git a/src/mux_h2.c b/src/mux_h2.c
index bec6af004468675891da51bc1427d156133841cd..c6634c5b749aca5d6655285105a3ad87dc54f062 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -3524,13 +3524,13 @@ static struct task *h2_io_cb(struct task *t, void *ctx, unsigned short status)
        int ret = 0;
 
 
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        if (t->context == NULL) {
                /* The connection has been taken over by another thread,
                 * we're no longer responsible for it, so just free the
                 * tasklet, and do nothing.
                 */
-               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                tasklet_free(tl);
                goto leave;
        }
@@ -3547,7 +3547,7 @@ static struct task *h2_io_cb(struct task *t, void *ctx, unsigned short status)
        if (conn_in_list)
                MT_LIST_DEL(&conn->list);
 
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        if (!(h2c->wait_event.events & SUB_RETRY_SEND))
                ret = h2_send(h2c);
@@ -3643,15 +3643,15 @@ static int h2_process(struct h2c *h2c)
                }
 
                /* connections in error must be removed from the idle lists */
-               HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                MT_LIST_DEL((struct mt_list *)&conn->list);
-               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        }
        else if (h2c->st0 == H2_CS_ERROR) {
                /* connections in error must be removed from the idle lists */
-               HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
                MT_LIST_DEL((struct mt_list *)&conn->list);
-               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        }
 
        if (!b_data(&h2c->dbuf))
@@ -3721,7 +3721,7 @@ static struct task *h2_timeout_task(struct task *t, void *context, unsigned shor
        /* We're about to destroy the connection, so make sure nobody attempts
         * to steal it from us.
         */
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        if (h2c && h2c->conn->flags & CO_FL_LIST_MASK)
                MT_LIST_DEL(&h2c->conn->list);
@@ -3732,7 +3732,7 @@ static struct task *h2_timeout_task(struct task *t, void *context, unsigned shor
        if (!t->context)
                h2c = NULL;
 
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        task_destroy(t);
 
@@ -3778,9 +3778,9 @@ static struct task *h2_timeout_task(struct task *t, void *context, unsigned shor
        }
 
        /* in any case this connection must not be considered idle anymore */
-       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
        MT_LIST_DEL((struct mt_list *)&h2c->conn->list);
-       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[tid]);
+       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[tid].toremove_lock);
 
        /* either we can release everything now or it will be done later once
         * the last stream closes.

diff --git a/src/server.c b/src/server.c
index 1fd29046e841f418bc2ee2c98dd6e087d5a667af..1a00363e8d2dab83086364792d95494f7e32a57f 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -62,9 +62,6 @@ static struct srv_kw_list srv_keywords = {
 __decl_thread(HA_SPINLOCK_T idle_conn_srv_lock);
 struct eb_root idle_conn_srv = EB_ROOT;
 struct task *idle_conn_task = NULL;
-struct task *idle_conn_cleanup[MAX_THREADS] = { NULL };
-struct mt_list toremove_connections[MAX_THREADS];
-__decl_thread(HA_SPINLOCK_T toremove_lock[MAX_THREADS]);
 
 /* The server names dictionary */
 struct dict server_name_dict = {
@@ -5172,7 +5169,7 @@ struct task *srv_cleanup_toremove_connections(struct task *task, void *context,
 {
        struct connection *conn;
 
-       while ((conn = MT_LIST_POP(&toremove_connections[tid],
+       while ((conn = MT_LIST_POP(&idle_conns[tid].toremove_conns,
                                       struct connection *, list)) != NULL) {
                conn->mux->destroy(conn->ctx);
        }
@@ -5193,7 +5190,7 @@ static void srv_cleanup_connections(struct server *srv)
        HA_SPIN_LOCK(OTHER_LOCK, &idle_conn_srv_lock);
        for (i = 0; i < global.nbthread; i++) {
                did_remove = 0;
-               HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[i]);
+               HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[i].toremove_lock);
                for (j = 0; j < srv->curr_idle_conns; j++) {
                        conn = MT_LIST_POP(&srv->idle_conns[i], struct connection *, list);
                        if (!conn)
@@ -5202,11 +5199,11 @@ static void srv_cleanup_connections(struct server *srv)
                        if (!conn)
                                break;
                        did_remove = 1;
-                       MT_LIST_ADDQ(&toremove_connections[i], (struct mt_list *)&conn->list);
+                       MT_LIST_ADDQ(&idle_conns[i].toremove_conns, (struct mt_list *)&conn->list);
                }
-               HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[i]);
+               HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[i].toremove_lock);
                if (did_remove)
-                       task_wakeup(idle_conn_cleanup[i], TASK_WOKEN_OTHER);
+                       task_wakeup(idle_conns[i].cleanup_task, TASK_WOKEN_OTHER);
        }
        HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conn_srv_lock);
 }
@@ -5265,7 +5262,7 @@ struct task *srv_cleanup_idle_connections(struct task *task, void *context, unsi
 
                        max_conn = (exceed_conns * srv->curr_idle_thr[i]) /
                                   curr_idle + 1;
-                       HA_SPIN_LOCK(OTHER_LOCK, &toremove_lock[i]);
+                       HA_SPIN_LOCK(OTHER_LOCK, &idle_conns[i].toremove_lock);
                        for (j = 0; j < max_conn; j++) {
                                struct connection *conn = MT_LIST_POP(&srv->idle_conns[i], struct connection *, list);
                                if (!conn)
@@ -5274,13 +5271,13 @@ struct task *srv_cleanup_idle_connections(struct task *task, void *context, unsi
                                if (!conn)
                                        break;
                                did_remove = 1;
-                               MT_LIST_ADDQ(&toremove_connections[i], (struct mt_list *)&conn->list);
+                               MT_LIST_ADDQ(&idle_conns[i].toremove_conns, (struct mt_list *)&conn->list);
                        }
-                       HA_SPIN_UNLOCK(OTHER_LOCK, &toremove_lock[i]);
+                       HA_SPIN_UNLOCK(OTHER_LOCK, &idle_conns[i].toremove_lock);
                        if (did_remove && max_conn < srv->curr_idle_thr[i])
                                srv_is_empty = 0;
                        if (did_remove)
-                               task_wakeup(idle_conn_cleanup[i], TASK_WOKEN_OTHER);
+                               task_wakeup(idle_conns[i].cleanup_task, TASK_WOKEN_OTHER);
                }
 remove:
                eb32_delete(&srv->idle_node);