Fix a misfeature in the hasher-API: Pass the digest in, so we don't have

to touch the (potential) new objhead, unless we intend to insert it.

Straighten out the colvolved logic of critbit while here.

bin/varnishd/cache/cache_hash.c

@@ -253,15 +253,10 @@ HSH_Insert(struct worker *wrk, const void *digest, struct objcore *oc)
 	CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC);
 
 	hsh_prealloc(wrk);
-	if (cache_param->diag_bitmap & 0x80000000)
-		hsh_testmagic(wrk->nobjhead->digest);
 
 	AN(wrk->nobjhead);
-	memcpy(wrk->nobjhead->digest, digest, SHA256_LEN);
-	oh = hash->lookup(wrk, wrk->nobjhead);
+	oh = hash->lookup(wrk, digest, &wrk->nobjhead);
 	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
-	if (oh == wrk->nobjhead)
-		wrk->nobjhead = NULL;
 	Lck_Lock(&oh->mtx);
 	assert(oh->refcnt > 0);
 
@@ -303,9 +298,8 @@ HSH_Lookup(struct sess *sp)
 	AN(hash);
 
 	hsh_prealloc(wrk);
-	memcpy(wrk->nobjhead->digest, req->digest, sizeof req->digest);
 	if (cache_param->diag_bitmap & 0x80000000)
