r4549 - trunk/varnish-cache/bin/varnishd

phk at projects.linpro.no phk at projects.linpro.no
Wed Feb 10 13:48:18 CET 2010


Author: phk
Date: 2010-02-10 13:48:17 +0100 (Wed, 10 Feb 2010)
New Revision: 4549

Modified:
   trunk/varnish-cache/bin/varnishd/hash_critbit.c
Log:
Close what might be a race, and assert that it doesn't happen:

When we do a lookup, we first try without the lock, if this gives
us a refcnt==0 objhead, we try again, with the lock.

When we deref an objhead, we would decrement the refcnt, holding oh->mtx,
then lock the critbit-lock and remove it from the tree.

It might be possible for a locked lookup to find an oh we just decremented
the refcnt off, and we did not check for refcnt==0 in that case.

Fix this, by removing the oh from the tree, holding the critbit-lock,
before decrementing the refcnt.  This way, a locked lookup can never
find a refcnt==0 oh in the tree, and the unlocked lookup still catches
this with the refcnt==0 check.




Modified: trunk/varnish-cache/bin/varnishd/hash_critbit.c
===================================================================
--- trunk/varnish-cache/bin/varnishd/hash_critbit.c	2010-02-10 12:44:42 UTC (rev 4548)
+++ trunk/varnish-cache/bin/varnishd/hash_critbit.c	2010-02-10 12:48:17 UTC (rev 4549)
@@ -216,6 +216,7 @@
 
 	/* 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))
 		return (oh2);
 
@@ -357,6 +358,8 @@
 		(void)sleep(1);
 		Lck_Lock(&hcb_mtx);
 		VTAILQ_FOREACH_SAFE(oh, &laylow, coollist, oh2) {
+			CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
+			AZ(oh->refcnt);
 			y = (void *)&oh->u;
 			if (y->leaf[0] || y->leaf[1])
 				continue;
@@ -365,7 +368,6 @@
 #ifdef PHK
 				fprintf(stderr, "OH %p is cold enough\n", oh);
 #endif
-				AZ(oh->refcnt);
 				HSH_DeleteObjHead(&ww, oh);
 			}
 		}
@@ -401,7 +403,8 @@
 	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
 	Lck_Lock(&oh->mtx);
 	assert(oh->refcnt > 0);
-	if (--oh->refcnt == 0) {
+	if (oh->refcnt == 1) {
+		/* Remove from tree before decrementing refcnt to zero */
 		Lck_Lock(&hcb_mtx);
 		hcb_delete(&hcb_root, oh);
 		assert(VTAILQ_EMPTY(&oh->objcs));
@@ -410,6 +413,7 @@
 		VTAILQ_INSERT_TAIL(&laylow, oh, coollist);
 		Lck_Unlock(&hcb_mtx);
 	}
+	oh->refcnt--;
 	Lck_Unlock(&oh->mtx);
 #ifdef PHK
 	fprintf(stderr, "hcb_defef %d %d <%s>\n", __LINE__, r, oh->hash);
@@ -452,6 +456,7 @@
 	oh =  hcb_insert(&hcb_root, noh, 1);
 	if (oh == noh) {
 		VSL_stats->hcb_insert++;
+		assert(oh->refcnt > 0);
 #ifdef PHK
 		fprintf(stderr, "hcb_lookup %d\n", __LINE__);
 #endif
@@ -462,6 +467,7 @@
 		fprintf(stderr, "hcb_lookup %d\n", __LINE__);
 #endif
 		Lck_Lock(&oh->mtx);
+		assert(oh->refcnt > 0);
 		oh->refcnt++;
 		Lck_Unlock(&oh->mtx);
 	}



More information about the varnish-commit mailing list