[6.0] 673ba3a7d Kill goto statements in vmod-shard

Reza Naghibi reza at naghibi.com
Wed Aug 19 13:17:06 UTC 2020


commit 673ba3a7df1218f629a487b40981f07cf1da2e59
Author: Dridi Boukelmoune <dridi.boukelmoune at gmail.com>
Date:   Mon Feb 25 18:49:06 2019 +0100

    Kill goto statements in vmod-shard
    
    With that, (almost) only libvgz carries goto statements from zlib.
    
    This isn't changing any of the previous semantics, in particular the
    AN(be) assertion from the "ok:" jump is honored by all code paths
    formerly leading to it.
    
    Previously, the bitmap was allocated on the stack prior to the magic
    check of shardd, which is now fixed at the expense of a potential code
    style violation. But more importantly, we currently read the number of
    backends prior to acquiring the read lock, but there is no evidence
    that this was done on purpose and not overlooked, besides moving a
    possibly expensive state initialization outside of the critical
    section.
    
    If that was on purpose, please document it.

diff --git a/lib/libvmod_directors/shard_dir.c b/lib/libvmod_directors/shard_dir.c
index a936e08d6..be240eb70 100644
--- a/lib/libvmod_directors/shard_dir.c
+++ b/lib/libvmod_directors/shard_dir.c
@@ -331,114 +331,135 @@ sharddir_any_healthy(struct sharddir *shardd, const struct busyobj *bo,
 	sharddir_unlock(shardd);
 	return (retval);
 }
+
 /*
  * core function for the director backend/resolve method
  */
-VCL_BACKEND
-sharddir_pick_be(VRT_CTX, struct sharddir *shardd,
+
+static VCL_BACKEND
+sharddir_pick_be_locked(VRT_CTX, struct sharddir *shardd,
     uint32_t key, VCL_INT alt, VCL_REAL warmup, VCL_BOOL rampup,
-    enum healthy_e healthy)
+    enum healthy_e healthy, struct shard_state *state)
 {
 	VCL_BACKEND be;
-	struct shard_state state;
-	unsigned picklist_sz = VBITMAP_SZ(shardd->n_backend);
-	char picklist_spc[picklist_sz];
 	VCL_DURATION chosen_r, alt_r;
 
 	CHECK_OBJ_NOTNULL(shardd, SHARDDIR_MAGIC);
 	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
 	AN(ctx->vsl);
 
-	memset(&state, 0, sizeof(state));
-	init_state(&state, ctx, shardd, vbit_init(picklist_spc, picklist_sz));
-
-	sharddir_rdlock(shardd);
 	if (shardd->n_backend == 0) {
 		shard_err0(ctx, shardd, "no backends");
-		goto err;
+		return (NULL);
 	}
 
 	assert(shardd->hashcircle);
 
 	validate_alt(ctx, shardd, &alt);
 
-	state.idx = shard_lookup(shardd, key);
-	assert(state.idx >= 0);
+	state->idx = shard_lookup(shardd, key);
+	assert(state->idx >= 0);
 
 	SHDBG(SHDBG_LOOKUP, shardd, "lookup key %x idx %d host %u",
-	    key, state.idx, shardd->hashcircle[state.idx].host);
+	    key, state->idx, shardd->hashcircle[state->idx].host);
 
 	if (alt > 0) {
-		if (shard_next(&state, alt - 1, healthy == ALL ? 1 : 0) == -1) {
-			if (state.previous.hostid != -1) {
+		if (shard_next(state, alt - 1, healthy == ALL ? 1 : 0) == -1) {
+			if (state->previous.hostid != -1) {
 				be = sharddir_backend(shardd,
-				    state.previous.hostid);
-				goto ok;
+				    state->previous.hostid);
+				AN(be);
+				return (be);
 			}
-			goto err;
+			return (NULL);
 		}
 	}
 
-	if (shard_next(&state, 0, healthy == IGNORE ? 0 : 1) == -1) {
-		if (state.previous.hostid != -1) {
-			be = sharddir_backend(shardd, state.previous.hostid);
-			goto ok;
+	if (shard_next(state, 0, healthy == IGNORE ? 0 : 1) == -1) {
+		if (state->previous.hostid != -1) {
+			be = sharddir_backend(shardd, state->previous.hostid);
+			AN(be);
+			return (be);
 		}
-		goto err;
+		return (NULL);
 	}
 
-	be = sharddir_backend(shardd, state.last.hostid);
+	be = sharddir_backend(shardd, state->last.hostid);
+	AN(be);
 
 	if (warmup == -1)
 		warmup = shardd->warmup;
 
 	/* short path for cases we dont want ramup/warmup or can't */
 	if (alt > 0 || healthy == IGNORE || (!rampup && warmup == 0) ||
-	    shard_next(&state, 0, 1) == -1)
-		goto ok;
+	    shard_next(state, 0, 1) == -1)
+		return (be);
 
 	assert(alt == 0);
-	assert(state.previous.hostid >= 0);
-	assert(state.last.hostid >= 0);
-	assert(state.previous.hostid != state.last.hostid);
-	assert(be == sharddir_backend(shardd, state.previous.hostid));
+	assert(state->previous.hostid >= 0);
+	assert(state->last.hostid >= 0);
+	assert(state->previous.hostid != state->last.hostid);
+	assert(be == sharddir_backend(shardd, state->previous.hostid));
 
-	chosen_r = shardcfg_get_rampup(shardd, state.previous.hostid);
-	alt_r = shardcfg_get_rampup(shardd, state.last.hostid);
+	chosen_r = shardcfg_get_rampup(shardd, state->previous.hostid);
+	alt_r = shardcfg_get_rampup(shardd, state->last.hostid);
 
 	SHDBG(SHDBG_RAMPWARM, shardd, "chosen host %d rampup %f changed %f",
-	    state.previous.hostid, chosen_r,
-	    ctx->now - state.previous.changed);
+	    state->previous.hostid, chosen_r,
+	    ctx->now - state->previous.changed);
 	SHDBG(SHDBG_RAMPWARM, shardd, "alt host %d rampup %f changed %f",
-	    state.last.hostid, alt_r,
-	    ctx->now - state.last.changed);
+	    state->last.hostid, alt_r,
+	    ctx->now - state->last.changed);
 
