From 881bc63d2f8b4b655443b72a39dd6d1113649def Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Fri, 14 Feb 2025 14:59:10 +0100 Subject: [PATCH 1/4] params: give a sensible upper limit to max_restarts and max_retries "almost 64k restarts ought to be enough for everyone" --- include/tbl/params.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/tbl/params.h b/include/tbl/params.h index e66a653cb6..4bdf0bf431 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -786,7 +786,7 @@ PARAM_SIMPLE( /* name */ max_restarts, /* type */ uint, /* min */ "0", - /* max */ NULL, + /* max */ "65534", // (1<<16)-2 #4269 /* def */ "4", /* units */ "restarts", /* descr */ @@ -798,7 +798,7 @@ PARAM_SIMPLE( /* name */ max_retries, /* type */ uint, /* min */ "0", - /* max */ NULL, + /* max */ "65534", // (1<<16)-2 #4269 /* def */ "4", /* units */ "retries", /* descr */ From 74b45e0c189d080f1633f497ac265e2119a506b3 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Thu, 13 Feb 2025 08:51:21 +0100 Subject: [PATCH 2/4] req: New facility to retain objcores Context: Only while we hold a reference to an object are we allowed to access any attributes from it. With restarts, we might want to keep references to attributes of objects from previous restart iterations. Before this patch, this could lead to use-after-dereference, which was "fixed" by always copying object variables into the request's workspace (though this was problematic if VMODs did not copy always, as will be shown by a follow-up vtc addition). But that copyp is wasteful, so we should rather make sure that we keep references until we are done with the task and not copy (which is the second next commit). An argument had been made in the past that keeping object references until the end of the task would increase their lifetime by too much. It is true that, to support all possible use cases, we need to keep any oc references until the end of the task, which might include a final body delivery. But keeping the additional references can be avoided by either not restarting, or rolling back also. In general, until now we have charged all Varnish-Cache users with the cost for specific use cases, but we should rather only charge the specific use cases instead. Implementation: struct ocstash basically is a Variable Length Array (VLA) on the workspace, allocated when the request is created. For each invocation, ocstash_push() copies the passed objcore to one of max_restarts + 1 slots and clears the original location as HSH_DerefObjCore() would. Alternatives considered: * Dynamically allocating space for each objcore pointer was dismissed due to the substantial overhead. * Using a TASK_PRIV was tried and dismissed because this would require setting up a VRT_CTX or pulling the VRT_CTX out one level to the core of the FSM, which was intrusive and increased complexity substantially. * The original iteration of this patch would dynamically allocate the workspace for struct ocstash only when neeed, but after some involved discussion between Dridi and Nils this idea was given up in favor of the simpler upfront allocation. Co-authored-by: Dridi Boukelmoune --- bin/varnishd/cache/cache.h | 2 + bin/varnishd/cache/cache_req.c | 65 ++++++++++++++++++++++++++++- bin/varnishd/cache/cache_req_fsm.c | 9 ++-- bin/varnishd/cache/cache_varnishd.h | 1 + bin/varnishtest/tests/r02219.vtc | 1 + include/tbl/params.h | 2 +- 6 files changed, 72 insertions(+), 8 deletions(-) diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h index fc5b9cf933..d36390f50b 100644 --- a/bin/varnishd/cache/cache.h +++ b/bin/varnishd/cache/cache.h @@ -551,6 +551,8 @@ struct req { struct vrt_privs privs[1]; struct vcf *vcf; + + struct ocstash *stash; }; #define IS_TOPREQ(req) ((req)->top->topreq == (req)) diff --git a/bin/varnishd/cache/cache_req.c b/bin/varnishd/cache/cache_req.c index 2fa6ecd773..082f19bd07 100644 --- a/bin/varnishd/cache/cache_req.c +++ b/bin/varnishd/cache/cache_req.c @@ -39,12 +39,60 @@ #include "cache_varnishd.h" #include "cache_filter.h" +#include "cache_objhead.h" #include "cache_pool.h" #include "cache_transport.h" #include "common/heritage.h" #include "vtim.h" +/*-------------------------------------------------------------------- + * Facility to keep obcore references until the end of the task across restarts + */ + +struct ocstash { + unsigned magic; +#define OCSTASH_MAGIC 0x242031b5 + uint16_t l; + uint16_t n; + struct objcore * ocs[] v_counted_by_(l); +}; + +static void +ocstash_push(struct ocstash *stash, struct objcore **ocp) +{ + + CHECK_OBJ_NOTNULL(stash, OCSTASH_MAGIC); + assert(stash->n < stash->l); + TAKE_OBJ_NOTNULL(stash->ocs[stash->n], ocp, OBJCORE_MAGIC); + stash->n++; +} + +static void +ocstash_clear(struct worker *wrk, struct ocstash *stash) +{ + uint16_t u; + + CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); + CHECK_OBJ_NOTNULL(stash, OCSTASH_MAGIC); + assert(stash->n <= stash->l); + for (u = 0; u < stash->n; u++) + (void)HSH_DerefObjCore(wrk, &stash->ocs[u]); + for (; u < stash->l; u++) + AZ(stash->ocs[u]); + stash->n = 0; +} + +void +Req_StashObjcore(struct req *req, struct objcore **ocp) +{ + + CHECK_OBJ_NOTNULL(req, REQ_MAGIC); + ocstash_push(req->stash, ocp); +} + +/*--------------------------------------------------------------------*/ + void Req_AcctLogCharge(struct VSC_main_wrk *ds, struct req *req) { @@ -127,7 +175,7 @@ Req_New(struct sess *sp, const struct req *preq) { struct pool *pp; struct req *req; - uint16_t nhttp; + uint16_t l, nhttp; unsigned sz, hl; char *p, *e; @@ -191,6 +239,17 @@ Req_New(struct sess *sp, const struct req *preq) p = (void*)PRNDUP(p + sizeof(*req->top)); } + req->max_restarts = cache_param->max_restarts; + assert(req->max_restarts + 1 <= UINT16_MAX); + // each restart may ref one stale_oc and one oc + l = (2 * req->max_restarts) + 1; + sz = SIZEOF_FLEX_OBJ(req->stash, ocs, l); + req->stash = (void*)p; + req->stash->magic = OCSTASH_MAGIC; + req->stash->l = l; + p += sz; + p = (void*)PRNDUP(p); + assert(p < e); WS_Init(req->ws, "req", p, e - p); @@ -200,7 +259,6 @@ Req_New(struct sess *sp, const struct req *preq) req->t_req = NAN; req->req_step = R_STP_TRANSPORT; req->doclose = SC_NULL; - req->max_restarts = cache_param->max_restarts; return (req); } @@ -253,6 +311,7 @@ Req_Rollback(VRT_CTX) if (IS_TOPREQ(req)) VCL_TaskLeave(ctx, req->top->privs); VCL_TaskLeave(ctx, req->privs); + ocstash_clear(req->wrk, req->stash); VCL_TaskEnter(req->privs); if (IS_TOPREQ(req)) VCL_TaskEnter(req->top->privs); @@ -284,6 +343,8 @@ Req_Cleanup(struct sess *sp, struct worker *wrk, struct req *req) AZ(req->director_hint); req->restarts = 0; + ocstash_clear(wrk, req->stash); + if (req->vcl != NULL) VCL_Recache(wrk, &req->vcl); diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index 9dc0db092b..7fecf80996 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -240,7 +240,7 @@ cnt_deliver(struct worker *wrk, struct req *req) if (wrk->vpi->handling != VCL_RET_DELIVER) { HSH_Cancel(wrk, req->objcore, NULL); - (void)HSH_DerefObjCore(wrk, &req->objcore); + Req_StashObjcore(req, &req->objcore); http_Teardown(req->resp); switch (wrk->vpi->handling) { @@ -685,8 +685,7 @@ cnt_lookup(struct worker *wrk, struct req *req) WRONG("Illegal return from vcl_hit{}"); } - /* Drop our object, we won't need it */ - (void)HSH_DerefObjCore(wrk, &req->objcore); + Req_StashObjcore(req, &req->objcore); if (busy != NULL) { HSH_Withdraw(wrk, &busy); @@ -716,7 +715,7 @@ cnt_miss(struct worker *wrk, struct req *req) wrk->stats->cache_miss++; VBF_Fetch(wrk, req, req->objcore, req->stale_oc, VBF_NORMAL); if (req->stale_oc != NULL) - (void)HSH_DerefObjCore(wrk, &req->stale_oc); + Req_StashObjcore(req, &req->stale_oc); req->req_step = R_STP_FETCH; return (REQ_FSM_MORE); case VCL_RET_FAIL: @@ -736,7 +735,7 @@ cnt_miss(struct worker *wrk, struct req *req) } VRY_Clear(req); if (req->stale_oc != NULL) - (void)HSH_DerefObjCore(wrk, &req->stale_oc); + Req_StashObjcore(req, &req->stale_oc); HSH_Withdraw(wrk, &req->objcore); return (REQ_FSM_MORE); } diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h index 56ce1c14bc..a3212ae1d0 100644 --- a/bin/varnishd/cache/cache_varnishd.h +++ b/bin/varnishd/cache/cache_varnishd.h @@ -430,6 +430,7 @@ void Req_Fail(struct req *req, stream_close_t reason); void Req_AcctLogCharge(struct VSC_main_wrk *, struct req *); void Req_LogHit(struct worker *, struct req *, struct objcore *, intmax_t); const char *Req_LogStart(const struct worker *, struct req *); +void Req_StashObjcore(struct req *, struct objcore **); /* cache_req_body.c */ int VRB_Ignore(struct req *); diff --git a/bin/varnishtest/tests/r02219.vtc b/bin/varnishtest/tests/r02219.vtc index 86403fa532..63c0c03aa5 100644 --- a/bin/varnishtest/tests/r02219.vtc +++ b/bin/varnishtest/tests/r02219.vtc @@ -11,6 +11,7 @@ server s1 { varnish v1 -arg "-p workspace_client=9k" \ -arg "-p vsl_buffer=4k" \ + -arg "-p max_restarts=2" \ -proto PROXY \ -vcl+backend { import std; diff --git a/include/tbl/params.h b/include/tbl/params.h index 4bdf0bf431..0b2b1c757e 100644 --- a/include/tbl/params.h +++ b/include/tbl/params.h @@ -786,7 +786,7 @@ PARAM_SIMPLE( /* name */ max_restarts, /* type */ uint, /* min */ "0", - /* max */ "65534", // (1<<16)-2 #4269 + /* max */ "32766", // (1<<15)-2 #4269 /* def */ "4", /* units */ "restarts", /* descr */ From 59ca693aacf1b6254715e6deed350b8c3ae14e21 Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Tue, 11 Feb 2025 12:13:44 +0100 Subject: [PATCH 3/4] cache_vrt: Avoid making unnecessary copies of strings --- bin/varnishd/cache/cache_vrt.c | 19 +++++-------------- vmod/vmod_debug.c | 6 ------ 2 files changed, 5 insertions(+), 20 deletions(-) diff --git a/bin/varnishd/cache/cache_vrt.c b/bin/varnishd/cache/cache_vrt.c index 2847e8b874..343e928a81 100644 --- a/bin/varnishd/cache/cache_vrt.c +++ b/bin/varnishd/cache/cache_vrt.c @@ -95,14 +95,6 @@ VRT_synth(VRT_CTX, VCL_INT code, VCL_STRING reason) return; } - if (reason && !WS_Allocated(ctx->ws, reason, -1)) { - reason = WS_Copy(ctx->ws, reason, -1); - if (!reason) { - VRT_fail(ctx, "Workspace overflow"); - return; - } - } - if (ctx->req == NULL) { CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC); ctx->bo->err_code = (uint16_t)code; @@ -459,12 +451,11 @@ VRT_StrandsWS(struct ws *ws, const char *h, VCL_STRANDS s) } } - if (q == NULL) { - if (h == NULL) - return (""); - if (WS_Allocated(ws, h, -1)) - return (h); - } else if (h == NULL && WS_Allocated(ws, q, -1)) { + if (q == NULL && h == NULL) + return (""); + if (q == NULL) + return (h); + if (h == NULL) { for (i++; i < s->n; i++) if (s->p[i] != NULL && *s->p[i] != '\0') break; diff --git a/vmod/vmod_debug.c b/vmod/vmod_debug.c index bdf11deafd..700bd5c77d 100644 --- a/vmod/vmod_debug.c +++ b/vmod/vmod_debug.c @@ -646,8 +646,6 @@ xyzzy_concatenate(VRT_CTX, VCL_STRANDS s) CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); r = VRT_StrandsWS(ctx->ws, NULL, s); - if (r != NULL && *r != '\0') - AN(WS_Allocated(ctx->ws, r, strlen(r) + 1)); return (r); } @@ -658,8 +656,6 @@ xyzzy_collect(VRT_CTX, VCL_STRANDS s) CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); r = VRT_STRANDS_string(ctx, s); - if (r != NULL && *r != '\0') - AN(WS_Allocated(ctx->ws, r, strlen(r) + 1)); return (r); } @@ -692,8 +688,6 @@ xyzzy_sethdr(VRT_CTX, VCL_HEADER hdr, VCL_STRANDS s) VSLbs(ctx->vsl, SLT_LostHeader, TOSTRAND(hdr->what->str)); } else { - if (*b != '\0') - AN(WS_Allocated(hp->ws, b, strlen(b) + 1)); http_Unset(hp, hdr->what); http_SetHeader(hp, b); } From 96f2da6b375705a3683b07b6c34c5195aba3136d Mon Sep 17 00:00:00 2001 From: Nils Goroll Date: Fri, 14 Feb 2025 14:39:36 +0100 Subject: [PATCH 4/4] req: test our new ocstash's capability --- bin/varnishtest/tests/b00092.vtc | 10 +++-- vmod/vmod_debug.c | 29 +++++++++++++ vmod/vmod_debug.h | 14 +++++++ vmod/vmod_debug.vcc | 4 ++ vmod/vmod_debug_filters.c | 70 ++++++++++++++++++++++++++++++++ 5 files changed, 124 insertions(+), 3 deletions(-) diff --git a/bin/varnishtest/tests/b00092.vtc b/bin/varnishtest/tests/b00092.vtc index 878c68dd65..71264d77f7 100644 --- a/bin/varnishtest/tests/b00092.vtc +++ b/bin/varnishtest/tests/b00092.vtc @@ -2,11 +2,11 @@ varnishtest "Use multiple objects for a single request" server s0 { rxreq - txresp -hdr "method: O1_METHOD" + txresp -hdr "method: O1_METHOD" -hdr "bodypart: head," rxreq - txresp -hdr "url: /o2_url" + txresp -hdr "url: /o2_url" -hdr "bodypart: arms," rxreq - txresp -hdr "val: 3" + txresp -hdr "val: 3" -body "all the rest" } -start @@ -25,14 +25,17 @@ varnish v1 -vcl+backend { if (req.restarts == 0) { set req.method = resp.http.method; debug.log_strands("resp.http.method", resp.http.method); + debug.body_prefix(resp.http.bodypart); } else if (req.restarts == 1) { set req.url = resp.http.url; debug.log_strands("resp.http.url", resp.http.url); + debug.body_prefix(resp.http.bodypart); } else { set resp.http.method = req.method; set resp.http.url = req.url; debug.log_strands("req.method", req.method); debug.log_strands("req.url", req.url); + set resp.filters = "debug.body_prefix"; return (deliver); } return (restart); @@ -45,4 +48,5 @@ client c1 { expect resp.http.method == O1_METHOD expect resp.http.url == /o2_url expect resp.http.val == 3 + expect resp.body == "head,arms,all the rest" } -run diff --git a/vmod/vmod_debug.c b/vmod/vmod_debug.c index 700bd5c77d..d23db71e96 100644 --- a/vmod/vmod_debug.c +++ b/vmod/vmod_debug.c @@ -1367,3 +1367,32 @@ xyzzy_log_strands(VRT_CTX, VCL_STRING prefix, VCL_STRANDS subject, VCL_INT nn) ptr_where(ctx, p), p, n, p, strlen(p) > (unsigned)n ? "..." : ""); } } + +const void * const priv_task_id_bp = &priv_task_id_bp; + +VCL_VOID +xyzzy_body_prefix(VRT_CTX, VCL_STRING s) +{ + struct xyzzy_bp_string *bps; + struct xyzzy_bp *bp; + struct vmod_priv *task; + + CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + + task = VRT_priv_task(ctx, priv_task_id_bp); + AN(task); + bp = task->priv; + if (bp == NULL) { + bp = WS_Alloc(ctx->ws, (unsigned)sizeof *bp); + AN(bp); + task->priv = bp; + INIT_OBJ(bp, XYZZY_BP_MAGIC); + VSTAILQ_INIT(&bp->head); + } + CHECK_OBJ(bp, XYZZY_BP_MAGIC); + + bps = WS_Alloc(ctx->ws, (unsigned)sizeof *bps); + AN(bps); + bps->s = s; + VSTAILQ_INSERT_TAIL(&bp->head, bps, list); +} diff --git a/vmod/vmod_debug.h b/vmod/vmod_debug.h index 641148bfd8..6b51b1a10f 100644 --- a/vmod/vmod_debug.h +++ b/vmod/vmod_debug.h @@ -34,6 +34,20 @@ debug_add_filters(VRT_CTX); void debug_remove_filters(VRT_CTX); +/* body_prefix vdp */ +struct xyzzy_bp_string { + VSTAILQ_ENTRY(xyzzy_bp_string) list; + VCL_STRING s; +}; + +struct xyzzy_bp { + unsigned magic; +#define XYZZY_BP_MAGIC 0x88db5d93 + VSTAILQ_HEAD(,xyzzy_bp_string) head; +}; + +extern const void * const priv_task_id_bp; + /* vmod_debug_transport_reembarking_http1.c */ void debug_transport_reembarking_http1_use(VRT_CTX); diff --git a/vmod/vmod_debug.vcc b/vmod/vmod_debug.vcc index d752c1ebad..9afb407afa 100644 --- a/vmod/vmod_debug.vcc +++ b/vmod/vmod_debug.vcc @@ -498,6 +498,10 @@ $Restrict vcl_deliver Switch to the VAI http1 debug transport. Calling it from any other transport than http1 results in VCL failure. +$Function VOID body_prefix(STRING) + +Add this string to the prefix sent by the body_prefix filter + DEPRECATED ========== diff --git a/vmod/vmod_debug_filters.c b/vmod/vmod_debug_filters.c index 9dc6921518..7fb0b811dd 100644 --- a/vmod/vmod_debug_filters.c +++ b/vmod/vmod_debug_filters.c @@ -776,6 +776,74 @@ static const struct vdp xyzzy_vdp_awshog = { .init = xyzzy_awshog_init }; +/**********************************************************************/ + +static int v_matchproto_(vdp_init_f) +xyzzy_vdp_body_prefix_init(VRT_CTX, struct vdp_ctx *vdc, void **priv) +{ + struct xyzzy_bp_string *bps; + struct xyzzy_bp *bp; + struct vmod_priv *task; + intmax_t l; + + CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); + CHECK_OBJ_NOTNULL(vdc, VDP_CTX_MAGIC); + AN(vdc->clen); + + task = VRT_priv_task_get(ctx, priv_task_id_bp); + if (task == NULL) + return (1); + CAST_OBJ_NOTNULL(bp, task->priv, XYZZY_BP_MAGIC); + AN(priv); + AZ(*priv); + *priv = bp; + + if (*vdc->clen < 0) + return (0); + + l = 0; + VSTAILQ_FOREACH(bps, &bp->head, list) + l += strlen(bps->s); + *vdc->clen += l; + + return (0); +} + +static int v_matchproto_(vdp_bytes_f) +xyzzy_vdp_body_prefix_bytes(struct vdp_ctx *vdc, enum vdp_action act, void **priv, + const void *ptr, ssize_t len) +{ + struct xyzzy_bp_string *bps; + struct xyzzy_bp *bp; + + AN(priv); + CAST_OBJ_NOTNULL(bp, *priv, XYZZY_BP_MAGIC); + + while ((bps = VSTAILQ_FIRST(&bp->head)) != NULL) { + VSTAILQ_REMOVE_HEAD(&bp->head, list); + if (VDP_bytes(vdc, VDP_NULL, bps->s, strlen(bps->s))) + return (vdc->retval); + } + + return (VDP_bytes(vdc, act, ptr, len)); +} + +static int v_matchproto_(vdp_fini_f) +xyzzy_vdp_body_prefix_fini(struct vdp_ctx *vdc, void **priv) +{ + (void)vdc; + AN(priv); + *priv = NULL; + return (0); +} + +static const struct vdp xyzzy_vdp_body_prefix = { + .name = "debug.body_prefix", + .init = xyzzy_vdp_body_prefix_init, + .bytes = xyzzy_vdp_body_prefix_bytes, + .fini = xyzzy_vdp_body_prefix_fini +}; + void debug_add_filters(VRT_CTX) { @@ -787,6 +855,7 @@ debug_add_filters(VRT_CTX) AZ(VRT_AddFilter(ctx, NULL, &xyzzy_vdp_chkcrc32)); AZ(VRT_AddFilter(ctx, NULL, &xyzzy_vdp_chklen)); AZ(VRT_AddFilter(ctx, NULL, &xyzzy_vdp_awshog)); + AZ(VRT_AddFilter(ctx, NULL, &xyzzy_vdp_body_prefix)); } void @@ -800,4 +869,5 @@ debug_remove_filters(VRT_CTX) VRT_RemoveFilter(ctx, NULL, &xyzzy_vdp_chkcrc32); VRT_RemoveFilter(ctx, NULL, &xyzzy_vdp_chklen); VRT_RemoveFilter(ctx, NULL, &xyzzy_vdp_awshog); + VRT_RemoveFilter(ctx, NULL, &xyzzy_vdp_body_prefix); }