[master] bd443dd52 pool: Safely update pool statistics

[Dridi Boukelmoune]

commit bd443dd525654c3108ae57a1073dddf37dde8876
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Wed Jun 28 11:08:48 2023 +0200

    pool: Safely update pool statistics

diff --git a/bin/varnishd/cache/cache_pool.c b/bin/varnishd/cache/cache_pool.c
index 89d12b655..53d00a5b8 100644
--- a/bin/varnishd/cache/cache_pool.c
+++ b/bin/varnishd/cache/cache_pool.c
@@ -116,17 +116,27 @@ void v_matchproto_(task_func_t)
 pool_stat_summ(struct worker *wrk, void *priv)
 {
 	struct VSC_main_wrk *src;
+	struct pool *pp;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
-	CHECK_OBJ_NOTNULL(wrk->pool, POOL_MAGIC);
+	pp = wrk->pool;
+	CHECK_OBJ_NOTNULL(pp, POOL_MAGIC);
 	AN(priv);
 	src = priv;
+
 	Lck_Lock(&wstat_mtx);
 	VSC_main_Summ_wrk(VSC_C_main, src);
+
+	Lck_Lock(&pp->mtx);
+	VSC_main_Summ_pool(VSC_C_main, pp->stats);
+	Lck_Unlock(&pp->mtx);
+	memset(pp->stats, 0, sizeof pp->stats);
+
 	Lck_Unlock(&wstat_mtx);
 	memset(src, 0, sizeof *src);
-	AZ(wrk->pool->b_stat);
-	wrk->pool->b_stat = src;
+
+	AZ(pp->b_stat);
+	pp->b_stat = src;
 }
 
 /*--------------------------------------------------------------------
diff --git a/bin/varnishd/cache/cache_wrk.c b/bin/varnishd/cache/cache_wrk.c
index 427ddafbf..8803b5048 100644
--- a/bin/varnishd/cache/cache_wrk.c
+++ b/bin/varnishd/cache/cache_wrk.c
@@ -655,10 +655,6 @@ pool_herder(void *priv)
 			t_idle = VTIM_real() - cache_param->wthread_timeout;
 
 			Lck_Lock(&pp->mtx);
-			/* XXX: unsafe counters */
-			VSC_main_Summ_pool(VSC_C_main, pp->stats);
-			memset(pp->stats, 0, sizeof pp->stats);
-
 			wrk = NULL;
 			pt = VTAILQ_LAST(&pp->idle_queue, taskhead);
 			if (pt != NULL) {
diff --git a/bin/varnishtest/tests/c00124.vtc b/bin/varnishtest/tests/c00124.vtc
new file mode 100644
index 000000000..2da09f139
--- /dev/null
+++ b/bin/varnishtest/tests/c00124.vtc
@@ -0,0 +1,116 @@
+varnishtest "rushed task queued"
+
+# thread reserve mitigation barrier
+barrier b0 sock 2
+
+# client ordering barrier
+barrier b1 cond 3
+
+# waitinglist barrier
+barrier b2 cond 2
+
+# thread starvation barrier
+barrier b3 cond 3
+
+server s1 {
+	rxreq
+	barrier b1 sync
+	barrier b3 sync
+	txresp
+} -start
+
+server s2 {
+	rxreq
+	barrier b1 sync
+	barrier b2 sync
+	txresp -nolen -hdr "Transfer-Encoding: chunked"
+	chunkedlen 10
+	barrier b3 sync
+	chunkedlen 0
+} -start
+
+varnish v1 -cliok "param.set thread_pools 1"
+varnish v1 -cliok "param.set thread_pool_min 5"
+varnish v1 -cliok "param.set thread_pool_max 5"
+varnish v1 -cliok "param.set debug +syncvsl"
+varnish v1 -cliok "param.set debug +waitinglist"
+
+varnish v1 -vcl+backend {
+	import vtc;
+
+	sub vcl_recv {
+		if (req.http.server) {
+			# ensure both c1 and c2 got a thread
+			vtc.barrier_sync("${b0_sock}");
+		}
+	}
+
+	sub vcl_backend_fetch {
+		if (bereq.http.server == "s1") {
+			set bereq.backend = s1;
+		} else if (bereq.http.server == "s2") {
+			set bereq.backend = s2;
+		}
+	}
+} -start
+
+# wait for all threads to be started
+varnish v1 -expect threads == 5
+
+# 2 threads
+client c1 {
+	txreq -hdr "Cookie: foo" -hdr "server: s1"
+	rxresp
+} -start
+
+# 2 threads
+client c2 {
+	txreq -hdr "server: s2"
+	rxresp
+} -start
+
+# ensure c1 and c2 fetch tasks are started
+barrier b1 sync
+
+logexpect l1 -v v1 -g raw {
+	expect * 1007 Debug "on waiting list"
+} -start
+
+logexpect l2 -v v1 -g raw {
+	expect * 1007 Debug "off waiting list"
+} -start
+
+varnish v1 -expect sess_dropped == 0
+varnish v1 -expect sess_queued == 0
+
+# At this point, we are thread-starved and c3 below will steal the
+# acceptor thread. It will be queued before the acceptor task queues
+# itself with a lower priority.
+client c3 {
+	txreq
+	rxresp
+} -start
+
+logexpect l1 -wait
+
+varnish v1 -expect sess_dropped == 0
+varnish v1 -expect sess_queued == 1
+varnish v1 -expect busy_sleep == 1
+
+# Wake up c2, This will in turn trigger a waitinglist rush and wake up c3.
+barrier b2 sync
+
+# The acceptor thread could have restarted on the newly available thread
+# if it weren't for the thread pool reserve. For the same reason, c3's
+# client task should be queued once woken up.
+logexpect l2 -wait
+
+# let everyone loose
+barrier b3 sync
+
+client c1 -wait
+client c2 -wait
+client c3 -wait
+
+varnish v1 -expect sess_dropped == 0
+varnish v1 -expect sess_queued == 2