[master] b5443cc8a Make VXID's 64 bit and VRT_INTEGER limited.
Dridi Boukelmoune
dridi at varni.sh
Mon Jan 23 13:32:24 UTC 2023
On Mon, Jan 23, 2023 at 11:45 AM Poul-Henning Kamp <phk at freebsd.org> wrote:
>
>
> commit b5443cc8a2aa1550ca3e36766ce2d2ceba8a42a7
> Author: Poul-Henning Kamp <phk at FreeBSD.org>
> Date: Mon Jan 23 11:44:28 2023 +0000
>
> Make VXID's 64 bit and VRT_INTEGER limited.
>
> diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h
> index a0e141a48..55a86960a 100644
> --- a/bin/varnishd/cache/cache.h
> +++ b/bin/varnishd/cache/cache.h
> @@ -65,8 +65,8 @@ typedef struct vxids vxid_t;
>
> #define NO_VXID ((struct vxids){0})
> #define IS_NO_VXID(x) ((x).vxid == 0)
> -#define VXID_TAG(x) ((x).vxid & (VSL_CLIENTMARKER|VSL_BACKENDMARKER))
> -#define VXID(u) ((uintmax_t)(u.vxid) & VSL_IDENTMASK)
> +#define VXID_TAG(x) ((uintmax_t)((x).vxid & (VSL_CLIENTMARKER|VSL_BACKENDMARKER)))
> +#define VXID(u) ((uintmax_t)((u.vxid) & VSL_IDENTMASK))
> #define IS_SAME_VXID(x, y) ((x).vxid == (y).vxid)
>
> /*--------------------------------------------------------------------*/
> @@ -705,10 +705,6 @@ extern const char H__Reason[];
> // rfc7233,l,1207,1208
> #define http_range_at(str, tok, l) http_tok_at(str, #tok, l)
>
> -/* cache_main.c */
> -vxid_t VXID_Get(const struct worker *, uint32_t marker);
> -extern pthread_key_t witness_key;
> -
> /* cache_lck.c */
>
> /* Internal functions, call only through macros below */
> diff --git a/bin/varnishd/cache/cache_main.c b/bin/varnishd/cache/cache_main.c
> index ae414bc01..370b8c741 100644
> --- a/bin/varnishd/cache/cache_main.c
> +++ b/bin/varnishd/cache/cache_main.c
> @@ -184,7 +184,7 @@ static uint32_t vxid_chunk = 32768;
> static struct lock vxid_lock;
>
> vxid_t
> -VXID_Get(const struct worker *wrk, uint32_t mask)
> +VXID_Get(const struct worker *wrk, uint64_t mask)
> {
> struct vxid_pool *v;
> vxid_t retval;
> @@ -193,12 +193,12 @@ VXID_Get(const struct worker *wrk, uint32_t mask)
> CHECK_OBJ_NOTNULL(wrk->wpriv, WORKER_PRIV_MAGIC);
> v = wrk->wpriv->vxid_pool;
> AZ(mask & VSL_IDENTMASK);
> - while (v->count == 0 || v->next >= VSL_CLIENTMARKER) {
> + while (v->count == 0 || v->next >= VRT_INTEGER_MAX) {
> Lck_Lock(&vxid_lock);
> v->next = vxid_base;
> v->count = vxid_chunk;
> vxid_base += v->count;
> - if (vxid_base >= VSL_CLIENTMARKER)
> + if (vxid_base >= VRT_INTEGER_MAX)
> vxid_base = 1;
> Lck_Unlock(&vxid_lock);
> }
> diff --git a/bin/varnishd/cache/cache_shmlog.c b/bin/varnishd/cache/cache_shmlog.c
> index 7a217d6f8..b3b2e213d 100644
> --- a/bin/varnishd/cache/cache_shmlog.c
> +++ b/bin/varnishd/cache/cache_shmlog.c
> @@ -143,7 +143,7 @@ vsl_hdr(enum VSL_tag_e tag, uint32_t *p, unsigned len, vxid_t vxid)
> assert(tag < SLT__Reserved);
> AZ(len & ~VSL_LENMASK);
>
> - p[2] = 0;
> + p[2] = vxid.vxid >> 32;
> p[1] = vxid.vxid;
> p[0] = (((unsigned)tag & VSL_IDMASK) << VSL_IDSHIFT) |
> (VSL_VERSION_3 << VSL_VERSHIFT) |
> diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h
> index 20077e182..e6dc7e697 100644
> --- a/bin/varnishd/cache/cache_varnishd.h
> +++ b/bin/varnishd/cache/cache_varnishd.h
> @@ -302,6 +302,9 @@ uint16_t HTTP1_DissectResponse(struct http_conn *, struct http *resp,
> unsigned HTTP1_Write(const struct worker *w, const struct http *hp, const int*);
>
> /* cache_main.c */
> +vxid_t VXID_Get(const struct worker *, uint64_t marker);
> +extern pthread_key_t witness_key;
> +
> void THR_SetName(const char *name);
> const char* THR_GetName(void);
> void THR_SetBusyobj(const struct busyobj *);
> diff --git a/bin/varnishtest/tests/c00122.vtc b/bin/varnishtest/tests/c00122.vtc
> new file mode 100644
> index 000000000..d624df224
> --- /dev/null
> +++ b/bin/varnishtest/tests/c00122.vtc
> @@ -0,0 +1,107 @@
> +varnishtest "test 64 bit vxid rollover"
> +
> +server s1 {
> + rxreq
> + txresp
> +
> + accept
> + rxreq
> + delay 5
> + txresp
> +} -start
> +
> +varnish v1 -vcl+backend {
> + sub vcl_backend_response {
> + if (bereq.url == "/retry" &&
> + bereq.retries == 0) {
> + return (retry);
> + }
> + }
> + sub vcl_deliver {
> + if (req.url == "/restart" &&
> + req.restarts == 0) {
> + return (restart);
> + }
> + }
> +
> +} -start
> +
> +process p1 -winsz 100 200 {exec varnishlog -n ${v1_name} -g raw} -start
> +process p1 -expect-text 0 0 CLI
> +
> +varnish v1 -cliok "param.set debug +syncvsl"
> +varnish v1 -cliok "debug.xid 999999999999998"
> +
> +logexpect l1 -v v1 -g request -T 2 {
> + expect 0 1 Begin "req 999999999999998"
> + expect * = ReqStart
> + expect 0 = ReqMethod GET
> + expect 0 = ReqURL /
> + expect 0 = ReqProtocol HTTP/1.1
> + expect * = ReqHeader "Foo: bar"
> + expect * = Link "bereq 2 fetch"
> + expect * = VSL "timeout"
> + expect * = End "synth"
> +
> + expect 0 2 Begin "bereq 1"
> + expect * 2 Link "bereq 3 retry"
> + expect * = End
> +
> + expect 0 3 Begin "bereq 2 retry"
> + expect * = End
> +} -start
> +
> +client c1 {
> + txreq -url "/retry" -hdr "Foo: bar"
> + rxresp
> + expect resp.status == 200
> +} -run
> +
> +varnish v1 -vsl_catchup
> +
> +logexpect l1 -wait
> +process p1 -expect-text 0 1 "999999999999998 SessClose"
> +process p1 -screen_dump
> +
> +
> +################################################################################
> +
> +server s1 {
> + rxreq
> + txresp
> +} -start
> +
> +varnish v1 -cliok "param.set debug +syncvsl"
> +varnish v1 -cliok "debug.xid 999999999999997"
> +
> +logexpect l1 -v v1 -g request {
> + expect 0 999999999999998 Begin "req 999999999999997"
> + expect * = ReqStart
> + expect 0 = ReqMethod GET
> + expect 0 = ReqURL /
> + expect 0 = ReqProtocol HTTP/1.1
> + expect * = ReqHeader "Foo: bar"
> + expect * = Link "bereq 1 fetch"
> + expect * = Link "req 2 restart"
> + expect * = End
> +
> + expect 0 1 Begin "bereq 999999999999998"
> + expect * = End
> +
> + expect 0 2 Begin "req 999999999999998 restart"
> + expect * = End
> +} -start
> +
> +client c1 {
> + txreq -url "/restart" -hdr "Foo: bar"
> + rxresp
> + expect resp.status == 200
> +} -run
> +
> +varnish v1 -vsl_catchup
> +logexpect l1 -wait
> +
> +process p1 -expect-text 0 1 "999999999999998 Link c req 2 restart"
> +process p1 -expect-text 0 1 "999999999999997 SessClose"
> +process p1 -screen_dump
> +process p1 -stop
> diff --git a/bin/varnishtest/tests/r01762.vtc b/bin/varnishtest/tests/r01762.vtc
> index 5c5b39f2b..d0f96d9bb 100644
> --- a/bin/varnishtest/tests/r01762.vtc
> +++ b/bin/varnishtest/tests/r01762.vtc
> @@ -1,5 +1,7 @@
> varnishtest "test vsl api handling of incomplete vtxes combined with bad vxids"
>
> +feature cmd false
> +
> server s1 {
> rxreq
> txresp
> diff --git a/bin/varnishtest/vtc_logexp.c b/bin/varnishtest/vtc_logexp.c
> index 88cb5049f..7eede8a7e 100644
> --- a/bin/varnishtest/vtc_logexp.c
> +++ b/bin/varnishtest/vtc_logexp.c
> @@ -661,7 +661,7 @@ cmd_logexp_common(struct logexp *le, struct vtclog *vl,
> else if (!strcmp(av[2], "="))
> vxid = LE_LAST;
> else {
> - vxid = (int)strtoll(av[2], &end, 10);
> + vxid = strtoll(av[2], &end, 10);
> if (*end != '\0' || vxid < 0)
> vtc_fatal(vl, "Not a positive integer: '%s'", av[2]);
> }
> diff --git a/doc/changes.rst b/doc/changes.rst
> index 3e47502bf..8d16a3988 100644
> --- a/doc/changes.rst
> +++ b/doc/changes.rst
> @@ -35,6 +35,24 @@ release process.
> Varnish Cache NEXT (2023-03-15)
> ===============================
>
> +* VXIDs are 64 bit now and the binary format of SHM and raw saved
> + VSL files has changed as a consequence.
> +
> + The actual valid range for VXIDs is [1…999999999999999], so it
> + fits in a VRT_INTEGER.
> +
> + At one million cache-missing single request sessions per second
> + VXIDs will roll over in a little over ten years::
> +
> + (1e15-1) / (3 * 1e6 * 86400 * 365) = 10.57
> +
> + That should be enough for everybody™.
> +
> + You can test if your downstream log-chewing pipeline handle the
> + larger VXIDs correctly using the CLI command::
> +
> + ``debug.xid 20000000000``
> +
> * The ``debug.xid`` CLI command now sets the next XID to be used,
> rather than "one less than the next XID to be used"
>
> diff --git a/include/vapi/vsl_int.h b/include/vapi/vsl_int.h
> index faeb9d111..a40158e04 100644
> --- a/include/vapi/vsl_int.h
> +++ b/include/vapi/vsl_int.h
> @@ -62,9 +62,9 @@
> * changing corresponding magic numbers in varnishd/cache/cache_shmlog.c
> */
>
> -#define VSL_CLIENTMARKER (1U<<30)
> -#define VSL_BACKENDMARKER (1U<<31)
> -#define VSL_IDENTMASK (~(3U<<30))
> +#define VSL_CLIENTMARKER (1ULL<<62)
> +#define VSL_BACKENDMARKER (1ULL<<63)
> +#define VSL_IDENTMASK ((1ULL<<51)-1)
Should we reserve 4 bits to track the ESI level of client requests?
That would set an upper limit of 15 for the max_esi_depth, which is
probably reasonable. If not, we still have a few more bits that could
then be (ab)used for faster VSL query processing. Please note that I'm
very well aware that the current "level" syntax for vslq has nothing
to do with ESI, so I was also implicitly suggesting we could extend
VSL filtering and/or to include esi_level.
I understand it is hard to decide on an arbitrary maximum for
parameters like max_esi_depth, but I'm willing to make that kind of
trade off.
Dridi
>
> #define VSL_LENMASK 0xffff
> #define VSL_VERMASK 0x3
> @@ -81,9 +81,10 @@
> #define VSL_LEN(ptr) ((ptr)[0] & VSL_LENMASK)
> #define VSL_VER(ptr) (((ptr)[0] & VSL_VERMASK) >> VSL_VERSHIFT)
> #define VSL_TAG(ptr) ((enum VSL_tag_e)((ptr)[0] >> VSL_IDSHIFT))
> -#define VSL_ID(ptr) (((ptr)[1]) & VSL_IDENTMASK)
> -#define VSL_CLIENT(ptr) (((ptr)[1]) & VSL_CLIENTMARKER)
> -#define VSL_BACKEND(ptr) (((ptr)[1]) & VSL_BACKENDMARKER)
> +#define VSL_ID64(ptr) (((uint64_t)((ptr)[2])<<32) | ((ptr)[1]))
> +#define VSL_ID(ptr) (VSL_ID64(ptr) & VSL_IDENTMASK)
> +#define VSL_CLIENT(ptr) (((ptr)[2]) & (VSL_CLIENTMARKER >> 32))
> +#define VSL_BACKEND(ptr) (((ptr)[2]) & (VSL_BACKENDMARKER >> 32))
> #define VSL_DATA(ptr) ((char*)((ptr)+VSL_OVERHEAD))
> #define VSL_CDATA(ptr) ((const char*)((ptr)+VSL_OVERHEAD))
> #define VSL_BATCHLEN(ptr) ((ptr)[1])
> diff --git a/include/vrt.h b/include/vrt.h
> index e65e59737..e9ca4f087 100644
> --- a/include/vrt.h
> +++ b/include/vrt.h
> @@ -58,6 +58,7 @@
> * binary/load-time compatible, increment MAJOR version
> *
> * NEXT (2023-03-15)
> + * VXID is 64 bit
> * [cache.h] http_GetRange() changed
> * 16.0 (2022-09-15)
> * VMOD C-prototypes moved into JSON
> diff --git a/lib/libvarnishapi/vsl_dispatch.c b/lib/libvarnishapi/vsl_dispatch.c
> index 34b8af199..3e990fb36 100644
> --- a/lib/libvarnishapi/vsl_dispatch.c
> +++ b/lib/libvarnishapi/vsl_dispatch.c
> @@ -1033,6 +1033,7 @@ vtx_synth_rec(struct vtx *vtx, unsigned tag, const char *fmt, ...)
> va_list ap;
> char *buf;
> int l, buflen;
> + uint64_t vxid;
>
> ALLOC_OBJ(synth, SYNTH_MAGIC);
> AN(synth);
> @@ -1046,18 +1047,21 @@ vtx_synth_rec(struct vtx *vtx, unsigned tag, const char *fmt, ...)
> if (l > buflen - 1)
> l = buflen - 1;
> buf[l++] = '\0'; /* NUL-terminated */
> - synth->data[1] = vtx->key.vxid;
> + vxid = vtx->key.vxid;
> switch (vtx->type) {
> case VSL_t_req:
> - synth->data[1] |= VSL_CLIENTMARKER;
> + vxid |= VSL_CLIENTMARKER;
> break;
> case VSL_t_bereq:
> - synth->data[1] |= VSL_BACKENDMARKER;
> + vxid |= VSL_BACKENDMARKER;
> break;
> default:
> break;
> }
> - synth->data[0] = (((tag & 0xff) << 24) | l);
> + synth->data[2] = vxid >> 32;
> + synth->data[1] = vxid;
> + synth->data[0] = (((tag & VSL_IDMASK) << VSL_IDSHIFT) |
> + (VSL_VERSION_3 << VSL_VERSHIFT) | l);
> synth->offset = vtx->c.offset;
>
> VTAILQ_FOREACH_REVERSE(it, &vtx->synth, synthhead, list) {
> diff --git a/vmod/vmod_vtc.c b/vmod/vmod_vtc.c
> index aa1c2b9ad..6c38db3ae 100644
> --- a/vmod/vmod_vtc.c
> +++ b/vmod/vmod_vtc.c
> @@ -422,7 +422,7 @@ vmod_vsl(VRT_CTX, VCL_INT id, VCL_STRING tag_s, VCL_ENUM side, VCL_STRANDS s)
> return;
> }
>
> - if (id < 0 || id > VSL_IDENTMASK) {
> + if (id < 0 || id > VRT_INTEGER_MAX) {
> VRT_fail(ctx, "id out of bounds");
> return;
> }
> _______________________________________________
> varnish-commit mailing list
> varnish-commit at varnish-cache.org
> https://www.varnish-cache.org/lists/mailman/listinfo/varnish-commit
More information about the varnish-commit
mailing list