MINOR: lb/api: let callers of take_conn/drop_conn tell if they have the lock

The two algos defining these functions (first and leastconn) do not need the
server's lock. However it's already present in pendconn_process_next_strm()
so the API must be updated so that the functions may take it if needed and
that the callers indicate whether they already own it.

As such, the call places (backend.c and stream.c) now do not take it
anymore, queue.c was unchanged since it's already held, and both "first"
and "leastconn" were updated to take it if not already held.

A quick test on the "first" algo showed a jump from 432 to 565k rps by
just dropping the lock in stream.c!

diff --git a/include/haproxy/backend-t.h b/include/haproxy/backend-t.h
index 3e3024033cf809577fb605acde2c32c0565ab642..8bee110fef19628195100367777dc4bd5b1fa469 100644 (file)
--- a/include/haproxy/backend-t.h
+++ b/include/haproxy/backend-t.h
@@ -160,12 +160,15 @@ struct lbprm {
        __decl_thread(HA_RWLOCK_T lock);
        struct server *fbck;            /* first backup server when !PR_O_USE_ALL_BK, or NULL */
 
-       /* Call backs for some actions. Any of them may be NULL (thus should be ignored). */
-       void (*update_server_eweight)(struct server *);  /* to be called after eweight change */
-       void (*set_server_status_up)(struct server *);   /* to be called after status changes to UP */
-       void (*set_server_status_down)(struct server *); /* to be called after status changes to DOWN */
-       void (*server_take_conn)(struct server *);       /* to be called when connection is assigned */
-       void (*server_drop_conn)(struct server *);       /* to be called when connection is dropped */
+       /* Call backs for some actions. Any of them may be NULL (thus should be ignored).
+        * Those marked "srvlock" will need to be called with the server lock held.
+        * The other ones might take it themselves if needed, based on indications.
+        */
+       void (*update_server_eweight)(struct server *);  /* to be called after eweight change // srvlock */
+       void (*set_server_status_up)(struct server *);   /* to be called after status changes to UP // srvlock */
+       void (*set_server_status_down)(struct server *); /* to be called after status changes to DOWN // srvlock */
+       void (*server_take_conn)(struct server *, int locked); /* to be called when connection is assigned */
+       void (*server_drop_conn)(struct server *, int locked); /* to be called when connection is dropped */
 };
 
 #endif /* _HAPROXY_BACKEND_T_H */

diff --git a/src/backend.c b/src/backend.c
index b03da4a62e2a51d886b518e1f85027e0fe0df37f..65fb25f9dec0b2091e7010995f9ae5bd4578d33e 100644 (file)
--- a/src/backend.c
+++ b/src/backend.c
@@ -1670,11 +1670,8 @@ skip_reuse:
                s->flags |= SF_CURR_SESS;
                count = _HA_ATOMIC_ADD(&srv->cur_sess, 1);
                HA_ATOMIC_UPDATE_MAX(&srv->counters.cur_sess_max, count);
-               if (s->be->lbprm.server_take_conn) {
-                       HA_SPIN_LOCK(SERVER_LOCK, &srv->lock);
-                       s->be->lbprm.server_take_conn(srv);
-                       HA_SPIN_UNLOCK(SERVER_LOCK, &srv->lock);
-               }
+               if (s->be->lbprm.server_take_conn)
+                       s->be->lbprm.server_take_conn(srv, 0);
        }
 
        /* Now handle synchronously connected sockets. We know the stream-int

diff --git a/src/lb_fas.c b/src/lb_fas.c
index e30606507c14bc4bc068738d5991094373d3102c..3debfeceb8ebecf42d4819ddd45705caac94c557 100644 (file)
--- a/src/lb_fas.c
+++ b/src/lb_fas.c
@@ -61,16 +61,23 @@ static inline void fas_queue_srv(struct server *s)
  * connection or after it has released one. Note that it is possible that
  * the server has been moved out of the tree due to failed health-checks.
  *
- * The server's lock must be held. The lbprm's lock will be used.
+ * <locked> must reflect the server's lock ownership. The lbprm's lock will
+ * be used.
  */
