r4722 - in branches/2.1: . varnish-cache/bin/varnishd varnish-cache/bin/varnishtest/tests varnish-cache/include varnish-cache/lib/libvarnish varnish-cache/lib/libvcl

tfheen at varnish-cache.org tfheen at varnish-cache.org
Mon Apr 26 09:52:10 CEST 2010


Author: tfheen
Date: 2010-04-26 09:52:09 +0200 (Mon, 26 Apr 2010)
New Revision: 4722

Modified:
   branches/2.1/
   branches/2.1/varnish-cache/bin/varnishd/cache_backend.h
   branches/2.1/varnish-cache/bin/varnishd/cache_backend_cfg.c
   branches/2.1/varnish-cache/bin/varnishd/hash_critbit.c
   branches/2.1/varnish-cache/bin/varnishd/vparam.h
   branches/2.1/varnish-cache/bin/varnishtest/tests/c00019.vtc
   branches/2.1/varnish-cache/bin/varnishtest/tests/r00325.vtc
   branches/2.1/varnish-cache/bin/varnishtest/tests/r00416.vtc
   branches/2.1/varnish-cache/bin/varnishtest/tests/v00011.vtc
   branches/2.1/varnish-cache/include/vct.h
   branches/2.1/varnish-cache/include/vev.h
   branches/2.1/varnish-cache/lib/libvarnish/tcp.c
   branches/2.1/varnish-cache/lib/libvarnish/vev.c
   branches/2.1/varnish-cache/lib/libvcl/vcc_dir_random.c
Log:
Merge r4717: Fix a deadlock in the critbit hasher.

Spotted by: dormando
Testing by: Kristian

Fixes: #684



Property changes on: branches/2.1
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk:4637,4640,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk:4637,4640,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/bin/varnishd/cache_backend.h
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/bin/varnishd/cache_backend.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/bin/varnishd/cache_backend.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/bin/varnishd/cache_backend_cfg.c
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/bin/varnishd/cache_backend_cfg.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/bin/varnishd/cache_backend_cfg.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717

Modified: branches/2.1/varnish-cache/bin/varnishd/hash_critbit.c
===================================================================
--- branches/2.1/varnish-cache/bin/varnishd/hash_critbit.c	2010-04-26 07:36:36 UTC (rev 4721)
+++ branches/2.1/varnish-cache/bin/varnishd/hash_critbit.c	2010-04-26 07:52:09 UTC (rev 4722)
@@ -160,7 +160,9 @@
 	return ((struct hcb_y *)(u & ~HCB_BIT_Y));
 }
 
-/**********************************************************************/
+/**********************************************************************
+ * Find the "critical" bit that separates these two digests
+ */
 
 static unsigned
 hcb_crit_bit(const struct objhead *oh1, const struct objhead *oh2,
@@ -238,6 +240,7 @@
 
 	while(hcb_is_y(*p)) {
 		y = hcb_l_y(*p);
+		assert(y->critbit != y2->critbit);
 		if (y->critbit > y2->critbit)
 			break;
 		assert(y->ptr < DIGEST_LEN);
@@ -353,7 +356,6 @@
 	THR_SetName("hcb_cleaner");
 	(void)priv;
 	while (1) {
-		(void)sleep(1);
 		Lck_Lock(&hcb_mtx);
 		VTAILQ_FOREACH_SAFE(oh, &laylow, coollist, oh2) {
 			CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
@@ -363,14 +365,16 @@
 				continue;
 			if (++oh->digest[0] > params->critbit_cooloff) {
 				VTAILQ_REMOVE(&laylow, oh, coollist);
-#ifdef PHK
-				fprintf(stderr, "OH %p is cold enough\n", oh);
-#endif
-				HSH_DeleteObjHead(&ww, oh);
+				break;
 			}
 		}
 		Lck_Unlock(&hcb_mtx);
-		WRK_SumStat(&ww);
+		if (oh == NULL) {
+			WRK_SumStat(&ww);
+			(void)sleep(1);
+		} else {
+			HSH_DeleteObjHead(&ww, oh);
+		}
 	}
 	NEEDLESS_RETURN(NULL);
 }
@@ -385,11 +389,11 @@
 
 	(void)oh;
 	CLI_AddFuncs(hcb_cmds);
+	Lck_New(&hcb_mtx);
 	AZ(pthread_create(&tp, NULL, hcb_cleaner, NULL));
 	assert(sizeof(struct hcb_y) <= sizeof(oh->u));
 	memset(&hcb_root, 0, sizeof hcb_root);
 	hcb_build_bittbl();
-	Lck_New(&hcb_mtx);
 }
 
 static int
