Revert "MEDIUM: queue: use a dedicated lock for the queues"

This reverts commit fcb8bf8650ec6b5614d1b88db54f1200ebd96cbd.

The recent changes since 5304669e1 MEDIUM: queue: make
pendconn_process_next_strm() only return the pendconn opened a tiny race
condition between stream_free() and process_srv_queue(), as the pendconn
is accessed outside of the lock, possibly while it's being freed. A
different approach is required.

diff --git a/include/haproxy/queue-t.h b/include/haproxy/queue-t.h
index cbc9d7183eb1b82881e781775602431336f93a98..4478d6c55624b379e36412b00a8fdd14f47ea64f 100644 (file)
--- a/include/haproxy/queue-t.h
+++ b/include/haproxy/queue-t.h
@@ -41,7 +41,6 @@ struct pendconn {
 
 struct queue {
        struct eb_root head;                    /* queued pendconnds */
-       __decl_thread(HA_SPINLOCK_T lock);      /* for manipulations in the tree */
        unsigned int idx;                       /* current queuing index */
        unsigned int length;                    /* number of entries */
 };

diff --git a/include/haproxy/thread.h b/include/haproxy/thread.h
index 0ca773cf601b47f7391d57f9155f1b4c8d771566..b64e2bb24f64c224183794bbe11b7c0f6bac7769 100644 (file)
--- a/include/haproxy/thread.h
+++ b/include/haproxy/thread.h
@@ -399,7 +399,6 @@ enum lock_label {
        LOGSRV_LOCK,
        DICT_LOCK,
        PROTO_LOCK,
-       QUEUE_LOCK,
        CKCH_LOCK,
        SNI_LOCK,
        SSL_SERVER_LOCK,
@@ -452,7 +451,6 @@ static inline const char *lock_label(enum lock_label label)
        case LOGSRV_LOCK:          return "LOGSRV";
        case DICT_LOCK:            return "DICT";
        case PROTO_LOCK:           return "PROTO";
-       case QUEUE_LOCK:           return "QUEUE";
        case CKCH_LOCK:            return "CKCH";
        case SNI_LOCK:             return "SNI";
        case SSL_SERVER_LOCK:      return "SSL_SERVER";

diff --git a/src/proxy.c b/src/proxy.c
index 98a46fd5b38c72dc696e38b59c52f8d6fac866ac..f6d0442f166cb2d69cbdbfa4b01fb96ff83eb2ea 100644 (file)
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1293,7 +1293,6 @@ void init_new_proxy(struct proxy *p)
        memset(p, 0, sizeof(struct proxy));
        p->obj_type = OBJ_TYPE_PROXY;
        p->queue.head = EB_ROOT;
-       HA_SPIN_INIT(&p->queue.lock);
        LIST_INIT(&p->acl);
        LIST_INIT(&p->http_req_rules);
        LIST_INIT(&p->http_res_rules);

diff --git a/src/queue.c b/src/queue.c
index 78a4b5211a756cb42995d6dc1168bf68ebc03fe9..c9134afb92e0f4396c03dbbf056d2270a5043e25 100644 (file)
--- a/src/queue.c
+++ b/src/queue.c
@@ -157,9 +157,9 @@ static void __pendconn_unlink_prx(struct pendconn *p)
 static inline void pendconn_queue_lock(struct pendconn *p)
 {
        if (p->srv)
-               HA_SPIN_LOCK(QUEUE_LOCK, &p->srv->queue.lock);
+               HA_SPIN_LOCK(SERVER_LOCK, &p->srv->lock);
        else
-               HA_SPIN_LOCK(QUEUE_LOCK, &p->px->queue.lock);
+               HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->px->lock);
 }
 
 /* Unlocks the queue the pendconn element belongs to. This relies on both p->px
@@ -169,9 +169,9 @@ static inline void pendconn_queue_lock(struct pendconn *p)
 static inline void pendconn_queue_unlock(struct pendconn *p)
 {
        if (p->srv)
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &p->srv->queue.lock);
+               HA_SPIN_UNLOCK(SERVER_LOCK, &p->srv->lock);
        else
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &p->px->queue.lock);
+               HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->px->lock);
 }
 
 /* Removes the pendconn from the server/proxy queue. At this stage, the
@@ -187,12 +187,12 @@ void pendconn_unlink(struct pendconn *p)
 
        if (p->srv) {
                /* queued in the server */
-               HA_SPIN_LOCK(QUEUE_LOCK, &p->srv->queue.lock);
+               HA_SPIN_LOCK(SERVER_LOCK, &p->srv->lock);
                if (p->node.node.leaf_p) {
                        __pendconn_unlink_srv(p);
                        done = 1;
                }
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &p->srv->queue.lock);
+               HA_SPIN_UNLOCK(SERVER_LOCK, &p->srv->lock);
                if (done) {
                        _HA_ATOMIC_DEC(&p->srv->queue.length);
                        _HA_ATOMIC_DEC(&p->px->totpend);
@@ -200,12 +200,12 @@ void pendconn_unlink(struct pendconn *p)
        }
        else {
                /* queued in the proxy */
-               HA_SPIN_LOCK(QUEUE_LOCK, &p->px->queue.lock);
+               HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->px->lock);
                if (p->node.node.leaf_p) {
                        __pendconn_unlink_prx(p);
                        done = 1;
                }
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &p->px->queue.lock);
+               HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->px->lock);
                if (done) {
                        _HA_ATOMIC_DEC(&p->px->queue.length);
                        _HA_ATOMIC_DEC(&p->px->totpend);
@@ -344,13 +344,15 @@ void process_srv_queue(struct server *s, int server_locked)
        while (s->served < maxconn) {
                struct pendconn *pc;
 
-               HA_SPIN_LOCK(QUEUE_LOCK, &s->queue.lock);
-               HA_SPIN_LOCK(QUEUE_LOCK, &p->queue.lock);
+               if (!server_locked)
+                       HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
+               HA_RWLOCK_WRLOCK(PROXY_LOCK,  &p->lock);
 
                pc = pendconn_process_next_strm(s, p);
 
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &p->queue.lock);
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &s->queue.lock);
+               HA_RWLOCK_WRUNLOCK(PROXY_LOCK,  &p->lock);
+               if (!server_locked)
+                       HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 
                if (!pc)
                        break;
@@ -422,10 +424,10 @@ struct pendconn *pendconn_add(struct stream *strm)
                }
                __ha_barrier_atomic_store();
 
-               HA_SPIN_LOCK(QUEUE_LOCK, &p->srv->queue.lock);
+               HA_SPIN_LOCK(SERVER_LOCK, &p->srv->lock);
                p->queue_idx = srv->queue.idx - 1; // for increment
                eb32_insert(&srv->queue.head, &p->node);
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &p->srv->queue.lock);
+               HA_SPIN_UNLOCK(SERVER_LOCK, &p->srv->lock);
        }
        else {
                unsigned int old_max, new_max;
@@ -438,10 +440,10 @@ struct pendconn *pendconn_add(struct stream *strm)
                }
                __ha_barrier_atomic_store();
 
-               HA_SPIN_LOCK(QUEUE_LOCK, &p->px->queue.lock);
+               HA_RWLOCK_WRLOCK(PROXY_LOCK, &p->px->lock);
                p->queue_idx = px->queue.idx - 1; // for increment
                eb32_insert(&px->queue.head, &p->node);
-               HA_SPIN_UNLOCK(QUEUE_LOCK, &p->px->queue.lock);
+               HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &p->px->lock);
        }
 
        _HA_ATOMIC_INC(&px->totpend);
@@ -450,7 +452,7 @@ struct pendconn *pendconn_add(struct stream *strm)
 
 /* Redistribute pending connections when a server goes down. The number of
  * connections redistributed is returned. It must be called with the server
- * queue lock held.
+ * lock held.
  */
 int pendconn_redistribute(struct server *s)
 {
@@ -487,8 +489,8 @@ int pendconn_redistribute(struct server *s)
 /* Check for pending connections at the backend, and assign some of them to
  * the server coming up. The server's weight is checked before being assigned
  * connections it may not be able to handle. The total number of transferred
- * connections is returned. It must be called with the server queue lock held,
- * and will take the proxy's queue lock.
+ * connections is returned. It must be called with the server lock held, and
+ * will take the proxy's lock.
  */
 int pendconn_grab_from_px(struct server *s)
 {
@@ -507,7 +509,7 @@ int pendconn_grab_from_px(struct server *s)
             ((s != s->proxy->lbprm.fbck) && !(s->proxy->options & PR_O_USE_ALL_BK))))
                return 0;
 
-       HA_SPIN_LOCK(QUEUE_LOCK, &s->proxy->queue.lock);
+       HA_RWLOCK_WRLOCK(PROXY_LOCK, &s->proxy->lock);
        maxconn = srv_dynamic_maxconn(s);
        while ((p = pendconn_first(&s->proxy->queue.head))) {
                if (s->maxconn && s->served + xferred >= maxconn)
@@ -519,7 +521,7 @@ int pendconn_grab_from_px(struct server *s)
                task_wakeup(p->strm->task, TASK_WOKEN_RES);
                xferred++;
        }
-       HA_SPIN_UNLOCK(QUEUE_LOCK, &s->proxy->queue.lock);
+       HA_RWLOCK_WRUNLOCK(PROXY_LOCK, &s->proxy->lock);
        if (xferred) {
                _HA_ATOMIC_SUB(&s->proxy->queue.length, xferred);
                _HA_ATOMIC_SUB(&s->proxy->totpend, xferred);

diff --git a/src/server.c b/src/server.c
index 0a128a2f1e927b522a296c04c124fba1125a96d5..5d869e6fba5640a9df4512e358b4f43e9fd86de8 100644 (file)
--- a/src/server.c
+++ b/src/server.c
@@ -2164,7 +2164,6 @@ struct server *new_server(struct proxy *proxy)
        srv->obj_type = OBJ_TYPE_SERVER;
        srv->proxy = proxy;
        srv->queue.head = EB_ROOT;
-       HA_SPIN_INIT(&srv->queue.lock);
        LIST_APPEND(&servers_list, &srv->global_list);
        LIST_INIT(&srv->srv_rec_item);
        LIST_INIT(&srv->ip_rec_item);