First cleanup sweep over new per-pool acceptor code:

Store the data from accept(2) on the worker->ws, we only need it for
a brief moment until the worker configures a session with it.

If we fail to allocate a session after accepting the connection,
we silently close the connection again:  Presumably it is a DoS
situation.  (false positive:  Extremely popular busy objects)

Make three clearly defined counters for sessions:  Accepted, Dropped
and failed (accept(2) failure).

Lots of spit&polish.

bin/varnishd/cache.h

@@ -289,6 +289,20 @@ struct stream_ctx {
 };
 
 /*--------------------------------------------------------------------*/
+
+struct wrk_accept {
+	unsigned		magic;
+#define WRK_ACCEPT_MAGIC	0x8c4b4d59
+
+	/* Accept stuff */
+	struct sockaddr_storage	acceptaddr;
+	socklen_t		acceptaddrlen;
+	int			acceptsock;
+	struct listen_sock	*acceptlsock;
+};
+
+/*--------------------------------------------------------------------*/
+
 struct worker {
 	unsigned		magic;
 #define WORKER_MAGIC		0x6391adcf
@@ -303,12 +317,6 @@ struct worker {
 	/* Pool stuff */
 	double			lastused;
 
-	/* Accept stuff */
-	struct sockaddr_storage	acceptaddr;
-	socklen_t		acceptaddrlen;
-	int			acceptsock;
-	struct listen_sock	*acceptlsock;
-
 	struct wrw		wrw;
 
 	pthread_cond_t		cond;
@@ -642,8 +650,9 @@ struct vbc {
 void VCA_Prep(struct sess *sp);
 void VCA_Init(void);
 void VCA_Shutdown(void);
-int VCA_Accept(int sock, socklen_t *slp, struct sockaddr_storage *sap);
+int VCA_Accept(struct listen_sock *ls, struct wrk_accept *wa);
 void VCA_SetupSess(struct worker *w);
+void VCA_FailSess(struct worker *w);
 
 /* cache_backend.c */
 void VBE_UseHealth(const struct director *vdi);

bin/varnishd/cache_acceptor.c

@@ -167,7 +167,7 @@ vca_pace_check(void)
 {
 	double p;
 
-	if (vca_pace == 0.0) 
+	if (vca_pace == 0.0)
 		return;
 	Lck_Lock(&pace_mtx);
 	p = vca_pace;
@@ -191,7 +191,7 @@ static void
 vca_pace_good(void)
 {
 
-	if (vca_pace == 0.0) 
+	if (vca_pace == 0.0)
 		return;
 	Lck_Lock(&pace_mtx);
 	vca_pace *= params->acceptor_sleep_decay;
@@ -207,69 +207,89 @@ vca_pace_good(void)
 static int hack_ready;
 
 int
-VCA_Accept(int sock, socklen_t *slp, struct sockaddr_storage *sap)
+VCA_Accept(struct listen_sock *ls, struct wrk_accept *wa)
 {
 	int i;
 
-	assert(sock >= 0);
+	CHECK_OBJ_NOTNULL(ls, LISTEN_SOCK_MAGIC);
+	assert(ls->sock >= 0);
 	vca_pace_check();
 
 	while(!hack_ready)
 		(void)usleep(100*1000);
 
-	*slp = sizeof *sap;
-	i = accept(sock, (void*)sap, slp);
+	wa->acceptaddrlen = sizeof wa->acceptaddr;
+	i = accept(ls->sock, (void*)&wa->acceptaddr, &wa->acceptaddrlen);
 
 	if (i < 0) {
-		VSC_C_main->accept_fail++;
 		switch (errno) {
-		case EAGAIN:
 		case ECONNABORTED:
 			break;
 		case EMFILE:
-			VSL(SLT_Debug, sock, "Too many open files");
+			VSL(SLT_Debug, ls->sock, "Too many open files");
 			vca_pace_bad();
 			break;
 		default:
-			VSL(SLT_Debug, sock, "Accept failed: %s",
+			VSL(SLT_Debug, ls->sock, "Accept failed: %s",
 			    strerror(errno));
 			vca_pace_bad();
 			break;
 		}
 	}
+	wa->acceptlsock = ls;
+	wa->acceptsock = i;
 	return (i);
 }
 
+/*--------------------------------------------------------------------
+ * Fail a session
+ *
+ * This happens if we accept the socket, but cannot get a session
+ * structure.
+ *
+ * We consider this a DoS situation (false positive:  Extremely popular
+ * busy objects) and silently close the connection with minimum effort
+ * and fuzz, rather than try to send an intelligent message back.
+ */
+
+void
+VCA_FailSess(struct worker *w)
+{
+	struct wrk_accept *wa;
+
+	CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
+	CAST_OBJ_NOTNULL(wa, (void*)w->ws->f, WRK_ACCEPT_MAGIC);
+	AZ(w->sp);
+	AZ(close(wa->acceptsock));
+	w->stats.sess_drop++;
+	vca_pace_bad();
+}
+
 /*--------------------------------------------------------------------*/
 
 void
 VCA_SetupSess(struct worker *w)
 {
 	struct sess *sp;
+	struct wrk_accept *wa;
 
+	CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
+	CAST_OBJ_NOTNULL(wa, (void*)w->ws->f, WRK_ACCEPT_MAGIC);
 	sp = w->sp;
-	if (sp == NULL) {
-		AZ(close(w->acceptsock));
-		w->acceptsock = -1;
-		VSC_C_main->client_drop++;
-		/* XXX: 50x Reply ? */
-		vca_pace_bad();
-		INCOMPL();
-	}
 	CHECK_OBJ_NOTNULL(sp, SESS_MAGIC);
-	sp->fd = w->acceptsock;
-	sp->id = w->acceptsock;
-	w->acceptsock = -1;
+	sp->fd = wa->acceptsock;
+	sp->id = wa->acceptsock;
+	wa->acceptsock = -1;
 	sp->t_open = TIM_real();
 	sp->t_end = sp->t_end;
-	sp->mylsock = w->acceptlsock;
-	assert(w->acceptaddrlen <= sp->sockaddrlen);
-	memcpy(&sp->sockaddr, &w->acceptaddr, w->acceptaddrlen);
-	sp->sockaddrlen = w->acceptaddrlen;
+	sp->mylsock = wa->acceptlsock;
+	CHECK_OBJ_NOTNULL(sp->mylsock, LISTEN_SOCK_MAGIC);
+	assert(wa->acceptaddrlen <= sp->sockaddrlen);
+	memcpy(&sp->sockaddr, &wa->acceptaddr, wa->acceptaddrlen);
+	sp->sockaddrlen = wa->acceptaddrlen;
 	sp->step = STP_FIRST;
 	vca_pace_good();
-	w->sp = sp;
-	w->stats.client_conn++;
+	w->stats.sess_conn++;
 }
 
 /*--------------------------------------------------------------------*/

bin/varnishd/cache_center.c

@@ -1115,7 +1115,7 @@ cnt_lookup(struct sess *sp)
 			WS_ReleaseP(sp->ws, (void*)sp->vary_l);
 		} else {
 			AZ(oc->busyobj->vary);
-			WS_Release(sp->ws, 0); 
+			WS_Release(sp->ws, 0);
 		}
 		sp->vary_b = NULL;
 		sp->vary_l = NULL;

bin/varnishd/cache_pool.c

@@ -67,7 +67,6 @@ struct poolsock {
 #define POOLSOCK_MAGIC			0x1b0a2d38
 	VTAILQ_ENTRY(poolsock)		list;
 	struct listen_sock		*lsock;
-	int				sock;
 };
 
 /* Number of work requests queued in excess of worker threads available */
@@ -95,121 +94,163 @@ static unsigned			nthr_max;
 static pthread_cond_t		herder_cond;
 static struct lock		herder_mtx;
 
-/*--------------------------------------------------------------------*/
+/*--------------------------------------------------------------------
+ * Nobody is accepting on this socket, so we do.
+ *
+ * As long as we can stick the accepted connection to another thread
+ * we do so, otherwise we return and handle it ourselves.
+ *
+ * Notice calling convention:  Called locked and returns locked, but
+ * works lock in the meantime.
+ *
+ * We store data about the accept in reserved workspace, it is only used
+ * for a brief moment and it takes up around 144 bytes.
+ */
 
 static void
 pool_accept(struct pool *pp, struct worker *w, const struct poolsock *ps)
 {
 	struct worker *w2;
+	struct wrk_accept *wa, *wa2;
 
 	CHECK_OBJ_NOTNULL(pp, POOL_MAGIC);
 	CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
 	CHECK_OBJ_NOTNULL(ps, POOLSOCK_MAGIC);
 
-	assert(ps->sock >= 0);
+	CHECK_OBJ_NOTNULL(ps->lsock, LISTEN_SOCK_MAGIC);
 	Lck_AssertHeld(&pp->mtx);
 	Lck_Unlock(&pp->mtx);
+	assert(sizeof *wa == WS_Reserve(w->ws, sizeof *wa));
+	wa = (void*)w->ws->f;
 	while (1) {
-		w->acceptsock =
-		    VCA_Accept(ps->sock, &w->acceptaddrlen, &w->acceptaddr);
-		if (w->acceptsock == -1)
+		memset(wa, 0, sizeof *wa);
+		wa->magic = WRK_ACCEPT_MAGIC;
+
+		if (VCA_Accept(ps->lsock, wa) < 0) {
+			w->stats.sess_fail++;
+			/* We're going to pace in vca anyway... */
+			(void)WRK_TrySumStat(w);
 			continue;
-		w->acceptlsock = ps->lsock;
+		}
+
 		Lck_Lock(&pp->mtx);
 		if (VTAILQ_EMPTY(&pp->idle))
 			return;
 		w2 = VTAILQ_FIRST(&pp->idle);
 		VTAILQ_REMOVE(&pp->idle, w2, list);
 		Lck_Unlock(&pp->mtx);
-		w2->acceptaddr = w->acceptaddr;
-		w2->acceptaddrlen = w->acceptaddrlen;
-		w2->acceptsock = w->acceptsock;
-		w2->acceptlsock = w->acceptlsock;
+		assert(sizeof *wa2 == WS_Reserve(w2->ws, sizeof *wa2));
+		wa2 = (void*)w2->ws->f;
+		memcpy(wa2, wa, sizeof *wa);
 		AZ(pthread_cond_signal(&w2->cond));
 	}
 }
 
-/*--------------------------------------------------------------------*/
+/*--------------------------------------------------------------------
+ * This is the work function for worker threads in the pool.
+ */
 
 void
 Pool_Work_Thread(void *priv, struct worker *w)
 {
-	struct pool *qp;
+	struct pool *pp;
 	int stats_clean;
 	struct poolsock *ps;
 
-	CAST_OBJ_NOTNULL(qp, priv, POOL_MAGIC);
-	w->pool = qp;
-	Lck_Lock(&qp->mtx);
-	qp->nthr++;
+	CAST_OBJ_NOTNULL(pp, priv, POOL_MAGIC);
+	w->pool = pp;
+	Lck_Lock(&pp->mtx);
+	pp->nthr++;
 	stats_clean = 1;
 	while (1) {
 
-		Lck_AssertHeld(&qp->mtx);
+		Lck_AssertHeld(&pp->mtx);
 
+		CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
 		CHECK_OBJ_NOTNULL(w->bereq, HTTP_MAGIC);
 		CHECK_OBJ_NOTNULL(w->beresp, HTTP_MAGIC);
 		CHECK_OBJ_NOTNULL(w->resp, HTTP_MAGIC);
-		CHECK_OBJ_NOTNULL(w, WORKER_MAGIC);
 
-		/* Process queued requests, if any */
-		w->sp = VTAILQ_FIRST(&qp->queue);
+		WS_Reset(w->ws, NULL);
+
+		w->sp = VTAILQ_FIRST(&pp->queue);
 		if (w->sp != NULL) {
-			VTAILQ_REMOVE(&qp->queue, w->sp, poollist);
-			qp->lqueue--;
-		} else if (VTAILQ_EMPTY(&qp->socks)) {
+			/* Process queued requests, if any */
+			assert(pp->lqueue > 0);
+			VTAILQ_REMOVE(&pp->queue, w->sp, poollist);
+			pp->lqueue--;
+		} else if (!VTAILQ_EMPTY(&pp->socks)) {
+			/* Accept on a socket */
+			ps = VTAILQ_FIRST(&pp->socks);
+			VTAILQ_REMOVE(&pp->socks, ps, list);
+			pool_accept(pp, w, ps);
+			Lck_AssertHeld(&pp->mtx);
+			VTAILQ_INSERT_TAIL(&pp->socks, ps, list);
+		} else if (VTAILQ_EMPTY(&pp->socks)) {
+			/* Nothing to do: To sleep, perchance to dream ... */
 			if (isnan(w->lastused))
 				w->lastused = TIM_real();
-			VTAILQ_INSERT_HEAD(&qp->idle, w, list);
+			VTAILQ_INSERT_HEAD(&pp->idle, w, list);
 			if (!stats_clean)
 				WRK_SumStat(w);
-			Lck_CondWait(&w->cond, &qp->mtx);
-		} else {
-			ps = VTAILQ_FIRST(&qp->socks);
-			VTAILQ_REMOVE(&qp->socks, ps, list);
-			pool_accept(qp, w, ps);
-			Lck_AssertHeld(&qp->mtx);
-			VTAILQ_INSERT_TAIL(&qp->socks, ps, list);
+			Lck_CondWait(&w->cond, &pp->mtx);
 		}
-		if (w->sp == NULL && w->acceptsock == -1)
+
+		/*
+		 * If we got neither session or accepted a socket, we were
+		 * woken up to die to cull the herd.
+		 */
+		if (w->sp == NULL && w->ws->r == NULL)
 			break;
-		Lck_Unlock(&qp->mtx);
+
+		Lck_Unlock(&pp->mtx);
+
 		if (w->sp == NULL) {
-			w->sp = SES_New(w, qp->sesspool);
-			VCA_SetupSess(w);
+			/* Turn accepted socket into a session */
+			assert(w->ws->r != NULL);
+			w->sp = SES_New(w, pp->sesspool);
+			if (w->sp == NULL)
+				VCA_FailSess(w);
+			else
+				VCA_SetupSess(w);
+			WS_Release(w->ws, 0);
 		}
-		AN(w->sp);
-		assert(w->acceptsock == -1);
-		stats_clean = 0;
-		w->lastused = NAN;
-		WS_Reset(w->ws, NULL);
-		w->storage_hint = NULL;
-
-		AZ(w->sp->wrk);
-		THR_SetSession(w->sp);
-		w->sp->wrk = w;
-		CHECK_OBJ_ORNULL(w->nobjhead, OBJHEAD_MAGIC);
-		CNT_Session(w->sp);
-		CHECK_OBJ_ORNULL(w->nobjhead, OBJHEAD_MAGIC);
-		THR_SetSession(NULL);
-
-		WS_Assert(w->ws);
-		AZ(w->bereq->ws);
-		AZ(w->beresp->ws);
-		AZ(w->resp->ws);
-		AZ(w->wrw.wfd);
-		AZ(w->storage_hint);
-		assert(w->wlp == w->wlb);
-		w->sp = NULL;
-		if (params->diag_bitmap & 0x00040000) {
-			if (w->vcl != NULL)
-				VCL_Rel(&w->vcl);
+		assert(w->ws->r == NULL);
+
+		if (w->sp != NULL) {
+			CHECK_OBJ_NOTNULL(w->sp, SESS_MAGIC);
+
+			stats_clean = 0;
+			w->lastused = NAN;
+			w->storage_hint = NULL;
+
+			AZ(w->sp->wrk);
+			THR_SetSession(w->sp);
+			w->sp->wrk = w;
+			CHECK_OBJ_ORNULL(w->nobjhead, OBJHEAD_MAGIC);
+			CNT_Session(w->sp);
+			CHECK_OBJ_ORNULL(w->nobjhead, OBJHEAD_MAGIC);
+			THR_SetSession(NULL);
+			w->sp = NULL;
+
+			WS_Assert(w->ws);
+			AZ(w->bereq->ws);
+			AZ(w->beresp->ws);
+			AZ(w->resp->ws);
+			AZ(w->wrw.wfd);
+			AZ(w->storage_hint);
+			assert(w->wlp == w->wlb);
+			if (params->diag_bitmap & 0x00040000) {
+				if (w->vcl != NULL)
+					VCL_Rel(&w->vcl);
+			}
 		}
 		stats_clean = WRK_TrySumStat(w);
-		Lck_Lock(&qp->mtx);
+		Lck_Lock(&pp->mtx);
 	}
-	qp->nthr--;
-	Lck_Unlock(&qp->mtx);
+	assert(pp->nthr > 0);
+	pp->nthr--;
+	Lck_Unlock(&pp->mtx);
 	w->pool = NULL;
 }
 
@@ -320,7 +361,6 @@ pool_mkpool(void)
 			continue;
 		ALLOC_OBJ(ps, POOLSOCK_MAGIC);
 		XXXAN(ps);
-		ps->sock = ls->sock;
 		ps->lsock = ls;
 		VTAILQ_INSERT_TAIL(&pp->socks, ps, list);
 	}

bin/varnishd/cache_vrt_vmod.c

@@ -81,7 +81,7 @@ VRT_Vmod_Init(void **hdl, void *ptr, int len, const char *nm,
 		if (v->hdl == NULL) {
 			VCLI_Out(cli, "Loading VMOD %s from %s:\n", nm, path);
 			VCLI_Out(cli, "dlopen() failed: %s\n", dlerror());
-			VCLI_Out(cli, "Check child process permissions.\n"); 
+			VCLI_Out(cli, "Check child process permissions.\n");
 			FREE_OBJ(v);
 			return (1);
 		}

bin/varnishd/cache_waiter_kqueue.c

@@ -52,9 +52,9 @@
 struct vwk {
 	unsigned		magic;
 #define VWK_MAGIC		0x1cc2acc2
-	pthread_t 		thread;
+	pthread_t		thread;
 	int			pipes[2];
-	int 			kq;
+	int			kq;
 	struct kevent		ki[NKEV];
 	unsigned		nki;
 	VTAILQ_HEAD(,sess)	sesshead;

bin/varnishd/cache_wrk.c

@@ -79,7 +79,7 @@ WRK_SumStat(struct worker *w)
 int
 WRK_TrySumStat(struct worker *w)
 {
-	if (Lck_Trylock(&wstat_mtx)) 
+	if (Lck_Trylock(&wstat_mtx))
 		return (0);
 	wrk_sumstat(w);
 	Lck_Unlock(&wstat_mtx);
@@ -165,7 +165,6 @@ wrk_thread_real(void *priv, unsigned shm_workspace, unsigned sess_workspace,
 	w->bereq = HTTP_create(http0, nhttp);
 	w->beresp = HTTP_create(http1, nhttp);
 	w->resp = HTTP_create(http2, nhttp);
-	w->acceptsock = -1;
 	w->wrw.iov = iov;
 	w->wrw.siov = siov;
 	w->wrw.ciov = siov;

bin/varnishtest/tests/b00000.vtc

@@ -10,7 +10,7 @@ varnish v1 -storage "-smalloc,1m" -vcl+backend {} -start
 varnish v1 -cliok "param.set diag_bitmap 0x2"
 
 varnish v1 -expect n_object == 0
-varnish v1 -expect client_conn == 0
+varnish v1 -expect sess_conn == 0
 varnish v1 -expect client_req == 0
 varnish v1 -expect cache_miss == 0
 
@@ -21,7 +21,7 @@ client c1 {
 } -run
 
 varnish v1 -expect n_object == 1
-varnish v1 -expect client_conn == 1
+varnish v1 -expect sess_conn == 1
 varnish v1 -expect client_req == 1
 varnish v1 -expect cache_miss == 1
 varnish v1 -expect s_sess == 1

bin/varnishtest/tests/b00001.vtc

@@ -18,7 +18,7 @@ client c1 {
 } -run
 
 varnish v1 -expect n_object == 0
-varnish v1 -expect client_conn == 1
+varnish v1 -expect sess_conn == 1
 varnish v1 -expect client_req == 1
 varnish v1 -expect s_sess == 1
 varnish v1 -expect s_req == 1

bin/varnishtest/tests/b00002.vtc

@@ -22,7 +22,7 @@ delay .1
 
 varnish v1 -expect n_object == 0
 varnish v1 -expect SMA.Transient.g_alloc == 0
-varnish v1 -expect client_conn == 1
+varnish v1 -expect sess_conn == 1
 varnish v1 -expect client_req == 1
 varnish v1 -expect s_sess == 1
 varnish v1 -expect s_req == 1

bin/varnishtest/tests/b00003.vtc

@@ -24,7 +24,7 @@ client c2 {
 # Give varnish a chance to update stats
 delay .1
 
-varnish v1 -expect client_conn == 2
+varnish v1 -expect sess_conn == 2
 varnish v1 -expect cache_hit == 1
 varnish v1 -expect cache_miss == 1
 varnish v1 -expect client_req == 2

bin/varnishtest/tests/c00020.vtc

@@ -50,7 +50,7 @@ client c2 {
         expect resp.http.X-Varnish == "1006 1003"
 } -run
 
-varnish v1 -expect client_conn == 3
+varnish v1 -expect sess_conn == 3
 varnish v1 -expect cache_hit == 3
 varnish v1 -expect cache_miss == 3
 varnish v1 -expect client_req == 6

bin/varnishtest/tests/c00023.vtc

@@ -148,7 +148,7 @@ client c1 {
 
 varnish v1 -cliok "hcb.dump"
 
-varnish v1 -expect client_conn == 2
+varnish v1 -expect sess_conn == 2
 varnish v1 -expect cache_hit == 9
 varnish v1 -expect cache_miss == 9
 varnish v1 -expect client_req == 18

include/vsc_fields.h

@@ -32,7 +32,7 @@
  *    n - Name:		Field name, in C-source and stats programs
  *    t - Type:		C-type, uint64_t, unless marked in 'f'
  *    l - Local:	Local counter in worker thread.
- *    f - Format: 	Semantics of the value in this field
+ *    f - Format:	Semantics of the value in this field
  *				'a' - Accumulator (deprecated, use 'c')
  *				'b' - Bitmap
  *				'c' - Counter, never decreases.
@@ -40,7 +40,7 @@
  *				'i' - Integer (deprecated, use 'g')
  *    e - Explantion:	Short explanation of field (for screen use)
  *    d - Description:	Long explanation of field (for doc use)
- *	
+ *
  * -----------------------
  * NB: Cleanup in progress
  * -----------------------
@@ -57,9 +57,30 @@
 
 #ifdef VSC_DO_MAIN
 
-VSC_F(client_conn,		uint64_t, 1, 'a', "Client connections accepted", "")
-VSC_F(client_drop,		uint64_t, 0, 'a',
-					"Connection dropped, no sess/wrk", "")
+/*---------------------------------------------------------------------
+ * Sessions
+ *    see: cache_acceptor.c and cache_pool.c
+ */
+
+VSC_F(sess_conn,		uint64_t, 1, 'c',
+    "Sessions accepted",
+	"Count of sessions succesfully accepted"
+)
+VSC_F(sess_drop,		uint64_t, 1, 'c',
+    "Sessions dropped",
+	"Count of sessions silently dropped due to lack of session memory."
+	"  See parameter 'max_sess'."
+)
+
+VSC_F(sess_fail,		uint64_t, 1, 'c',
+    "Session accept failures",
+	"Count of failures to accept TCP connection."
+	"  Either the client changed its mind, or the kernel ran out of"
+	" some resource like filedescriptors."
+)
+
+/*---------------------------------------------------------------------*/
+
 VSC_F(client_req,		uint64_t, 1, 'a', "Client requests received", "")
 
 VSC_F(cache_hit,		uint64_t, 1, 'a', "Cache hits", "")
@@ -195,7 +216,6 @@ VSC_F(hcb_insert,		uint64_t, 0, 'a', "HCB Inserts", "")
 
 VSC_F(esi_errors,		uint64_t, 0, 'a', "ESI parse errors (unlock)", "")
 VSC_F(esi_warnings,		uint64_t, 0, 'a', "ESI parse warnings (unlock)", "")
-VSC_F(accept_fail,		uint64_t, 0, 'a', "Accept failures", "")
 VSC_F(client_drop_late,	uint64_t, 0, 'a', "Connection dropped late", "")
 VSC_F(uptime,		uint64_t, 0, 'a', "Client uptime", "")