MINOR: server: move the lock inside srv_add_idle()

Almost all callers of _srv_add_idle() lock the list then call the
function. It's not the most efficient and it requires some care from
the caller to take care of that lock. Let's change this a little bit by
having srv_add_idle() that takes the lock and calls _srv_add_idle() that
is now inlined. This way callers don't have to handle the lock themselves
anymore, and the lock is only taken around the sensitive parts, not the
function call+return.

Interestingly, perf tests show a small perf increase from 2.28-2.32M RPS
to 2.32-2.37M RPS on a 128-thread system.

diff --git a/include/haproxy/server.h b/include/haproxy/server.h
index 39f71e05edfefa20a3dc730000f15c817666acef..e8e00a5e7a3f6c434ed6f39eadcf6d6f2ca6971d 100644 (file)
--- a/include/haproxy/server.h
+++ b/include/haproxy/server.h
@@ -99,7 +99,7 @@ void srv_release_conn(struct server *srv, struct connection *conn);
 struct connection *srv_lookup_conn(struct ceb_root **tree, uint64_t hash);
 struct connection *srv_lookup_conn_next(struct ceb_root **tree, struct connection *conn);
 
-void _srv_add_idle(struct server *srv, struct connection *conn, int is_safe);
+void srv_add_idle(struct server *srv, struct connection *conn, int is_safe);
 int srv_add_to_idle_list(struct server *srv, struct connection *conn, int is_safe);
 void srv_add_to_avail_list(struct server *srv, struct connection *conn);
 struct task *srv_cleanup_toremove_conns(struct task *task, void *context, unsigned int state);

diff --git a/src/connection.c b/src/connection.c
index b4d54cca64c88ace0105d1e930418c4900adbeee..201cd3497f8647d241ee5758f0b3b7894f1084c1 100644 (file)
--- a/src/connection.c
+++ b/src/connection.c
@@ -257,9 +257,7 @@ int conn_notify_mux(struct connection *conn, int old_flags, int forced_wake)
                        }
                        else {
                                ASSUME_NONNULL(srv); /* srv is guaranteed by CO_FL_LIST_MASK */
-                               HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                               _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
-                               HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+                               srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
                        }
                }
        }

diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c
index 0b97b4843947506b033e1e87b87cbb86b8d0f8f1..ff9521bc5fe599c2b5329b31f79c2dc869d91cf8 100644 (file)
--- a/src/mux_fcgi.c
+++ b/src/mux_fcgi.c
@@ -3107,9 +3107,7 @@ struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int state)
                        }
                        else {
                                ASSUME_NONNULL(srv); /* srv is guaranteed by CO_FL_LIST_MASK */
-                               HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                               _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
-                               HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+                               srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
                        }
                }
                else {

diff --git a/src/mux_h1.c b/src/mux_h1.c
index fd77700c0f268a1451a835e12f3e113bb4fa7734..6163d42c2ed78a8c6664d2362f1d51dd2e7910fd 100644 (file)
--- a/src/mux_h1.c
+++ b/src/mux_h1.c
@@ -4374,9 +4374,7 @@ struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state)
                        }
                        else {
                                ASSUME_NONNULL(srv); /* srv is guaranteed by CO_FL_LIST_MASK */
-                               HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                               _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
-                               HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+                               srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
                        }
                }
                else {

diff --git a/src/mux_h2.c b/src/mux_h2.c
index df59f749e6a2ede95acb7e4cdcf61e391e19954b..54d3c212b0cbdf32b0b21306ef88b04b4bf0f2db 100644 (file)
--- a/src/mux_h2.c
+++ b/src/mux_h2.c
@@ -5027,9 +5027,7 @@ struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state)
                        }
                        else {
                                ASSUME_NONNULL(srv); /* srv is guaranteed by CO_FL_LIST_MASK */
-                               HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                               _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
-                               HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+                               srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
                        }
                }
                else {

diff --git a/src/mux_quic.c b/src/mux_quic.c
index bdabfb082a2bf3ec7ad2deccf519a055892bda6a..135c7d8ecb6928f18311aa5f2d5b57e11853f7e2 100644 (file)
--- a/src/mux_quic.c
+++ b/src/mux_quic.c
@@ -3448,9 +3448,7 @@ struct task *qcc_io_cb(struct task *t, void *ctx, unsigned int state)
                        }
                }
                else {
-                       HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                       _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
-                       HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+                       srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
                }
 
                /* Do not access conn without protection as soon as it is reinserted in idle list. */

diff --git a/src/mux_spop.c b/src/mux_spop.c
index b3a43f03a68fc36955eb16343730d2a429556e32..00878b66959e360c862a76c725ec422a2640bd67 100644 (file)
--- a/src/mux_spop.c
+++ b/src/mux_spop.c
@@ -2603,9 +2603,7 @@ static struct task *spop_io_cb(struct task *t, void *ctx, unsigned int state)
                        }
                        else {
                                ASSUME_NONNULL(srv); /* srv is guaranteed by CO_FL_LIST_MASK */
-                               HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                               _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
-                               HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+                               srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
                        }
                }
                else {

diff --git a/src/server.c b/src/server.c
index ed49dd6e0a91b3d574f0f8fed90dbff42fd997db..2487e13eeaf1d2c9132dbbbb8fb3ea48a4b5b5b6 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -7318,7 +7318,7 @@ struct connection *srv_lookup_conn_next(struct ceb_root **tree, struct connectio
  *
  * Must be called with idle_conns_lock.
  */
-void _srv_add_idle(struct server *srv, struct connection *conn, int is_safe)
+static inline void _srv_add_idle(struct server *srv, struct connection *conn, int is_safe)
 {
        struct ceb_root **tree = is_safe ? &srv->per_thr[tid].safe_conns :
                                         &srv->per_thr[tid].idle_conns;
@@ -7330,6 +7330,22 @@ void _srv_add_idle(struct server *srv, struct connection *conn, int is_safe)
        LIST_APPEND(&srv->per_thr[tid].idle_conn_list, &conn->idle_list);
 }
 
+/* Add <conn> in <srv> idle trees. Set <is_safe> if connection is deemed safe
+ * for reuse.
+ *
+ * This function is a simple wrapper for tree insert. It should only be used
+ * for internal usage or when removing briefly the connection to avoid takeover
+ * on it before reinserting it with this function. In other context, prefer to
+ * use the full feature srv_add_to_idle_list(). This function takes the idle
+ * conns lock for the current thread (thus the owner must not already have it).
+ */
+void srv_add_idle(struct server *srv, struct connection *conn, int is_safe)
+{
+       HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+       _srv_add_idle(srv, conn, is_safe);
+       HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+}
+
 /* This adds an idle connection to the server's list if the connection is
  * reusable, not held by any owner anymore, but still has available streams.
  */

diff --git a/src/ssl_sock.c b/src/ssl_sock.c
index 4c9f10581284abd48899fc2ffb26a904b22a3adb..f0cfb14584658b99709138abf93111326ee2a56f 100644 (file)
--- a/src/ssl_sock.c
+++ b/src/ssl_sock.c
@@ -6878,9 +6878,7 @@ leave:
                        else {
                                ASSUME_NONNULL(srv); /* srv is guaranteed by CO_FL_LIST_MASK */
                                TRACE_DEVEL("adding conn back to idle list", SSL_EV_CONN_IO_CB, conn);
-                               HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
-                               _srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
-                               HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock);
+                               srv_add_idle(srv, conn, conn_in_list == CO_FL_SAFE_LIST);
                        }
                }
                else {