-	if (ctx->now - state.previous.changed < chosen_r) {
+	if (ctx->now - state->previous.changed < chosen_r) {
 		/*
 		 * chosen host is in rampup
 		 * - no change if alternative host is also in rampup or the dice
 		 *   has rolled in favour of the chosen host
 		 */
 		if (!rampup ||
-		    ctx->now - state.last.changed < alt_r ||
+		    ctx->now - state->last.changed < alt_r ||
 		    VRND_RandomTestableDouble() * chosen_r <
-		     (ctx->now - state.previous.changed))
-			goto ok;
+		    (ctx->now - state->previous.changed))
+			return (be);
 	} else {
 		/* chosen host not in rampup - warmup ? */
 		if (warmup == 0 || VRND_RandomTestableDouble() > warmup)
-			goto ok;
+			return (be);
 	}
 
-	be = sharddir_backend(shardd, state.last.hostid);
+	be = sharddir_backend(shardd, state->last.hostid);
+	return (be);
+}
 
-  ok:
-	AN(be);
+VCL_BACKEND
+sharddir_pick_be(VRT_CTX, struct sharddir *shardd,
+    uint32_t key, VCL_INT alt, VCL_REAL warmup, VCL_BOOL rampup,
+    enum healthy_e healthy)
+{
+	VCL_BACKEND be;
+	struct shard_state state[1];
+	unsigned picklist_sz;
+
+	CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC);
+	CHECK_OBJ_NOTNULL(shardd, SHARDDIR_MAGIC);
+
+	/* NB: Allocate bitmap on the stack
+	 *
+	 * XXX: Should we read n_backend under the lock and check whether
+	 * n_backend is zero before allocating the state?
+	 */
+	picklist_sz = VBITMAP_SZ(shardd->n_backend);
+	char picklist_spc[picklist_sz];
+
+	memset(state, 0, sizeof(state));
+	init_state(state, ctx, shardd, vbit_init(picklist_spc, picklist_sz));
+
+	sharddir_rdlock(shardd);
+	be = sharddir_pick_be_locked(ctx, shardd, key, alt, warmup, rampup,
+	    healthy, state);
 	sharddir_unlock(shardd);
-	vbit_destroy(state.picklist);
+
+	vbit_destroy(state->picklist);
 	return (be);
-  err:
-	sharddir_unlock(shardd);
-	vbit_destroy(state.picklist);
-	return NULL;
 }


More information about the varnish-commit mailing list