Skip to content
This repository was archived by the owner on Feb 16, 2026. It is now read-only.

Stash objcore references until the end of the task to avoid copies #4269

Open
nigoroll wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Stash objcore references until the end of the task to avoid copies #4269

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
881bc63
params: give a sensible upper limit to max_restarts and max_retries
nigoroll Feb 14, 2025
74b45e0
req: New facility to retain objcores
nigoroll Feb 13, 2025
59ca693
cache_vrt: Avoid making unnecessary copies of strings
nigoroll Feb 11, 2025
96f2da6
req: test our new ocstash's capability
nigoroll Feb 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions bin/varnishd/cache/cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))
Expand Down
65 changes: 63 additions & 2 deletions bin/varnishd/cache/cache_req.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
*/
Comment on lines +49 to +51
Copy link
Copy Markdown
Member

@dridi dridi Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My suggestion:

Facility to keep references of discarded objcores exposed to VCL code.

Copy link
Copy Markdown
Member Author

@nigoroll nigoroll Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"discarded objcores" to me sounds like they would be removed from cache.


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)
{
Expand Down Expand Up @@ -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;

Expand Down Expand Up @@ -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);
Expand All @@ -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);
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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);

Expand Down
9 changes: 4 additions & 5 deletions bin/varnishd/cache/cache_req_fsm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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:
Expand All @@ -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);
}
Expand Down
1 change: 1 addition & 0 deletions bin/varnishd/cache/cache_varnishd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 *);
Expand Down
19 changes: 5 additions & 14 deletions bin/varnishd/cache/cache_vrt.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
Comment on lines -98 to -104
Copy link
Copy Markdown
Member

@dridi dridi Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be safe actually:

return (synth(200, some_vmod.some_string()));

In that case we can't assume a lifetime beyond the VRT_synth() call for the reason argument.

Copy link
Copy Markdown
Member Author

@nigoroll nigoroll Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see #4269 (comment) regarding this and the next four review comments.


if (ctx->req == NULL) {
CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC);
ctx->bo->err_code = (uint16_t)code;
Expand Down Expand Up @@ -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) {
Comment on lines -462 to +458
Copy link
Copy Markdown
Member

@dridi dridi Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likewise, we can't assume a task lifetime of the arguments.

for (i++; i < s->n; i++)
if (s->p[i] != NULL && *s->p[i] != '\0')
break;
Expand Down
10 changes: 7 additions & 3 deletions bin/varnishtest/tests/b00092.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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


Expand All @@ -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);
Expand All @@ -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
1 change: 1 addition & 0 deletions bin/varnishtest/tests/r02219.vtc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
4 changes: 2 additions & 2 deletions include/tbl/params.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -786,7 +786,7 @@ PARAM_SIMPLE(
/* name */ max_restarts,
/* type */ uint,
/* min */ "0",
/* max */ NULL,
/* max */ "32766", // (1<<15)-2 #4269
/* def */ "4",
/* units */ "restarts",
/* descr */
Expand All @@ -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",
Comment on lines 798 to 803
Copy link
Copy Markdown
Member

@dridi dridi Feb 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it was up to me, the limit would be 20.

/* descr */
Expand Down
35 changes: 29 additions & 6 deletions vmod/vmod_debug.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Comment on lines 648 to -650
Copy link
Copy Markdown
Member

@dridi dridi Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If our concern is coverage for VRT_StrandsWS() then we need to keep a check that the result effectively belongs to the workspace.

return (r);
}

Expand All @@ -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));
Comment on lines -661 to -662
Copy link
Copy Markdown
Member

@dridi dridi Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous.

return (r);
}

Expand Down Expand Up @@ -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));
Comment on lines -695 to -696
Copy link
Copy Markdown
Member

@dridi dridi Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous.

http_Unset(hp, hdr->what);
http_SetHeader(hp, b);
}
Expand Down Expand Up @@ -1373,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);
}
14 changes: 14 additions & 0 deletions vmod/vmod_debug.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
4 changes: 4 additions & 0 deletions vmod/vmod_debug.vcc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
==========

Expand Down
Loading