-static void fas_srv_reposition(struct server *s)
+static void fas_srv_reposition(struct server *s, int locked)
 {
+       if (!locked)
+               HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
+
        HA_RWLOCK_WRLOCK(LBPRM_LOCK, &s->proxy->lbprm.lock);
        if (s->lb_tree) {
                fas_dequeue_srv(s);
                fas_queue_srv(s);
        }
        HA_RWLOCK_WRUNLOCK(LBPRM_LOCK, &s->proxy->lbprm.lock);
+
+       if (!locked)
+               HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 }
 
 /* This function updates the server trees according to server <srv>'s new

diff --git a/src/lb_fwlc.c b/src/lb_fwlc.c
index 630467b07288593ea317651150a5866787ee4427..4239a4c63a20d64a35c2e6e361fa9d72e5d757f7 100644 (file)
--- a/src/lb_fwlc.c
+++ b/src/lb_fwlc.c
@@ -67,16 +67,23 @@ static inline void fwlc_queue_srv(struct server *s, unsigned int eweight)
  * connection or after it has released one. Note that it is possible that
  * the server has been moved out of the tree due to failed health-checks.
  *
- * The server's lock must be held. The lbprm's lock will be used.
+ * <locked> must reflect the server's lock ownership. The lbprm's lock will
+ * be used.
  */
-static void fwlc_srv_reposition(struct server *s)
+static void fwlc_srv_reposition(struct server *s, int locked)
 {
+       if (!locked)
+               HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
+
        HA_RWLOCK_WRLOCK(LBPRM_LOCK, &s->proxy->lbprm.lock);
        if (s->lb_tree) {
                fwlc_dequeue_srv(s);
                fwlc_queue_srv(s, s->cur_eweight);
        }
        HA_RWLOCK_WRUNLOCK(LBPRM_LOCK, &s->proxy->lbprm.lock);
+
+       if (!locked)
+               HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 }
 
 /* This function updates the server trees according to server <srv>'s new

diff --git a/src/queue.c b/src/queue.c
index b9b08823152dcc07de2bb038a60f5e503d6dce4b..e5ab5db4578a442e7a314b5b8eaaadb75772f5cf 100644 (file)
--- a/src/queue.c
+++ b/src/queue.c
@@ -327,7 +327,7 @@ static int pendconn_process_next_strm(struct server *srv, struct proxy *px)
        _HA_ATOMIC_ADD(&srv->proxy->served, 1);
        __ha_barrier_atomic_store();
        if (px->lbprm.server_take_conn)
-               px->lbprm.server_take_conn(srv);
+               px->lbprm.server_take_conn(srv, 1);
        stream_add_srv_conn(p->strm, srv);
 
        task_wakeup(p->strm->task, TASK_WOKEN_RES);

diff --git a/src/stream.c b/src/stream.c
index c48af7a89db31f52ae3d4b96c50771db253d773f..9394ac0a74ef91eb06d390acf8ba93d576d35d85 100644 (file)
--- a/src/stream.c
+++ b/src/stream.c
@@ -2541,11 +2541,8 @@ void sess_change_server(struct stream *sess, struct server *newsrv)
                _HA_ATOMIC_SUB(&oldsrv->served, 1);
                _HA_ATOMIC_SUB(&oldsrv->proxy->served, 1);
                __ha_barrier_atomic_store();
-               if (oldsrv->proxy->lbprm.server_drop_conn) {
-                       HA_SPIN_LOCK(SERVER_LOCK, &oldsrv->lock);
-                       oldsrv->proxy->lbprm.server_drop_conn(oldsrv);
-                       HA_SPIN_UNLOCK(SERVER_LOCK, &oldsrv->lock);
-               }
+               if (oldsrv->proxy->lbprm.server_drop_conn)
+                       oldsrv->proxy->lbprm.server_drop_conn(oldsrv, 0);
                stream_del_srv_conn(sess);
        }
 
@@ -2553,11 +2550,8 @@ void sess_change_server(struct stream *sess, struct server *newsrv)
                _HA_ATOMIC_ADD(&newsrv->served, 1);
                _HA_ATOMIC_ADD(&newsrv->proxy->served, 1);
                __ha_barrier_atomic_store();
-               if (newsrv->proxy->lbprm.server_take_conn) {
-                       HA_SPIN_LOCK(SERVER_LOCK, &newsrv->lock);
-                       newsrv->proxy->lbprm.server_take_conn(newsrv);
-                       HA_SPIN_UNLOCK(SERVER_LOCK, &newsrv->lock);
-               }
+               if (newsrv->proxy->lbprm.server_take_conn)
+                       newsrv->proxy->lbprm.server_take_conn(newsrv, 0);
                stream_add_srv_conn(sess, newsrv);
        }
 }