BUG/MAJOR: queue/threads: avoid an AB/BA locking issue in process_srv_queue()

A problem involving server slowstart was reported by @max2k1 in issue #197.
The problem is that pendconn_grab_from_px() takes the proxy lock while
already under the server's lock while process_srv_queue() first takes the
proxy's lock then the server's lock.

While the latter seems more natural, it is fundamentally incompatible with
mayn other operations performed on servers, namely state change propagation,
where the proxy is only known after the server and cannot be locked around
the servers. Howwever reversing the lock in process_srv_queue() is trivial
and only the few functions related to dynamic cookies need to be adjusted
for this so that the proxy's lock is taken for each server operation. This
is possible because the proxy's server list is built once at boot time and
remains stable. So this is what this patch does.

The comments in the proxy and server structs were updated to mention this
rule that the server's lock may not be taken under the proxy's lock but
may enclose it.

Another approach could consist in using a second lock for the proxy's queue
which would be different from the regular proxy's lock, but given that the
operations above are rare and operate on small servers list, there is no
reason for overdesigning a solution.

This fix was successfully tested with 10000 servers in a backend where
adjusting the dyncookies in loops over the CLI didn't have a measurable
impact on the traffic.

The only workaround without the fix is to disable any occurrence of
"slowstart" on server lines, or to disable threads using "nbthread 1".

This must be backported as far as 1.8.

include/types/proxy.h

@@ -486,7 +486,7 @@ struct proxy {
 						 * name is used
 						 */
 	struct list filter_configs;		/* list of the filters that are declared on this proxy */
-	__decl_hathreads(HA_SPINLOCK_T lock);
+	__decl_hathreads(HA_SPINLOCK_T lock);   /* may be taken under the server's lock */
 };
 
 struct switching_rule {

include/types/server.h

@@ -319,7 +319,7 @@ struct server {
 	} ssl_ctx;
 #endif
 	struct dns_srvrq *srvrq;		/* Pointer representing the DNS SRV requeest, if any */
-	__decl_hathreads(HA_SPINLOCK_T lock);
+	__decl_hathreads(HA_SPINLOCK_T lock);   /* may enclose the proxy's lock, must not be taken under */
 	struct {
 		const char *file;		/* file where the section appears */
 		struct eb32_node id;		/* place in the tree of used IDs */

src/proxy.c

@@ -1903,9 +1903,12 @@ static int cli_parse_enable_dyncookie_backend(char **args, char *payload, struct
 	if (!px)
 		return 1;
 
+	/* Note: this lock is to make sure this doesn't change while another
+	 * thread is in srv_set_dyncookie().
+	 */
 	HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
-
 	px->ck_opts |= PR_CK_DYNAMIC;
+	HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
 
 	for (s = px->srv; s != NULL; s = s->next) {
 		HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
@@ -1913,8 +1916,6 @@ static int cli_parse_enable_dyncookie_backend(char **args, char *payload, struct
 		HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 	}
 
-	HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
-
 	return 1;
 }
 
@@ -1934,9 +1935,12 @@ static int cli_parse_disable_dyncookie_backend(char **args, char *payload, struc
 	if (!px)
 		return 1;
 
+	/* Note: this lock is to make sure this doesn't change while another
+	 * thread is in srv_set_dyncookie().
+	 */
 	HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
-
 	px->ck_opts &= ~PR_CK_DYNAMIC;
+	HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
 
 	for (s = px->srv; s != NULL; s = s->next) {
 		HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
@@ -1947,8 +1951,6 @@ static int cli_parse_disable_dyncookie_backend(char **args, char *payload, struc
 		HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 	}
 
-	HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
-
 	return 1;
 }
 
@@ -1984,10 +1986,13 @@ static int cli_parse_set_dyncookie_key_backend(char **args, char *payload, struc
 		return 1;
 	}
 
+	/* Note: this lock is to make sure this doesn't change while another
+	 * thread is in srv_set_dyncookie().
+	 */
 	HA_SPIN_LOCK(PROXY_LOCK, &px->lock);
-
 	free(px->dyncookie_key);
 	px->dyncookie_key = newkey;
+	HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
 
 	for (s = px->srv; s != NULL; s = s->next) {
 		HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
@@ -1995,8 +2000,6 @@ static int cli_parse_set_dyncookie_key_backend(char **args, char *payload, struc
 		HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 	}
 
-	HA_SPIN_UNLOCK(PROXY_LOCK, &px->lock);
-
 	return 1;
 }
 

src/queue.c

@@ -312,16 +312,16 @@ void process_srv_queue(struct server *s)
 	struct proxy  *p = s->proxy;
 	int maxconn;
 
-	HA_SPIN_LOCK(PROXY_LOCK,  &p->lock);
 	HA_SPIN_LOCK(SERVER_LOCK, &s->lock);
+	HA_SPIN_LOCK(PROXY_LOCK,  &p->lock);
 	maxconn = srv_dynamic_maxconn(s);
 	while (s->served < maxconn) {
 		int ret = pendconn_process_next_strm(s, p);
 		if (!ret)
 			break;
 	}
-	HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 	HA_SPIN_UNLOCK(PROXY_LOCK,  &p->lock);
+	HA_SPIN_UNLOCK(SERVER_LOCK, &s->lock);
 }
 
 /* Adds the stream <strm> to the pending connection queue of server <strm>->srv
@@ -424,7 +424,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.
+ * 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)
 {

src/server.c

@@ -132,7 +132,7 @@ static inline void srv_check_for_dup_dyncookie(struct server *s)
 }
 
 /*
- * Must be called with the server lock held.
+ * Must be called with the server lock held, and will grab the proxy lock.
  */
 void srv_set_dyncookie(struct server *s)
 {
@@ -144,15 +144,17 @@ void srv_set_dyncookie(struct server *s)
 	int addr_len;
 	int port;
 
+	HA_SPIN_LOCK(PROXY_LOCK, &p->lock);
+
 	if ((s->flags & SRV_F_COOKIESET) ||
 	    !(s->proxy->ck_opts & PR_CK_DYNAMIC) ||
 	    s->proxy->dyncookie_key == NULL)
-		return;
+		goto out;
 	key_len = strlen(p->dyncookie_key);
 
 	if (s->addr.ss_family != AF_INET &&
 	    s->addr.ss_family != AF_INET6)
-		return;
+		goto out;
 	/*
 	 * Buffer to calculate the cookie value.
 	 * The buffer contains the secret key + the server IP address
@@ -181,7 +183,7 @@ void srv_set_dyncookie(struct server *s)
 	hash_value = XXH64(tmpbuf, buffer_len, 0);
 	memprintf(&s->cookie, "%016llx", hash_value);
 	if (!s->cookie)
-		return;
+		goto out;
 	s->cklen = 16;
 
 	/* Don't bother checking if the dyncookie is duplicated if
@@ -190,6 +192,8 @@ void srv_set_dyncookie(struct server *s)
 	 */
 	if (!(s->next_admin & SRV_ADMF_FMAINT))
 		srv_check_for_dup_dyncookie(s);
+ out:
+	HA_SPIN_UNLOCK(PROXY_LOCK, &p->lock);
 }
 
 /*