hash: Transfer waiting list ownership to the objcore

The advantages of the objhead owning the waiting list are that we need
less waiting list heads as they are shared by across multiple objcores,
we get more frequent opportunities to rush waiting requests, and we
avoid one expensive part of the lookup, which is to find an objhead from
a request hash.

The downsides are that the requirement to always perform a new lookup,
which increases contention on the objhead lock, and prevents expired but
valid objects to be considered fresh. This is another way to describe
the cache-control no-cache directive: a response considered fresh at
fetch time, but immediately stale afterwards.

Having the waiting list on the objcore means that we reenter the cache
lookup with the object candidate from the previous lookup, so we may
short-circuit the more expensive objhead lookup when the objcore is
compatible with the request rushing off the waiting list. The expensive
objhead lookup from the request hash is still avoided, the objcore has
a reference to the objhead.

This moves the infamous waiting list serialization phenomenon to the
vary header match. Knowing that a rushed object is guaranteed to lead to
a cache hit allows the rush policy to be applied wholesale, instead of
exponentially. Only requests incompatible with the objcore vary header
may reenter the waiting list, a scenario no different from the spurious
rush wakeups when operating on the objhead.

The waiting list operations are still guarded by the objhead lock.

This is the first step towards implementing no-cache and private support
at the header field granularity.

Author: dridi

bin/varnishd/cache/cache.h

@@ -359,6 +359,7 @@ struct objcore {
 	VTAILQ_ENTRY(objcore)	lru_list;
 	VTAILQ_ENTRY(objcore)	ban_list;
 	VSTAILQ_ENTRY(objcore)	exp_list;
+	VTAILQ_HEAD(, req)	waitinglist;
 	struct ban		*ban;
 };
 
@@ -490,8 +491,8 @@ struct req {
 
 	struct objcore		*body_oc;
 
-	/* The busy objhead we sleep on */
-	struct objhead		*hash_objhead;
+	/* The busy objcore we sleep on */
+	struct objcore		*hash_oc;
 
 	/* Built Vary string == workspace reservation */
 	uint8_t			*vary_b;

bin/varnishd/cache/cache_fetch.c

@@ -923,7 +923,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo)
 
 	stale = bo->stale_oc;
 	oc->t_origin = now;