@@ -401,8 +405,8 @@
 	CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
 	Lck_Lock(&oh->mtx);
 	assert(oh->refcnt > 0);
-	if (oh->refcnt == 1) {
-		/* Remove from tree before decrementing refcnt to zero */
+	oh->refcnt--;
+	if (oh->refcnt == 0) {
 		Lck_Lock(&hcb_mtx);
 		hcb_delete(&hcb_root, oh);
 		assert(VTAILQ_EMPTY(&oh->objcs));
@@ -411,7 +415,6 @@
 		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);
@@ -423,54 +426,52 @@
 hcb_lookup(const struct sess *sp, struct objhead *noh)
 {
 	struct objhead *oh;
-	unsigned u;
+	volatile unsigned u;
+	unsigned with_lock;
 
 	(void)sp;
-	oh =  hcb_insert(&hcb_root, noh, 0);
-	if (oh != NULL) {
-		/* Assert that we didn't muck with the tree without lock */
-		assert(oh != noh);
+	
+	with_lock = 0;
+	while (1) {
+		if (with_lock) {
+			Lck_Lock(&hcb_mtx);
+			VSL_stats->hcb_lock++;
+			assert(noh->refcnt == 1);
+			oh =  hcb_insert(&hcb_root, noh, 1);
+			Lck_Unlock(&hcb_mtx);
+		} else {
+			VSL_stats->hcb_nolock++;
+			oh =  hcb_insert(&hcb_root, noh, 0);
+		}
+
+		if (oh != NULL && oh == noh) {
+			/* Assert that we only muck with the tree with a lock */
+			assert(with_lock);
+			VSL_stats->hcb_insert++;
+			assert(oh->refcnt > 0);
+			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
 		 * under us, so fall through and try with the lock held.
 		 */
 		u = oh->refcnt;
-		if (u)
+		if (u > 0)
 			oh->refcnt++;
 		Lck_Unlock(&oh->mtx);
-		if (u) {
-			VSL_stats->hcb_nolock++;
+		if (u > 0)
 			return (oh);
-		}
 	}
-
-	/*
-	 * Try again, holding lock and fully ready objhead, so that if
-	 * somebody else beats us back, they do not get surprised.
-	 */
-	Lck_Lock(&hcb_mtx);
-	assert(noh->refcnt == 1);
-	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
-	} else {
-		CHECK_OBJ_NOTNULL(noh, OBJHEAD_MAGIC);
-		VSL_stats->hcb_lock++;
-#ifdef PHK
-		fprintf(stderr, "hcb_lookup %d\n", __LINE__);
-#endif
-		Lck_Lock(&oh->mtx);
-		assert(oh->refcnt > 0);
-		oh->refcnt++;
-		Lck_Unlock(&oh->mtx);
-	}
-	Lck_Unlock(&hcb_mtx);
-	return (oh);
 }
 
 


Property changes on: branches/2.1/varnish-cache/bin/varnishd/vparam.h
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/bin/varnishd/vparam.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/bin/varnishd/vparam.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/bin/varnishtest/tests/c00019.vtc
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/bin/varnishtest/tests/c00019.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/bin/varnishtest/tests/c00019.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/bin/varnishtest/tests/r00325.vtc
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/bin/varnishtest/tests/r00325.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/bin/varnishtest/tests/r00325.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/bin/varnishtest/tests/r00416.vtc
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/bin/varnishtest/tests/r00416.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/bin/varnishtest/tests/r00416.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/bin/varnishtest/tests/v00011.vtc
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/bin/varnishtest/tests/v00011.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/bin/varnishtest/tests/v00011.vtc:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/include/vct.h
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/include/vct.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/include/vct.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/include/vev.h
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/include/vev.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/include/vev.h:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/lib/libvarnish/tcp.c
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/lib/libvarnish/tcp.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/lib/libvarnish/tcp.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/lib/libvarnish/vev.c
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/lib/libvarnish/vev.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/lib/libvarnish/vev.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717


Property changes on: branches/2.1/varnish-cache/lib/libvcl/vcc_dir_random.c
___________________________________________________________________
Modified: svn:mergeinfo
   - /trunk/varnish-cache/lib/libvcl/vcc_dir_random.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4716
   + /trunk/varnish-cache/lib/libvcl/vcc_dir_random.c:4637,4643-4645,4647-4650,4654-4670,4686,4689-4690,4700,4712,4715-4717




More information about the varnish-commit mailing list