Hi,
following up an attempt to discuss this on irc:
The following discussion is, for the time being, regarding workpace_backend
only, but the question is relevant for the other workspaces also:
Relevant portions of workspace_backend are not available for use in VCL, but are
consumed in core code. Setting workspace_backend too low will trigger an
immediate assertion failure in VBO_GetBusyObj().
IMHO, it doesn't make sense to handle these assertions differently, because
space for http headers, vsl and the initial read from the backend are mandatory
for varnish to issue backend requests.
Instead of trying to better handle nonsense workspace_backend sizes, I'd suggest
to make it impossible to tune it stupidly in the first place - and to give
admins a tunable which is easier to understand.
The attached patch contains a PoC (this is _not_ a finished patch, see below)
suggesting the following changes:
- make workspace_backend a protected (read-only) parameter
- add headroom_backend as a new admin-tunable parameter which roughly equals to
"workspace available to VCL"
- calculate the size of workspace_backend based on all other relevant parameters
Notes on the patch:
- I've kept the workspace parameter as protected so the mempool code can stay
as is (pointing to a single parameter for sizing)
- in VBO_Init() I've taken a lightweight approach at the consistency issue from
parameter changes, and I'd be curious to hear if others agree or think
that we need to make this safer.
- what is the best way to cross the border between the cache and mgt code?
Originally I would have preferred to have the sizing code in cache_busyobj.c,
but then we'd need to make mgt_param visible there - which I didn't like.
But what I have in the patch now doesn't look clean at all, I am really
unsure at this point.
Thx, Nils
--
** * * UPLEX - Nils Goroll Systemoptimierung
Scheffelstraße 32
22301 Hamburg
tel +49 40 28805731
mob +49 170 2723133
fax +49 40 42949753
xmpp://[email protected]/
http://uplex.de/
diff --git a/bin/varnishd/cache/cache_busyobj.c b/bin/varnishd/cache/cache_busyobj.c
index c3fa436..3956bd6 100644
--- a/bin/varnishd/cache/cache_busyobj.c
+++ b/bin/varnishd/cache/cache_busyobj.c
@@ -45,6 +45,15 @@ static struct mempool *vbopool;
/*--------------------------------------------------------------------
*/
+size_t
+VBO_Sz(void)
+{
+ return sizeof(struct busyobj);
+}
+
+/*--------------------------------------------------------------------
+ */
+
void
VBO_Init(void)
{
@@ -91,40 +100,52 @@ VBO_GetBusyObj(struct worker *wrk, const struct req *req)
uint16_t nhttp;
unsigned sz;
char *p;
+ int tries = 0;
CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
- bo = vbo_New();
- CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
+ do {
+ bo = vbo_New();
+ CHECK_OBJ_NOTNULL(bo, BUSYOBJ_MAGIC);
+
+ p = (void*)(bo + 1);
+ p = (void*)PRNDUP(p);
+ assert(p < bo->end);
+
+ nhttp = (uint16_t)cache_param->http_max_hdr;
+ sz = HTTP_estimate(nhttp);
+
+ bo->bereq0 = HTTP_create(p, nhttp);
+ p += sz;
+ p = (void*)PRNDUP(p);
+ assert(p < bo->end);
+
+ bo->bereq = HTTP_create(p, nhttp);
+ p += sz;
+ p = (void*)PRNDUP(p);
+ assert(p < bo->end);
+
+ bo->beresp = HTTP_create(p, nhttp);
+ p += sz;
+ p = (void*)PRNDUP(p);
+ assert(p < bo->end);
+
+ sz = cache_param->vsl_buffer;
+ VSL_Setup(bo->vsl, p, sz);
+ bo->vsl->wid = VXID_Get(wrk, VSL_BACKENDMARKER);
+ p += sz;
+ p = (void*)PRNDUP(p);
+ assert(p < bo->end);
+
+ if (bo->end - p >= cache_param->headroom_backend)
+ break;
+
+ AN(tries++ < 3);
- p = (void*)(bo + 1);
- p = (void*)PRNDUP(p);
- assert(p < bo->end);
-
- nhttp = (uint16_t)cache_param->http_max_hdr;
- sz = HTTP_estimate(nhttp);
-
- bo->bereq0 = HTTP_create(p, nhttp);
- p += sz;
- p = (void*)PRNDUP(p);
- assert(p < bo->end);
-
- bo->bereq = HTTP_create(p, nhttp);
- p += sz;
- p = (void*)PRNDUP(p);
- assert(p < bo->end);
-
- bo->beresp = HTTP_create(p, nhttp);
- p += sz;
- p = (void*)PRNDUP(p);
- assert(p < bo->end);
-
- sz = cache_param->vsl_buffer;
- VSL_Setup(bo->vsl, p, sz);
- bo->vsl->wid = VXID_Get(wrk, VSL_BACKENDMARKER);
- p += sz;
- p = (void*)PRNDUP(p);
- assert(p < bo->end);
+ // too small, wait for parameter tweak to settle
+ vbo_Free(&bo);
+ usleep (100);
+ } while(1);
WS_Init(bo->ws, "bo", p, bo->end - p);
diff --git a/bin/varnishd/cache/cache_priv.h b/bin/varnishd/cache/cache_priv.h
index 2676680..0a5b582 100644
--- a/bin/varnishd/cache/cache_priv.h
+++ b/bin/varnishd/cache/cache_priv.h
@@ -56,6 +56,7 @@ int BAN_CheckObject(struct worker *, struct objcore *, struct req *);
/* cache_busyobj.c */
+size_t VBO_Sz(void);
void VBO_Init(void);
/* cache_cli.c [CLI] */
diff --git a/bin/varnishd/mgt/mgt_main.c b/bin/varnishd/mgt/mgt_main.c
index ec4ff5a..f1a05ef 100644
--- a/bin/varnishd/mgt/mgt_main.c
+++ b/bin/varnishd/mgt/mgt_main.c
@@ -443,7 +443,7 @@ init_params(struct cli *cli)
* VM space.
*/
MCF_SetDefault("workspace_client", "24k");
- MCF_SetDefault("workspace_backend", "16k");
+ MCF_SetDefault("headroom_backend", "8k");
MCF_SetDefault("http_resp_size", "8k");
MCF_SetDefault("http_req_size", "12k");
MCF_SetDefault("gzip_buffer", "4k");
diff --git a/bin/varnishd/mgt/mgt_param.c b/bin/varnishd/mgt/mgt_param.c
index 0ee9397..8f74b5e 100644
--- a/bin/varnishd/mgt/mgt_param.c
+++ b/bin/varnishd/mgt/mgt_param.c
@@ -362,6 +362,9 @@ MCF_ParamSet(struct cli *cli, const char *param, const char *val)
if (pp->func(cli->sb, pp, val))
VCLI_SetResult(cli, CLIS_PARAM);
+ if (pp->cb)
+ pp->cb(cli->sb, pp);
+
if (cli->result == CLIS_OK && heritage.param != NULL)
*heritage.param = mgt_param;
diff --git a/bin/varnishd/mgt/mgt_param.h b/bin/varnishd/mgt/mgt_param.h
index 58b2dd6..bbb1b1c 100644
--- a/bin/varnishd/mgt/mgt_param.h
+++ b/bin/varnishd/mgt/mgt_param.h
@@ -30,7 +30,8 @@
struct parspec;
-typedef int tweak_t(struct vsb *, const struct parspec *, const char *arg);
+typedef int tweak_t(struct vsb *, const struct parspec *, const char *arg);
+typedef void cb_t(struct vsb *, const struct parspec *);
struct parspec {
const char *name;
@@ -50,6 +51,7 @@ struct parspec {
#define ONLY_ROOT (1<<7)
const char *def;
const char *units;
+ cb_t *cb;
};
tweak_t tweak_bool;
@@ -64,6 +66,8 @@ tweak_t tweak_waiter;
tweak_t tweak_vsl_buffer;
tweak_t tweak_vsl_reclen;
+cb_t update_workspace_backend;
+
int tweak_generic_uint(struct vsb *vsb, volatile unsigned *dest,
const char *arg, const char *min, const char *max);
diff --git a/bin/varnishd/mgt/mgt_param_tbl.c b/bin/varnishd/mgt/mgt_param_tbl.c
index 59fcfed..029a9bd 100644
--- a/bin/varnishd/mgt/mgt_param_tbl.c
+++ b/bin/varnishd/mgt/mgt_param_tbl.c
@@ -43,8 +43,8 @@
"\tmax_age\tmax age of free element."
struct parspec mgt_parspec[] = {
-#define PARAM(nm, ty, mi, ma, de, un, fl, st, lt, fn) \
- { #nm, tweak_##ty, &mgt_param.nm, mi, ma, st, fl, de, un },
+#define PARAM(nm, ty, mi, ma, de, un, fl, st, lt, cb) \
+ { #nm, tweak_##ty, &mgt_param.nm, mi, ma, st, fl, de, un, cb },
#include "tbl/params.h"
#undef PARAM
diff --git a/bin/varnishd/mgt/mgt_param_tweak.c b/bin/varnishd/mgt/mgt_param_tweak.c
index 00ad35c..3dc98dc 100644
--- a/bin/varnishd/mgt/mgt_param_tweak.c
+++ b/bin/varnishd/mgt/mgt_param_tweak.c
@@ -430,3 +430,28 @@ tweak_poolparam(struct vsb *vsb, const struct parspec *par, const char *arg)
}
return (retval);
}
+
+/*
+ * --------------------------------------------------------------------
+ * for parameters affecting workspace_backend
+ */
+
+// XXX HACK
+size_t VBO_Sz(void);
+unsigned HTTP_estimate(unsigned nhttp);
+
+void
+update_workspace_backend(struct vsb *vsb, const struct parspec *par) {
+ const size_t align = 4 * 1024 - 1;
+
+ size_t s = PRNDUP(VBO_Sz()) +
+ 3 * PRNDUP(HTTP_estimate((uint16_t)mgt_param.http_max_hdr)) +
+ PRNDUP(mgt_param.vsl_buffer) +
+ PRNDUP(mgt_param.http_resp_size) +
+ PRNDUP(mgt_param.headroom_backend);
+
+ (void) vsb;
+ (void) par;
+
+ mgt_param.workspace_backend = (s + align) & ~(align);
+}
diff --git a/bin/varnishtest/tests/r01038.vtc b/bin/varnishtest/tests/r01038.vtc
index 705e11c..89a3107 100644
--- a/bin/varnishtest/tests/r01038.vtc
+++ b/bin/varnishtest/tests/r01038.vtc
@@ -45,7 +45,7 @@ server s1 {
txresp -body "foo8"
} -start
-varnish v1 -arg "-p workspace_backend=10k" -vcl+backend {
+varnish v1 -arg "-p headroom_backend=2k" -vcl+backend {
sub vcl_backend_response {
set beresp.do_esi = true;
}
diff --git a/bin/varnishtest/tests/r01120.vtc b/bin/varnishtest/tests/r01120.vtc
index c55064a..a7bb71f 100644
--- a/bin/varnishtest/tests/r01120.vtc
+++ b/bin/varnishtest/tests/r01120.vtc
@@ -9,7 +9,7 @@ server s1 {
varnish v1 \
-cliok "param.set workspace_client 10k" \
- -cliok "param.set workspace_backend 200k" \
+ -cliok "param.set headroom_backend 192k" \
-vcl+backend {
} -start
diff --git a/doc/sphinx/reference/vmod.rst b/doc/sphinx/reference/vmod.rst
index 40b7bf7..e2594d2 100644
--- a/doc/sphinx/reference/vmod.rst
+++ b/doc/sphinx/reference/vmod.rst
@@ -253,7 +253,7 @@ STRING_LIST
a function, we may relax that at a latter time.
If you don't want to bother with STRING_LIST, just use STRING
- and make sure your workspace_client and workspace_backend params
+ and make sure your ``workspace_client`` and ``headroom_backend`` params
are big enough.
TIME
diff --git a/include/tbl/params.h b/include/tbl/params.h
index 402497f..2377533 100644
--- a/include/tbl/params.h
+++ b/include/tbl/params.h
@@ -542,7 +542,7 @@ PARAM(
"Cheap, ~20 bytes, in terms of workspace memory.\n"
"Note that the first line occupies five header lines.",
/* l-text */ "",
- /* func */ NULL
+ /* func */ update_workspace_backend
)
PARAM(
@@ -624,7 +624,7 @@ PARAM(
"(param: workspace_backend) and this parameter limits how much "
"of that the request is allowed to take up.",
/* l-text */ "",
- /* func */ NULL
+ /* func */ update_workspace_backend
)
PARAM(
@@ -1348,7 +1348,7 @@ PARAM(
"mutex.\n\n"
"The minimum tracks the vsl_reclen parameter + 12 bytes.",
/* l-text */ "",
- /* func */ NULL
+ /* func */ update_workspace_backend
)
#if 0
@@ -1436,16 +1436,34 @@ PARAM(
#endif
PARAM(
+ /* name */ headroom_backend,
+ /* typ */ bytes_u,
+ /* min */ "1k",
+ /* max */ NULL,
+ /* default */ "56k",
+ /* units */ "bytes",
+ /* flags */ DELAYED_EFFECT,
+ /* s-text */
+ "Minimum space available on the backend workspace for use in VCL and "
+ "by fetch processors.\n"
+ "When set too small, \"out of workspace (Bo)\" errors will get logged. "
+ "Besides tuning this parameter, workspace consumption in "
+ "vcl_backend_* should be reviewed.",
+ /* l-text */ "",
+ /* func */ update_workspace_backend
+)
+
+PARAM(
/* name */ workspace_backend,
/* typ */ bytes_u,
/* min */ "1k",
/* max */ NULL,
/* default */ "64k",
/* units */ "bytes",
- /* flags */ DELAYED_EFFECT,
+ /* flags */ DELAYED_EFFECT|PROTECTED,
/* s-text */
- "Bytes of HTTP protocol workspace for backend HTTP req/resp. If "
- "larger than 4k, use a multiple of 4k for VM efficiency.",
+ "Do not tune directly - gets set indirectly thorugh "
+ "headroom_backend, http_max_hdr, vsl_buffer, http_resp_size",
/* l-text */ "",
/* func */ NULL
)
_______________________________________________
varnish-dev mailing list
[email protected]
https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev
