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 cbc9d71..4478d6c 100644
--- a/include/haproxy/queue-t.h
+++ b/include/haproxy/queue-t.h
@@ -41,7 +41,6 @@
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 0ca773c..b64e2bb 100644
--- a/include/haproxy/thread.h
+++ b/include/haproxy/thread.h
@@ -399,7 +399,6 @@
LOGSRV_LOCK,
DICT_LOCK,
PROTO_LOCK,
- QUEUE_LOCK,
CKCH_LOCK,
SNI_LOCK,
SSL_SERVER_LOCK,
@@ -452,7 +451,6 @@
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 98a46fd..f6d0442 100644
--- a/src/proxy.c
+++ b/src/proxy.c
@@ -1293,7 +1293,6 @@
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 78a4b52..c9134af 100644
--- a/src/queue.c
+++ b/src/queue.c
@@ -157,9 +157,9 @@
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_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 @@
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 @@
}
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 @@
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 @@
}
__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 @@
}
__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 @@
/* 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 @@
/* 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 @@
((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 @@
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 0a128a2..5d869e6 100644
--- a/src/server.c
+++ b/src/server.c
@@ -2164,7 +2164,6 @@
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);