-		hsh_testmagic(wrk->nobjhead->digest);
+		hsh_testmagic(req->digest);
 
 	if (req->hash_objhead != NULL) {
 		/*
@@ -317,9 +311,7 @@ HSH_Lookup(struct sess *sp)
 		req->hash_objhead = NULL;
 	} else {
 		AN(wrk->nobjhead);
-		oh = hash->lookup(wrk, wrk->nobjhead);
-		if (oh == wrk->nobjhead)
-			wrk->nobjhead = NULL;
+		oh = hash->lookup(wrk, req->digest, &wrk->nobjhead);
 	}
 
 	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);

bin/varnishd/hash/hash_classic.c

@@ -111,24 +111,26 @@ hcl_start(void)
  */
 
 static struct objhead * __match_proto__(hash_lookup_f)
-hcl_lookup(struct worker *wrk, struct objhead *noh)
+hcl_lookup(struct worker *wrk, const void *digest, struct objhead **noh)
 {
 	struct objhead *oh;
 	struct hcl_hd *hp;
-	unsigned u1, digest;
+	unsigned u1, hdigest;
 	int i;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
-	CHECK_OBJ_NOTNULL(noh, OBJHEAD_MAGIC);
+	AN(digest);
+	AN(noh);
+	CHECK_OBJ_NOTNULL(*noh, OBJHEAD_MAGIC);
 
-	assert(sizeof noh->digest > sizeof digest);
-	memcpy(&digest, noh->digest, sizeof digest);
-	u1 = digest % hcl_nhash;
+	assert(sizeof oh->digest >= sizeof hdigest);
+	memcpy(&hdigest, digest, sizeof hdigest);
+	u1 = hdigest % hcl_nhash;
 	hp = &hcl_head[u1];
 
 	Lck_Lock(&hp->mtx);
 	VTAILQ_FOREACH(oh, &hp->head, hoh_list) {
-		i = memcmp(oh->digest, noh->digest, sizeof oh->digest);
+		i = memcmp(oh->digest, digest, sizeof oh->digest);
 		if (i < 0)
 			continue;
 		if (i > 0)
@@ -139,14 +141,18 @@ hcl_lookup(struct worker *wrk, struct objhead *noh)
 	}
 
 	if (oh != NULL)
-		VTAILQ_INSERT_BEFORE(oh, noh, hoh_list);
+		VTAILQ_INSERT_BEFORE(oh, *noh, hoh_list);
 	else
-		VTAILQ_INSERT_TAIL(&hp->head, noh, hoh_list);
+		VTAILQ_INSERT_TAIL(&hp->head, *noh, hoh_list);
 
-	noh->hoh_head = hp;
+	oh = *noh;
+	*noh = NULL;
+	memcpy(oh->digest, digest, sizeof oh->digest);
+
+	oh->hoh_head = hp;
 
 	Lck_Unlock(&hp->mtx);
-	return (noh);
+	return (oh);
 }
 
 /*--------------------------------------------------------------------

bin/varnishd/hash/hash_critbit.c

@@ -168,16 +168,15 @@ hcb_l_y(uintptr_t u)
  */
 
 static unsigned
-hcb_crit_bit(const struct objhead *oh1, const struct objhead *oh2,
-    struct hcb_y *y)
+hcb_crit_bit(const uint8_t *digest, const struct objhead *oh2, struct hcb_y *y)
 {
 	unsigned char u, r;
 
 	CHECK_OBJ_NOTNULL(y, HCB_Y_MAGIC);
-	for (u = 0; u < DIGEST_LEN && oh1->digest[u] == oh2->digest[u]; u++)
+	for (u = 0; u < DIGEST_LEN && digest[u] == oh2->digest[u]; u++)
 		;
 	assert(u < DIGEST_LEN);
-	r = hcb_bits(oh1->digest[u], oh2->digest[u]);
+	r = hcb_bits(digest[u], oh2->digest[u]);
 	y->ptr = u;
 	y->bitmask = 0x80 >> r;
 	y->critbit = u * 8 + r;
@@ -191,8 +190,8 @@ hcb_crit_bit(const struct objhead *oh1, const struct objhead *oh2,
  */
 
 static struct objhead *
-hcb_insert(struct worker *wrk, struct hcb_root *root, struct objhead *oh,
-    int has_lock)
+hcb_insert(struct worker *wrk, struct hcb_root *root, const uint8_t *digest,
+    struct objhead **noh)
 {
 	volatile uintptr_t *p;
 	uintptr_t pp;
@@ -203,17 +202,20 @@ hcb_insert(struct worker *wrk, struct hcb_root *root, struct objhead *oh,
 	p = &root->origo;
 	pp = *p;
 	if (pp == 0) {
-		if (!has_lock)
+		if (noh == NULL)
 			return (NULL);
-		*p = hcb_r_node(oh);
-		return (oh);
+		oh2 = *noh;
+		*noh = NULL;
+		memcpy(oh2->digest, digest, sizeof oh2->digest);
+		*p = hcb_r_node(oh2);
+		return (oh2);
 	}
 
 	while(hcb_is_y(pp)) {
 		y = hcb_l_y(pp);
 		CHECK_OBJ_NOTNULL(y, HCB_Y_MAGIC);
 		assert(y->ptr < DIGEST_LEN);
-		s = (oh->digest[y->ptr] & y->bitmask) != 0;
+		s = (digest[y->ptr] & y->bitmask) != 0;
 		assert(s < 2);
 		p = &y->leaf[s];
 		pp = *p;
@@ -221,7 +223,7 @@ hcb_insert(struct worker *wrk, struct hcb_root *root, struct objhead *oh,
 
 	if (pp == 0) {
 		/* We raced hcb_delete and got a NULL pointer */
-		assert(!has_lock);
+		assert(noh == NULL);
 		return (NULL);
 	}
 
@@ -230,20 +232,23 @@ hcb_insert(struct worker *wrk, struct hcb_root *root, struct objhead *oh,
 	/* We found a node, does it match ? */
 	oh2 = hcb_l_node(pp);
 	CHECK_OBJ_NOTNULL(oh2, OBJHEAD_MAGIC);
-	if (!memcmp(oh2->digest, oh->digest, DIGEST_LEN))
+	if (!memcmp(oh2->digest, digest, DIGEST_LEN))
 		return (oh2);
 
-	if (!has_lock)
+	if (noh == NULL)
 		return (NULL);
 
 	/* Insert */
 
 	CAST_OBJ_NOTNULL(y2, wrk->nhashpriv, HCB_Y_MAGIC);
 	wrk->nhashpriv = NULL;
-	(void)hcb_crit_bit(oh, oh2, y2);
-	s2 = (oh->digest[y2->ptr] & y2->bitmask) != 0;
+	(void)hcb_crit_bit(digest, oh2, y2);
+	s2 = (digest[y2->ptr] & y2->bitmask) != 0;
 	assert(s2 < 2);
-	y2->leaf[s2] = hcb_r_node(oh);
+	oh2 = *noh;
+	*noh = NULL;
+	memcpy(oh2->digest, digest, sizeof oh2->digest);
+	y2->leaf[s2] = hcb_r_node(oh2);
 	s2 = 1-s2;
 
 	p = &root->origo;
@@ -256,14 +261,14 @@ hcb_insert(struct worker *wrk, struct hcb_root *root, struct objhead *oh,
 		if (y->critbit > y2->critbit)
 			break;
 		assert(y->ptr < DIGEST_LEN);
-		s = (oh->digest[y->ptr] & y->bitmask) != 0;
+		s = (digest[y->ptr] & y->bitmask) != 0;
 		assert(s < 2);
 		p = &y->leaf[s];
 	}
 	y2->leaf[s2] = *p;
 	VWMB();
 	*p = hcb_r_y(y2);
-	return(oh);
+	return(oh2);
 }
 
 /*--------------------------------------------------------------------*/
@@ -412,45 +417,50 @@ hcb_deref(struct objhead *oh)
 }
 
 static struct objhead * __match_proto__(hash_lookup_f)
-hcb_lookup(struct worker *wrk, struct objhead *noh)
+hcb_lookup(struct worker *wrk, const void *digest, struct objhead **noh)
 {
 	struct objhead *oh;
 	struct hcb_y *y;
 	unsigned u;
-	unsigned with_lock;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
+	AN(digest);
+	AN(noh);
+	CHECK_OBJ_NOTNULL(*noh, OBJHEAD_MAGIC);
+	assert((*noh)->refcnt == 1);
+
+	/* First try in read-only mode without holding a lock */
+
+	VSC_C_main->hcb_nolock++;
+	oh = hcb_insert(wrk, &hcb_root, digest, NULL);
+	if (oh != NULL) {
+		Lck_Lock(&oh->mtx);
+		/*
+		 * A refcount of zero indicates that the tree changed
+		 * under us, so fall through and try with the lock held.
+		 */
+		u = oh->refcnt;
+		if (u > 0)
+			oh->refcnt++;
+		Lck_Unlock(&oh->mtx);
+		if (u > 0)
+			return (oh);
+	}
 
-	with_lock = 0;
 	while (1) {
-		if (with_lock) {
-			CAST_OBJ_NOTNULL(y, wrk->nhashpriv, HCB_Y_MAGIC);
-			Lck_Lock(&hcb_mtx);
-			VSC_C_main->hcb_lock++;
-			assert(noh->refcnt == 1);
-			oh = hcb_insert(wrk, &hcb_root, noh, 1);
-			Lck_Unlock(&hcb_mtx);
-		} else {
-			VSC_C_main->hcb_nolock++;
-			oh = hcb_insert(wrk, &hcb_root, noh, 0);
-		}
+		/* No luck, try with lock held, so we can modify tree */
+		CAST_OBJ_NOTNULL(y, wrk->nhashpriv, HCB_Y_MAGIC);
+		Lck_Lock(&hcb_mtx);
+		VSC_C_main->hcb_lock++;
+		oh = hcb_insert(wrk, &hcb_root, digest, noh);
+		Lck_Unlock(&hcb_mtx);
 
-		if (oh != NULL && oh == noh) {
-			/* Assert that we only muck with the tree with a lock */
-			assert(with_lock);
-			VSC_C_main->hcb_insert++;
+		CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+		if (*noh == NULL) {
 			assert(oh->refcnt > 0);
+			VSC_C_main->hcb_insert++;
 			return (oh);
 		}
-
-		if (oh == NULL) {
-			assert(!with_lock);
-			/* Try again, with lock */
-			with_lock = 1;
-			continue;
-		}
-
-		CHECK_OBJ_NOTNULL(noh, OBJHEAD_MAGIC);
 		Lck_Lock(&oh->mtx);
 		/*
 		 * A refcount of zero indicates that the tree changed
@@ -459,8 +469,6 @@ hcb_lookup(struct worker *wrk, struct objhead *noh)
 		u = oh->refcnt;
 		if (u > 0)
 			oh->refcnt++;
-		else
-			with_lock = 1;
 		Lck_Unlock(&oh->mtx);
 		if (u > 0)
 			return (oh);

bin/varnishd/hash/hash_simple_list.c

@@ -60,16 +60,19 @@ hsl_start(void)
  */
 
 static struct objhead * __match_proto__(hash_lookup_f)
-hsl_lookup(struct worker *wrk, struct objhead *noh)
+hsl_lookup(struct worker *wrk, const void *digest, struct objhead **noh)
 {
 	struct objhead *oh;
 	int i;
 
 	CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
-	CHECK_OBJ_NOTNULL(noh, OBJHEAD_MAGIC);
+	AN(digest);
+	AN(noh);
+	CHECK_OBJ_NOTNULL(*noh, OBJHEAD_MAGIC);
+
 	Lck_Lock(&hsl_mtx);
 	VTAILQ_FOREACH(oh, &hsl_head, hoh_list) {
-		i = memcmp(oh->digest, noh->digest, sizeof oh->digest);
+		i = memcmp(oh->digest, digest, sizeof oh->digest);
 		if (i < 0)
 			continue;
 		if (i > 0)
@@ -80,12 +83,15 @@ hsl_lookup(struct worker *wrk, struct objhead *noh)
 	}
 
 	if (oh != NULL)
-		VTAILQ_INSERT_BEFORE(oh, noh, hoh_list);
+		VTAILQ_INSERT_BEFORE(oh, *noh, hoh_list);
 	else
-		VTAILQ_INSERT_TAIL(&hsl_head, noh, hoh_list);
+		VTAILQ_INSERT_TAIL(&hsl_head, *noh, hoh_list);
 
+	oh = *noh;
+	*noh = NULL;
+	memcpy(oh->digest, digest, sizeof oh->digest);
 	Lck_Unlock(&hsl_mtx);
-	return (noh);
+	return (oh);
 }
 
 /*--------------------------------------------------------------------

bin/varnishd/hash/hash_slinger.h

@@ -35,8 +35,8 @@ struct object;
 typedef void hash_init_f(int ac, char * const *av);
 typedef void hash_start_f(void);
 typedef void hash_prep_f(struct worker *);
-typedef struct objhead *
-    hash_lookup_f(struct worker *wrk, struct objhead *nobj);
+typedef struct objhead *hash_lookup_f(struct worker *wrk, const void *digest,
+    struct objhead **nobj);
 typedef int hash_deref_f(struct objhead *obj);
 
 struct hash_slinger {