-	if (!VTAILQ_EMPTY(&oc->objhead->waitinglist)) {
+	if (!VTAILQ_EMPTY(&oc->waitinglist)) {
 		/*
 		 * If there is a waitinglist, it means that there is no
 		 * grace-able object, so cache the error return for a

bin/varnishd/cache/cache_hash.c

@@ -75,12 +75,11 @@ struct rush {
 static const struct hash_slinger *hash;
 static struct objhead *private_oh;
 
-static void hsh_rush1(const struct worker *, struct objhead *,
+static void hsh_rush1(const struct worker *, struct objcore *,
     struct rush *, int);
 static void hsh_rush2(struct worker *, struct rush *);
 static int hsh_deref_objhead(struct worker *wrk, struct objhead **poh);
-static int hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh,
-    int);
+static int hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh);
 
 /*---------------------------------------------------------------------*/
 
@@ -102,7 +101,6 @@ hsh_newobjhead(void)
 	XXXAN(oh);
 	oh->refcnt = 1;
 	VTAILQ_INIT(&oh->objcs);
-	VTAILQ_INIT(&oh->waitinglist);
 	Lck_New(&oh->mtx, lck_objhdr);
 	return (oh);
 }
@@ -182,7 +180,6 @@ HSH_DeleteObjHead(const struct worker *wrk, struct objhead *oh)
 
 	AZ(oh->refcnt);
 	assert(VTAILQ_EMPTY(&oh->objcs));
-	assert(VTAILQ_EMPTY(&oh->waitinglist));
 	Lck_Delete(&oh->mtx);
 	wrk->stats->n_objecthead--;
 	FREE_OBJ(oh);
@@ -323,8 +320,8 @@ HSH_Insert(struct worker *wrk, const void *digest, struct objcore *oc,
 	VTAILQ_REMOVE(&oh->objcs, oc, hsh_list);
 	VTAILQ_INSERT_HEAD(&oh->objcs, oc, hsh_list);
 	oc->flags &= ~OC_F_BUSY;
-	if (!VTAILQ_EMPTY(&oh->waitinglist))
-		hsh_rush1(wrk, oh, &rush, HSH_RUSH_POLICY);
+	if (!VTAILQ_EMPTY(&oc->waitinglist))
+		hsh_rush1(wrk, oc, &rush, HSH_RUSH_POLICY);
 	Lck_Unlock(&oh->mtx);
 	hsh_rush2(wrk, &rush);
 
@@ -358,16 +355,47 @@ hsh_insert_busyobj(const struct worker *wrk, struct objhead *oh)
 /*---------------------------------------------------------------------
  */
 
+static int
+hsh_is_rush_hit(struct req *req)
+{
+	struct objcore *oc;
+	const uint8_t *vary;
+
+	oc = req->hash_oc;
+	CHECK_OBJ_ORNULL(oc, OBJCORE_MAGIC);
+
+	if (oc == NULL)
+		return (0);
+
+	AZ(oc->flags & OC_F_BUSY);
+	if (oc->flags != 0)
+		return (0);
+
+	/* The objcore was serviceable, all the requests on the waiting list
+	 * were rushed at once.
+	 */
+	assert(VTAILQ_EMPTY(&oc->waitinglist));
+
+	if (req->hash_ignore_vary)
+		return (1);
+	if (!ObjHasAttr(req->wrk, oc, OA_VARY))
+		return (1);
+
+	vary = ObjGetAttr(req->wrk, oc, OA_VARY, NULL);
+	AN(vary);
+	return (VRY_Match(req, vary));
+}
+
 enum lookup_e
 HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 {
 	struct worker *wrk;
 	struct objhead *oh;
 	struct objcore *oc;
 	struct objcore *exp_oc;
+	struct objcore *busy_oc;
 	const struct vcf_return *vr;
 	vtim_real exp_t_origin;
-	int busy_found;
 	const uint8_t *vary;
 	intmax_t boc_progress;
 	unsigned xid = 0;
@@ -390,15 +418,30 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 	if (DO_DEBUG(DBG_HASHEDGE))
 		hsh_testmagic(req->digest);
 
-	if (req->hash_objhead != NULL) {
+	if (hsh_is_rush_hit(req)) {
+		TAKE_OBJ_NOTNULL(oc, &req->hash_oc, OBJCORE_MAGIC);
+		*ocp = oc;
+		oc->hits++;
+		boc_progress = oc->boc == NULL ? -1 : oc->boc->fetched_so_far;
+		Req_LogHit(wrk, req, oc, boc_progress);
+		/* NB: since this hit comes from the waiting list instead of
+		 * a regular lookup, grace is not considered. The object is
+		 * fresh in the context of the waiting list, even expired: it
+		 * was successfully [re]validated by a fetch task.
+		 */
+		return (HSH_HIT);
+	}
+
+	if (req->hash_oc != NULL) {
 		/*
 		 * This req came off the waiting list, and brings an
-		 * oh refcnt with it.
+		 * incompatible hash_oc refcnt with it.
 		 */
-		CHECK_OBJ_NOTNULL(req->hash_objhead, OBJHEAD_MAGIC);
-		oh = req->hash_objhead;
+		CHECK_OBJ_NOTNULL(req->hash_oc->objhead, OBJHEAD_MAGIC);
+		oh = req->hash_oc->objhead;
+		/* XXX: can we avoid grabbing the oh lock twice here? */
+		(void)HSH_DerefObjCore(wrk, &req->hash_oc, HSH_RUSH_POLICY);
 		Lck_Lock(&oh->mtx);
-		req->hash_objhead = NULL;
 	} else {
 		AN(wrk->wpriv->nobjhead);
 		oh = hash->lookup(wrk, req->digest, &wrk->wpriv->nobjhead);
@@ -417,7 +460,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 	}
 
 	assert(oh->refcnt > 0);
-	busy_found = 0;
+	busy_oc = NULL;
 	exp_oc = NULL;
 	exp_t_origin = 0.0;
 	VTAILQ_FOREACH(oc, &oh->objcs, hsh_list) {
@@ -434,17 +477,19 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 
 		CHECK_OBJ_ORNULL(oc->boc, BOC_MAGIC);
 		if (oc->flags & OC_F_BUSY) {
+			if (busy_oc != NULL)
+				continue;
 			if (req->hash_ignore_busy)
 				continue;
 
-			if (oc->boc && oc->boc->vary != NULL &&
+			if (oc->boc != NULL && oc->boc->vary != NULL &&
 			    !req->hash_ignore_vary &&
 			    !VRY_Match(req, oc->boc->vary)) {
 				wrk->strangelove++;
 				continue;
 			}
 
-			busy_found = 1;
+			busy_oc = oc;
 			continue;
 		}
 
@@ -501,7 +546,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 	if (oc != NULL && oc->flags & OC_F_HFP) {
 		xid = VXID(ObjGetXID(wrk, oc));
 		dttl = EXP_Dttl(req, oc);
-		AN(hsh_deref_objhead_unlock(wrk, &oh, HSH_RUSH_POLICY));
+		AN(hsh_deref_objhead_unlock(wrk, &oh));
 		wrk->stats->cache_hitpass++;
 		VSLb(req->vsl, SLT_HitPass, "%u %.6f", xid, dttl);
 		return (HSH_HITPASS);
@@ -521,7 +566,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 		}
 		oc->hits++;
 		boc_progress = oc->boc == NULL ? -1 : oc->boc->fetched_so_far;
-		AN(hsh_deref_objhead_unlock(wrk, &oh, HSH_RUSH_POLICY));
+		AN(hsh_deref_objhead_unlock(wrk, &oh));
 		Req_LogHit(wrk, req, oc, boc_progress);
 		return (HSH_HIT);
 	}
@@ -547,7 +592,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 	else
 		boc_progress = -1;
 
-	if (!busy_found) {
+	if (busy_oc == NULL) {
 		*bocp = hsh_insert_busyobj(wrk, oh);
 
 		if (exp_oc != NULL) {
@@ -564,33 +609,38 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
 		return (HSH_MISS);
 	}
 
-	AN(busy_found);
+	CHECK_OBJ_NOTNULL(busy_oc, OBJCORE_MAGIC);
+
 	if (exp_oc != NULL && EXP_Ttl_grace(req, exp_oc) >= req->t_req) {
 		/* we do not wait on the busy object if in grace */
 		exp_oc->refcnt++;
 		*ocp = exp_oc;
 		exp_oc->hits++;
-		AN(hsh_deref_objhead_unlock(wrk, &oh, 0));
+		AN(hsh_deref_objhead_unlock(wrk, &oh));
 		Req_LogHit(wrk, req, exp_oc, boc_progress);
 		return (HSH_GRACE);
 	}
 
 	/* There are one or more busy objects, wait for them */
-	VTAILQ_INSERT_TAIL(&oh->waitinglist, req, w_list);
+	VTAILQ_INSERT_TAIL(&busy_oc->waitinglist, req, w_list);
 
 	AZ(req->hash_ignore_busy);
 
 	/*
-	 * The objhead reference transfers to the sess, we get it
-	 * back when the sess comes off the waiting list and
-	 * calls us again
+	 * The objcore reference transfers to the req, we get it
+	 * back when the req comes off the waiting list and
+	 * calls us again.
 	 */
-	req->hash_objhead = oh;
+	assert(busy_oc->refcnt > 0);
+	busy_oc->refcnt++;
+
+	AZ(req->hash_oc);
+	req->hash_oc = busy_oc;
 	req->wrk = NULL;
 	req->waitinglist = 1;
 
 	if (DO_DEBUG(DBG_WAITINGLIST))
-		VSLb(req->vsl, SLT_Debug, "on waiting list <%p>", oh);
+		VSLb(req->vsl, SLT_Debug, "on waiting list <%p>", busy_oc);
 
 	Lck_Unlock(&oh->mtx);
 
@@ -603,30 +653,35 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
  */
 
 static void
-hsh_rush1(const struct worker *wrk, struct objhead *oh, struct rush *r, int max)
+hsh_rush1(const struct worker *wrk, struct objcore *oc, struct rush *r, int max)
 {
 	int i;
 	struct req *req;
 
 	if (max == 0)
 		return;
-	if (max == HSH_RUSH_POLICY)
-		max = cache_param->rush_exponent;
+	if (max == HSH_RUSH_POLICY) {
+		AZ(oc->flags & OC_F_BUSY);
+		if (oc->flags != 0)
+			max = cache_param->rush_exponent;
+		else
+			max = INT_MAX;
+	}
 	assert(max > 0);
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
-	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+	CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
 	CHECK_OBJ_NOTNULL(r, RUSH_MAGIC);
 	VTAILQ_INIT(&r->reqs);
-	Lck_AssertHeld(&oh->mtx);
+	Lck_AssertHeld(&oc->objhead->mtx);
 	for (i = 0; i < max; i++) {
-		req = VTAILQ_FIRST(&oh->waitinglist);
+		req = VTAILQ_FIRST(&oc->waitinglist);
 		if (req == NULL)
 			break;
 		CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
 		wrk->stats->busy_wakeup++;
 		AZ(req->wrk);
-		VTAILQ_REMOVE(&oh->waitinglist, req, w_list);
+		VTAILQ_REMOVE(&oc->waitinglist, req, w_list);
 		VTAILQ_INSERT_TAIL(&r->reqs, req, w_list);
 		req->waitinglist = 0;
 	}
@@ -887,9 +942,9 @@ HSH_Unbusy(struct worker *wrk, struct objcore *oc)
 	VTAILQ_REMOVE(&oh->objcs, oc, hsh_list);
 	VTAILQ_INSERT_HEAD(&oh->objcs, oc, hsh_list);
 	oc->flags &= ~OC_F_BUSY;
-	if (!VTAILQ_EMPTY(&oh->waitinglist)) {
-		assert(oh->refcnt > 1);
-		hsh_rush1(wrk, oh, &rush, HSH_RUSH_POLICY);
+	if (!VTAILQ_EMPTY(&oc->waitinglist)) {
+		assert(oc->refcnt > 1);
+		hsh_rush1(wrk, oc, &rush, HSH_RUSH_POLICY);
 	}
 	Lck_Unlock(&oh->mtx);
 	EXP_Insert(wrk, oc); /* Does nothing unless EXP_RefNewObjcore was
@@ -1041,9 +1096,9 @@ HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp, int rushmax)
 	r = --oc->refcnt;
 	if (!r)
 		VTAILQ_REMOVE(&oh->objcs, oc, hsh_list);
-	if (!VTAILQ_EMPTY(&oh->waitinglist)) {
-		assert(oh->refcnt > 1);
-		hsh_rush1(wrk, oh, &rush, rushmax);
+	if (!VTAILQ_EMPTY(&oc->waitinglist)) {
+		assert(oc->refcnt > 0);
+		hsh_rush1(wrk, oc, &rush, rushmax);
 	}
 	Lck_Unlock(&oh->mtx);
 	hsh_rush2(wrk, &rush);
@@ -1066,38 +1121,24 @@ HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp, int rushmax)
 }
 
 static int
-hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh, int max)
+hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh)
 {
 	struct objhead *oh;
-	struct rush rush;
-	int r;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	TAKE_OBJ_NOTNULL(oh, poh, OBJHEAD_MAGIC);
 
 	Lck_AssertHeld(&oh->mtx);
 
 	if (oh == private_oh) {
-		assert(VTAILQ_EMPTY(&oh->waitinglist));
 		assert(oh->refcnt > 1);
 		oh->refcnt--;
 		Lck_Unlock(&oh->mtx);
 		return (1);
 	}
 
-	INIT_OBJ(&rush, RUSH_MAGIC);
-	if (!VTAILQ_EMPTY(&oh->waitinglist)) {
-		assert(oh->refcnt > 1);
-		hsh_rush1(wrk, oh, &rush, max);
-	}
-
-	if (oh->refcnt == 1)
-		assert(VTAILQ_EMPTY(&oh->waitinglist));
-
 	assert(oh->refcnt > 0);
-	r = hash->deref(wrk, oh); /* Unlocks oh->mtx */
-	hsh_rush2(wrk, &rush);
-	return (r);
+	return (hash->deref(wrk, oh)); /* Unlocks oh->mtx */
 }
 
 static int
@@ -1109,7 +1150,7 @@ hsh_deref_objhead(struct worker *wrk, struct objhead **poh)
 	TAKE_OBJ_NOTNULL(oh, poh, OBJHEAD_MAGIC);
 
 	Lck_Lock(&oh->mtx);
-	return (hsh_deref_objhead_unlock(wrk, &oh, 0));
+	return (hsh_deref_objhead_unlock(wrk, &oh));
 }
 
 void

bin/varnishd/cache/cache_obj.c

@@ -142,6 +142,7 @@ ObjNew(const struct worker *wrk)
 	wrk->stats->n_objectcore++;
 	oc->last_lru = NAN;
 	oc->flags = OC_F_BUSY;
+	VTAILQ_INIT(&oc->waitinglist);
 
 	oc->boc = obj_newboc();
 
@@ -160,6 +161,7 @@ ObjDestroy(const struct worker *wrk, struct objcore **p)
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
 	TAKE_OBJ_NOTNULL(oc, p, OBJCORE_MAGIC);
+	assert(VTAILQ_EMPTY(&oc->waitinglist));
 	if (oc->boc != NULL)
 		obj_deleteboc(&oc->boc);
 	FREE_OBJ(oc);