From c86677da77ed6464c74e1cfabede5d6e52dcd5fd Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Mon, 22 Jan 2024 15:55:55 +0100 Subject: [PATCH 01/10] hash: Make HSH_Fail() rush the waiting list This is not a nice caching outcome, but it is a good reason to trigger a new fetch if there are clients on the waiting list. If the fetch task failed before reaching HSH_Unbusy() we make sure to "unbusy" it anyway. The OC_F_FAILED flag will prevent it from being looked up, so there is no reason to delay the rush until the final objcore reference is dropped. This also means that once the client task that triggered the fetch task resumes it can only be operating on a non-busy object. --- bin/varnishd/cache/cache_fetch.c | 13 +++++-------- bin/varnishd/cache/cache_hash.c | 10 +++++++++- bin/varnishd/cache/cache_objhead.h | 2 +- 3 files changed, 15 insertions(+), 10 deletions(-) diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c index f49de59cf71..e541da60b52 100644 --- a/bin/varnishd/cache/cache_fetch.c +++ b/bin/varnishd/cache/cache_fetch.c @@ -1047,9 +1047,8 @@ vbf_stp_fail(struct worker *wrk, struct busyobj *bo) CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); assert(oc->boc->state < BOS_FINISHED); - HSH_Fail(oc); - if (!(oc->flags & OC_F_BUSY)) - HSH_Kill(oc); + HSH_Fail(wrk, oc); + HSH_Kill(oc); ObjSetState(wrk, oc, BOS_FAILED); return (F_STP_DONE); } @@ -1234,11 +1233,9 @@ VBF_Fetch(struct worker *wrk, struct req *req, struct objcore *oc, (void)VRB_Ignore(req); } else { ObjWaitState(oc, BOS_STREAM); - if (oc->boc->state == BOS_FAILED) { - AN((oc->flags & OC_F_FAILED)); - } else { - AZ(oc->flags & OC_F_BUSY); - } + AZ(oc->flags & OC_F_BUSY); + if (oc->boc->state == BOS_FAILED) + AN(oc->flags & OC_F_FAILED); } } AZ(bo); diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index ce31aecf09f..6876a95d88e 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -780,13 +780,16 @@ HSH_Purge(struct worker *wrk, struct objhead *oh, vtim_real ttl_now, */ void -HSH_Fail(struct objcore *oc) +HSH_Fail(struct worker *wrk, struct objcore *oc) { struct objhead *oh; + struct rush rush; + CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); oh = oc->objhead; CHECK_OBJ(oh, OBJHEAD_MAGIC); + INIT_OBJ(&rush, RUSH_MAGIC); /* * We have to have either a busy bit, so that HSH_Lookup @@ -797,7 +800,12 @@ HSH_Fail(struct objcore *oc) Lck_Lock(&oh->mtx); oc->flags |= OC_F_FAILED; + if (oc->flags & OC_F_BUSY) { + oc->flags &= ~OC_F_BUSY; + hsh_rush1(wrk, oh, &rush, 1); + } Lck_Unlock(&oh->mtx); + hsh_rush2(wrk, &rush); } /*--------------------------------------------------------------------- diff --git a/bin/varnishd/cache/cache_objhead.h b/bin/varnishd/cache/cache_objhead.h index 2edfb5a883b..866dbc4a22a 100644 --- a/bin/varnishd/cache/cache_objhead.h +++ b/bin/varnishd/cache/cache_objhead.h @@ -65,11 +65,11 @@ enum lookup_e { HSH_BUSY, }; -void HSH_Fail(struct objcore *); void HSH_Kill(struct objcore *); void HSH_Replace(struct objcore *, const struct objcore *); void HSH_Insert(struct worker *, const void *hash, struct objcore *, struct ban *); +void HSH_Fail(struct worker *, struct objcore *); void HSH_Unbusy(struct worker *, struct objcore *); int HSH_Snipe(const struct worker *, struct objcore *); struct boc *HSH_RefBoc(const struct objcore *); From ca2f111bdc8880b9a26ba1f8eb6d6dadc72c3518 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Mon, 22 Jan 2024 14:58:27 +0100 Subject: [PATCH 02/10] obj: Rush objcores based state changes The BOS_PREP_STREAM state is retired. It was used as a stop-gap to "unbusy" objects before waking up the client task for good. Instead, the HSH_Unbusy() and HSH_Fail() functions are called once the gap is closed, depending on the outcome. This removes one unnecessary signal and consolidates multiple call sites, ensuring that objects always drop the OC_F_BUSY flag from a central location, for fetch tasks. --- bin/varnishd/cache/cache_fetch.c | 21 +++++---------------- bin/varnishd/cache/cache_obj.c | 10 +++++++--- bin/varnishd/cache/cache_varnishd.h | 3 +-- include/tbl/boc_state.h | 1 - 4 files changed, 13 insertions(+), 22 deletions(-) diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c index e541da60b52..7135ae987e5 100644 --- a/bin/varnishd/cache/cache_fetch.c +++ b/bin/varnishd/cache/cache_fetch.c @@ -705,11 +705,8 @@ vbf_stp_fetch(struct worker *wrk, struct busyobj *bo) assert(oc->boc->state == BOS_REQ_DONE); - if (bo->do_stream) { - ObjSetState(wrk, oc, BOS_PREP_STREAM); - HSH_Unbusy(wrk, oc); + if (bo->do_stream) ObjSetState(wrk, oc, BOS_STREAM); - } VSLb(bo->vsl, SLT_Fetch_Body, "%u %s %s", bo->htc->body_status->nbr, bo->htc->body_status->name, @@ -744,11 +741,8 @@ vbf_stp_fetchend(struct worker *wrk, struct busyobj *bo) if (bo->do_stream) assert(oc->boc->state == BOS_STREAM); - else { + else assert(oc->boc->state == BOS_REQ_DONE); - ObjSetState(wrk, oc, BOS_PREP_STREAM); - HSH_Unbusy(wrk, oc); - } ObjSetState(wrk, oc, BOS_FINISHED); VSLb_ts_busyobj(bo, "BerespBody", W_TIM_real(wrk)); @@ -877,11 +871,8 @@ vbf_stp_condfetch(struct worker *wrk, struct busyobj *bo) ObjSetFlag(bo->wrk, oc, OF_IMSCAND, 0); AZ(ObjCopyAttr(bo->wrk, oc, stale_oc, OA_GZIPBITS)); - if (bo->do_stream) { - ObjSetState(wrk, oc, BOS_PREP_STREAM); - HSH_Unbusy(wrk, oc); + if (bo->do_stream) ObjSetState(wrk, oc, BOS_STREAM); - } INIT_OBJ(vop, VBF_OBITER_PRIV_MAGIC); vop->bo = bo; @@ -1025,8 +1016,7 @@ vbf_stp_error(struct worker *wrk, struct busyobj *bo) } AZ(ObjSetU64(wrk, oc, OA_LEN, o)); VSB_destroy(&synth_body); - ObjSetState(wrk, oc, BOS_PREP_STREAM); - HSH_Unbusy(wrk, oc); + ObjSetState(wrk, oc, BOS_STREAM); if (stale != NULL && oc->ttl > 0) HSH_Kill(stale); ObjSetState(wrk, oc, BOS_FINISHED); @@ -1047,9 +1037,8 @@ vbf_stp_fail(struct worker *wrk, struct busyobj *bo) CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); assert(oc->boc->state < BOS_FINISHED); - HSH_Fail(wrk, oc); - HSH_Kill(oc); ObjSetState(wrk, oc, BOS_FAILED); + HSH_Kill(oc); return (F_STP_DONE); } diff --git a/bin/varnishd/cache/cache_obj.c b/bin/varnishd/cache/cache_obj.c index ec819bf8cb7..19f30b1d139 100644 --- a/bin/varnishd/cache/cache_obj.c +++ b/bin/varnishd/cache/cache_obj.c @@ -86,6 +86,7 @@ #include "cache_varnishd.h" #include "cache_obj.h" +#include "cache_objhead.h" #include "vend.h" #include "storage/storage.h" @@ -295,8 +296,7 @@ ObjWaitExtend(const struct worker *wrk, const struct objcore *oc, uint64_t l) */ void -ObjSetState(struct worker *wrk, const struct objcore *oc, - enum boc_state_e next) +ObjSetState(struct worker *wrk, struct objcore *oc, enum boc_state_e next) { const struct obj_methods *om; @@ -305,7 +305,6 @@ ObjSetState(struct worker *wrk, const struct objcore *oc, assert(next > oc->boc->state); CHECK_OBJ_ORNULL(oc->stobj->stevedore, STEVEDORE_MAGIC); - assert(next != BOS_STREAM || oc->boc->state == BOS_PREP_STREAM); assert(next != BOS_FINISHED || (oc->oa_present & (1 << OA_LEN))); if (oc->stobj->stevedore != NULL) { @@ -314,6 +313,11 @@ ObjSetState(struct worker *wrk, const struct objcore *oc, om->objsetstate(wrk, oc, next); } + if (next == BOS_FAILED) + HSH_Fail(wrk, oc); + else if (oc->boc->state < BOS_STREAM && next >= BOS_STREAM) + HSH_Unbusy(wrk, oc); + Lck_Lock(&oc->boc->mtx); oc->boc->state = next; PTOK(pthread_cond_broadcast(&oc->boc->cond)); diff --git a/bin/varnishd/cache/cache_varnishd.h b/bin/varnishd/cache/cache_varnishd.h index a73ff71708d..aec38824fc8 100644 --- a/bin/varnishd/cache/cache_varnishd.h +++ b/bin/varnishd/cache/cache_varnishd.h @@ -336,8 +336,7 @@ int ObjGetSpace(struct worker *, struct objcore *, ssize_t *sz, uint8_t **ptr); void ObjExtend(struct worker *, struct objcore *, ssize_t l, int final); uint64_t ObjWaitExtend(const struct worker *, const struct objcore *, uint64_t l); -void ObjSetState(struct worker *, const struct objcore *, - enum boc_state_e next); +void ObjSetState(struct worker *, struct objcore *, enum boc_state_e next); void ObjWaitState(const struct objcore *, enum boc_state_e want); void ObjTouch(struct worker *, struct objcore *, vtim_real now); void ObjFreeObj(struct worker *, struct objcore *); diff --git a/include/tbl/boc_state.h b/include/tbl/boc_state.h index 44984de81b9..2e041366068 100644 --- a/include/tbl/boc_state.h +++ b/include/tbl/boc_state.h @@ -32,7 +32,6 @@ BOC_STATE(INVALID, invalid) /* don't touch (yet) */ BOC_STATE(REQ_DONE, req_done) /* bereq.* can be examined */ -BOC_STATE(PREP_STREAM, prep_stream) /* Prepare for streaming */ BOC_STATE(STREAM, stream) /* beresp.* can be examined */ BOC_STATE(FINISHED, finished) /* object is complete */ BOC_STATE(FAILED, failed) /* something went wrong */ From 54dfb38356e43fb592ebde72f4804a3703e14e62 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 19 Dec 2023 15:18:02 +0100 Subject: [PATCH 03/10] hash: Trigger a rush when an objcore is withdrawn This is the counterpart of HSH_Unbusy() for the cases where the req task will not schedule a fetch task, at which point we know we can already wake up a request, if there is one on the waiting list. This encapsulates the "wake up only one request" semantic using a new OC_F_WITHDRAWN flag. Although this flag is not being used yet, it will when the state of the objcore determines the rush policy instead of the call site. --- bin/varnishd/cache/cache_hash.c | 49 +++++++++++++++++++++++++++++- bin/varnishd/cache/cache_objhead.h | 2 ++ bin/varnishd/cache/cache_req_fsm.c | 6 ++-- include/tbl/oc_flags.h | 3 +- 4 files changed, 55 insertions(+), 5 deletions(-) diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index 6876a95d88e..4fdbae2ef31 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -863,6 +863,36 @@ HSH_Cancel(struct worker *wrk, struct objcore *oc, struct boc *boc) ObjSlim(wrk, oc); } +/*--------------------------------------------------------------------- + * Withdraw an objcore that will not proceed with a fetch. + */ + +void +HSH_Withdraw(struct worker *wrk, struct objcore **ocp) +{ + struct objhead *oh; + struct objcore *oc; + struct rush rush; + + CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); + TAKE_OBJ_NOTNULL(oc, ocp, OBJCORE_MAGIC); + INIT_OBJ(&rush, RUSH_MAGIC); + + oh = oc->objhead; + CHECK_OBJ(oh, OBJHEAD_MAGIC); + + Lck_Lock(&oh->mtx); + AZ(oc->stobj->stevedore); + assert(oc->flags == OC_F_BUSY); + assert(oc->refcnt == 1); + assert(oh->refcnt > 0); + oc->flags = OC_F_WITHDRAWN; + hsh_rush1(wrk, oh, &rush, 1); + AZ(HSH_DerefObjCoreUnlock(wrk, &oc, 0)); + + hsh_rush2(wrk, &rush); +} + /*--------------------------------------------------------------------- * Unbusy an objcore when the object is completely fetched. */ @@ -1047,6 +1077,23 @@ HSH_DerefBoc(struct worker *wrk, struct objcore *oc) int HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp, int rushmax) +{ + struct objcore *oc; + struct objhead *oh; + + CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); + TAKE_OBJ_NOTNULL(oc, ocp, OBJCORE_MAGIC); + assert(oc->refcnt > 0); + + oh = oc->objhead; + CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); + + Lck_Lock(&oh->mtx); + return (HSH_DerefObjCoreUnlock(wrk, &oc, rushmax)); +} + +int +HSH_DerefObjCoreUnlock(struct worker *wrk, struct objcore **ocp, int rushmax) { struct objcore *oc; struct objhead *oh; @@ -1061,7 +1108,7 @@ HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp, int rushmax) oh = oc->objhead; CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); - Lck_Lock(&oh->mtx); + Lck_AssertHeld(&oh->mtx); assert(oh->refcnt > 0); r = --oc->refcnt; if (!r) diff --git a/bin/varnishd/cache/cache_objhead.h b/bin/varnishd/cache/cache_objhead.h index 866dbc4a22a..8d598d7b149 100644 --- a/bin/varnishd/cache/cache_objhead.h +++ b/bin/varnishd/cache/cache_objhead.h @@ -69,6 +69,7 @@ void HSH_Kill(struct objcore *); void HSH_Replace(struct objcore *, const struct objcore *); void HSH_Insert(struct worker *, const void *hash, struct objcore *, struct ban *); +void HSH_Withdraw(struct worker *, struct objcore **); void HSH_Fail(struct worker *, struct objcore *); void HSH_Unbusy(struct worker *, struct objcore *); int HSH_Snipe(const struct worker *, struct objcore *); @@ -79,6 +80,7 @@ void HSH_DeleteObjHead(const struct worker *, struct objhead *); int HSH_DerefObjCore(struct worker *, struct objcore **, int rushmax); #define HSH_RUSH_POLICY -1 +int HSH_DerefObjCoreUnlock(struct worker *, struct objcore **, int rushmax); enum lookup_e HSH_Lookup(struct req *, struct objcore **, struct objcore **); void HSH_Ref(struct objcore *o); void HSH_AddString(struct req *, void *ctx, const char *str); diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index ed97a01107a..74e515dc20b 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -667,7 +667,7 @@ cnt_lookup(struct worker *wrk, struct req *req) (void)HSH_DerefObjCore(wrk, &req->objcore, HSH_RUSH_POLICY); if (busy != NULL) { - (void)HSH_DerefObjCore(wrk, &busy, 0); + HSH_Withdraw(wrk, &busy); VRY_Clear(req); } @@ -715,7 +715,7 @@ cnt_miss(struct worker *wrk, struct req *req) VRY_Clear(req); if (req->stale_oc != NULL) (void)HSH_DerefObjCore(wrk, &req->stale_oc, 0); - AZ(HSH_DerefObjCore(wrk, &req->objcore, 1)); + HSH_Withdraw(wrk, &req->objcore); return (REQ_FSM_MORE); } @@ -1085,7 +1085,7 @@ cnt_purge(struct worker *wrk, struct req *req) (void)HSH_Purge(wrk, boc->objhead, req->t_req, 0, 0, 0); - AZ(HSH_DerefObjCore(wrk, &boc, 1)); + HSH_Withdraw(wrk, &boc); VCL_purge_method(req->vcl, wrk, req, NULL, NULL); switch (wrk->vpi->handling) { diff --git a/include/tbl/oc_flags.h b/include/tbl/oc_flags.h index 768962fdaee..0866cc38f02 100644 --- a/include/tbl/oc_flags.h +++ b/include/tbl/oc_flags.h @@ -30,7 +30,8 @@ /*lint -save -e525 -e539 */ -OC_FLAG(BUSY, busy, (1<<1)) //lint !e835 +OC_FLAG(WITHDRAWN, withdrawn, (1<<0)) //lint !e835 +OC_FLAG(BUSY, busy, (1<<1)) OC_FLAG(HFM, hfm, (1<<2)) OC_FLAG(HFP, hfp, (1<<3)) OC_FLAG(CANCEL, cancel, (1<<4)) From 1fc2565c9ad87d3e8b6e2ef0130def0c3093d0f0 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 19 Dec 2023 12:27:32 +0100 Subject: [PATCH 04/10] req_fsm: Clear OC_F_BUSY flag for synthetic objcores Objcores created in vcl_synth cannot be looked up, therefore there is never an object going on the waiting list because it sees a synthetic objcore's OC_F_BUSY flag. --- bin/varnishd/cache/cache_req_fsm.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index 74e515dc20b..fe84f4d7551 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -382,6 +382,7 @@ cnt_synth(struct worker *wrk, struct req *req) req->objcore = HSH_Private(wrk); CHECK_OBJ_NOTNULL(req->objcore, OBJCORE_MAGIC); + req->objcore->flags &= ~OC_F_BUSY; szl = -1; if (STV_NewObject(wrk, req->objcore, stv_transient, 0)) { body = VSB_data(synth_body); From 962dd64b40a757229cf64b77852fcfd5e091ac9a Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 19 Dec 2023 14:48:28 +0100 Subject: [PATCH 05/10] req_body: Clear OC_F_BUSY flag for req->body_oc Objcores created for request bodies cannot be looked up, therefore there is never an object going on the waiting list because it sees a req.body objcore's OC_F_BUSY flag. --- bin/varnishd/cache/cache_req_body.c | 1 + 1 file changed, 1 insertion(+) diff --git a/bin/varnishd/cache/cache_req_body.c b/bin/varnishd/cache/cache_req_body.c index 51d45471ec5..db154dc01f7 100644 --- a/bin/varnishd/cache/cache_req_body.c +++ b/bin/varnishd/cache/cache_req_body.c @@ -69,6 +69,7 @@ vrb_pull(struct req *req, ssize_t maxsize, objiterate_f *func, void *priv) req->body_oc = HSH_Private(req->wrk); AN(req->body_oc); + req->body_oc->flags &= ~OC_F_BUSY; if (req->storage != NULL) stv = req->storage; From d4837e39d42745fc9014623e4a7dd613e4a5811d Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 19 Dec 2023 15:24:14 +0100 Subject: [PATCH 06/10] hash: Dropping an objcore does not rush the waiting list Rushing the waiting list because an objcore was dropped was a source of spurious wakeups, including the case where the dropped objcore is not even relevant to the objhead waiting list. It is now guaranteed that an object either dropped its OC_F_BUSY flag once ready (otherwise failed or withdrawn) or never had this flag to begin with for objcores that can't be looked up. --- bin/varnishd/cache/cache_hash.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index 4fdbae2ef31..cdfdaca6c4f 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -1097,13 +1097,13 @@ HSH_DerefObjCoreUnlock(struct worker *wrk, struct objcore **ocp, int rushmax) { struct objcore *oc; struct objhead *oh; - struct rush rush; int r; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); TAKE_OBJ_NOTNULL(oc, ocp, OBJCORE_MAGIC); assert(oc->refcnt > 0); - INIT_OBJ(&rush, RUSH_MAGIC); + + (void)rushmax; oh = oc->objhead; CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); @@ -1113,15 +1113,11 @@ HSH_DerefObjCoreUnlock(struct worker *wrk, struct objcore **ocp, int rushmax) r = --oc->refcnt; if (!r) VTAILQ_REMOVE(&oh->objcs, oc, hsh_list); - if (!VTAILQ_EMPTY(&oh->waitinglist)) { - assert(oh->refcnt > 1); - hsh_rush1(wrk, oh, &rush, rushmax); - } Lck_Unlock(&oh->mtx); - hsh_rush2(wrk, &rush); if (r != 0) return (r); + AZ(oc->flags & OC_F_BUSY); AZ(oc->exp_flags); BAN_DestroyObj(oc); From a86f11f51613bcdb5439553e7aa09598d7354c47 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Mon, 22 Jan 2024 17:01:56 +0100 Subject: [PATCH 07/10] hash: Operate waiting list rush on objcores Operating on an objcore allows to preserve the different rush strategies that exist so far: - rush one request for failed fetch tasks - rush one request for withdrawn objects - rush rush_exponent requests otherwise For the cases where no rush is expected, a null objcore is passed. For cacheable objects, all waiters are optimistically woken up at once. --- bin/varnishd/cache/cache_hash.c | 57 ++++++++++++++++++++------------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index cdfdaca6c4f..12a0496098f 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -75,12 +75,12 @@ struct rush { static const struct hash_slinger *hash; static struct objhead *private_oh; -static void hsh_rush1(const struct worker *, struct objhead *, - struct rush *, int); +static void hsh_rush1(const struct worker *, struct objcore *, + struct rush *); static void hsh_rush2(struct worker *, struct rush *); static int hsh_deref_objhead(struct worker *wrk, struct objhead **poh); static int hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh, - int); + struct objcore *oc); /*---------------------------------------------------------------------*/ @@ -324,7 +324,7 @@ HSH_Insert(struct worker *wrk, const void *digest, struct objcore *oc, VTAILQ_INSERT_HEAD(&oh->objcs, oc, hsh_list); oc->flags &= ~OC_F_BUSY; if (!VTAILQ_EMPTY(&oh->waitinglist)) - hsh_rush1(wrk, oh, &rush, HSH_RUSH_POLICY); + hsh_rush1(wrk, oc, &rush); Lck_Unlock(&oh->mtx); hsh_rush2(wrk, &rush); @@ -501,7 +501,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) if (oc != NULL && oc->flags & OC_F_HFP) { xid = VXID(ObjGetXID(wrk, oc)); dttl = EXP_Dttl(req, oc); - AN(hsh_deref_objhead_unlock(wrk, &oh, HSH_RUSH_POLICY)); + AN(hsh_deref_objhead_unlock(wrk, &oh, oc)); wrk->stats->cache_hitpass++; VSLb(req->vsl, SLT_HitPass, "%u %.6f", xid, dttl); return (HSH_HITPASS); @@ -521,7 +521,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) } oc->hits++; boc_progress = oc->boc == NULL ? -1 : oc->boc->fetched_so_far; - AN(hsh_deref_objhead_unlock(wrk, &oh, HSH_RUSH_POLICY)); + AN(hsh_deref_objhead_unlock(wrk, &oh, oc)); Req_LogHit(wrk, req, oc, boc_progress); return (HSH_HIT); } @@ -570,7 +570,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) exp_oc->refcnt++; *ocp = exp_oc; exp_oc->hits++; - AN(hsh_deref_objhead_unlock(wrk, &oh, 0)); + AN(hsh_deref_objhead_unlock(wrk, &oh, NULL)); Req_LogHit(wrk, req, exp_oc, boc_progress); return (HSH_GRACE); } @@ -603,22 +603,34 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) */ static void -hsh_rush1(const struct worker *wrk, struct objhead *oh, struct rush *r, int max) +hsh_rush1(const struct worker *wrk, struct objcore *oc, struct rush *r) { - int i; + struct objhead *oh; struct req *req; - - if (max == 0) - return; - if (max == HSH_RUSH_POLICY) - max = cache_param->rush_exponent; - assert(max > 0); + int i, max; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); - CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); + CHECK_OBJ_ORNULL(oc, OBJCORE_MAGIC); CHECK_OBJ_NOTNULL(r, RUSH_MAGIC); VTAILQ_INIT(&r->reqs); + + if (oc == NULL) + return; + + oh = oc->objhead; + CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); Lck_AssertHeld(&oh->mtx); + + AZ(oc->flags & OC_F_BUSY); + if (oc->flags & (OC_F_WITHDRAWN|OC_F_FAILED)) + max = 1; + else if (oc->flags & (OC_F_HFM|OC_F_HFP|OC_F_CANCEL|OC_F_PRIVATE| + OC_F_DYING)) + max = cache_param->rush_exponent; + else + max = INT_MAX; + assert(max > 0); + for (i = 0; i < max; i++) { req = VTAILQ_FIRST(&oh->waitinglist); if (req == NULL) @@ -802,7 +814,7 @@ HSH_Fail(struct worker *wrk, struct objcore *oc) oc->flags |= OC_F_FAILED; if (oc->flags & OC_F_BUSY) { oc->flags &= ~OC_F_BUSY; - hsh_rush1(wrk, oh, &rush, 1); + hsh_rush1(wrk, oc, &rush); } Lck_Unlock(&oh->mtx); hsh_rush2(wrk, &rush); @@ -887,7 +899,7 @@ HSH_Withdraw(struct worker *wrk, struct objcore **ocp) assert(oc->refcnt == 1); assert(oh->refcnt > 0); oc->flags = OC_F_WITHDRAWN; - hsh_rush1(wrk, oh, &rush, 1); + hsh_rush1(wrk, oc, &rush); AZ(HSH_DerefObjCoreUnlock(wrk, &oc, 0)); hsh_rush2(wrk, &rush); @@ -933,7 +945,7 @@ HSH_Unbusy(struct worker *wrk, struct objcore *oc) oc->flags &= ~OC_F_BUSY; if (!VTAILQ_EMPTY(&oh->waitinglist)) { assert(oh->refcnt > 1); - hsh_rush1(wrk, oh, &rush, HSH_RUSH_POLICY); + hsh_rush1(wrk, oc, &rush); } Lck_Unlock(&oh->mtx); EXP_Insert(wrk, oc); /* Does nothing unless EXP_RefNewObjcore was @@ -1134,7 +1146,8 @@ HSH_DerefObjCoreUnlock(struct worker *wrk, struct objcore **ocp, int rushmax) } static int -hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh, int max) +hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh, + struct objcore *oc) { struct objhead *oh; struct rush rush; @@ -1156,7 +1169,7 @@ hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh, int max) INIT_OBJ(&rush, RUSH_MAGIC); if (!VTAILQ_EMPTY(&oh->waitinglist)) { assert(oh->refcnt > 1); - hsh_rush1(wrk, oh, &rush, max); + hsh_rush1(wrk, oc, &rush); } if (oh->refcnt == 1) @@ -1177,7 +1190,7 @@ hsh_deref_objhead(struct worker *wrk, struct objhead **poh) TAKE_OBJ_NOTNULL(oh, poh, OBJHEAD_MAGIC); Lck_Lock(&oh->mtx); - return (hsh_deref_objhead_unlock(wrk, &oh, 0)); + return (hsh_deref_objhead_unlock(wrk, &oh, NULL)); } void From d11bb699d8c1898e35b0e6c93c903e3940329ea7 Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Tue, 19 Dec 2023 10:35:23 +0100 Subject: [PATCH 08/10] hash: Retire HSH_RUSH_POLICY From now on, the policy will be derived from the objcore. --- bin/varnishd/cache/cache_ban_lurker.c | 2 +- bin/varnishd/cache/cache_busyobj.c | 3 +-- bin/varnishd/cache/cache_expire.c | 6 +++--- bin/varnishd/cache/cache_fetch.c | 8 ++++---- bin/varnishd/cache/cache_hash.c | 12 +++++------- bin/varnishd/cache/cache_objhead.h | 7 ++----- bin/varnishd/cache/cache_req_body.c | 10 +++++----- bin/varnishd/cache/cache_req_fsm.c | 16 ++++++++-------- bin/varnishd/cache/cache_vrt_var.c | 2 +- bin/varnishd/storage/storage_lru.c | 2 +- bin/varnishd/storage/storage_persistent_silo.c | 2 +- 11 files changed, 32 insertions(+), 38 deletions(-) diff --git a/bin/varnishd/cache/cache_ban_lurker.c b/bin/varnishd/cache/cache_ban_lurker.c index 76ac772b9f7..311ec31d7fe 100644 --- a/bin/varnishd/cache/cache_ban_lurker.c +++ b/bin/varnishd/cache/cache_ban_lurker.c @@ -332,7 +332,7 @@ ban_lurker_test_ban(struct worker *wrk, struct vsl_log *vsl, struct ban *bt, if (i) ObjSendEvent(wrk, oc, OEV_BANCHG); } - (void)HSH_DerefObjCore(wrk, &oc, 0); + (void)HSH_DerefObjCore(wrk, &oc); } } diff --git a/bin/varnishd/cache/cache_busyobj.c b/bin/varnishd/cache/cache_busyobj.c index 9a5fe36d11a..37d42088c5d 100644 --- a/bin/varnishd/cache/cache_busyobj.c +++ b/bin/varnishd/cache/cache_busyobj.c @@ -176,8 +176,7 @@ VBO_ReleaseBusyObj(struct worker *wrk, struct busyobj **pbo) wrk->stats->ws_backend_overflow++; if (bo->fetch_objcore != NULL) { - (void)HSH_DerefObjCore(wrk, &bo->fetch_objcore, - HSH_RUSH_POLICY); + (void)HSH_DerefObjCore(wrk, &bo->fetch_objcore); } VRT_Assign_Backend(&bo->director_req, NULL); diff --git a/bin/varnishd/cache/cache_expire.c b/bin/varnishd/cache/cache_expire.c index fb823e3cc9d..68bd56041ca 100644 --- a/bin/varnishd/cache/cache_expire.c +++ b/bin/varnishd/cache/cache_expire.c @@ -213,7 +213,7 @@ EXP_Insert(struct worker *wrk, struct objcore *oc) ObjSendEvent(wrk, oc, OEV_EXPIRE); tmpoc = oc; assert(oc->refcnt >= 2); /* Silence coverity */ - (void)HSH_DerefObjCore(wrk, &oc, 0); + (void)HSH_DerefObjCore(wrk, &oc); AZ(oc); assert(tmpoc->refcnt >= 1); /* Silence coverity */ } @@ -287,7 +287,7 @@ exp_inbox(struct exp_priv *ep, struct objcore *oc, unsigned flags, double now) VXID(ObjGetXID(ep->wrk, oc)), EXP_Ttl(NULL, oc) - now, (intmax_t)oc->hits); ObjSendEvent(ep->wrk, oc, OEV_EXPIRE); - (void)HSH_DerefObjCore(ep->wrk, &oc, 0); + (void)HSH_DerefObjCore(ep->wrk, &oc); return; } @@ -365,7 +365,7 @@ exp_expire(struct exp_priv *ep, vtim_real now) VXID(ObjGetXID(ep->wrk, oc)), EXP_Ttl(NULL, oc) - now, (intmax_t)oc->hits); ObjSendEvent(ep->wrk, oc, OEV_EXPIRE); - (void)HSH_DerefObjCore(ep->wrk, &oc, 0); + (void)HSH_DerefObjCore(ep->wrk, &oc); } return (0); } diff --git a/bin/varnishd/cache/cache_fetch.c b/bin/varnishd/cache/cache_fetch.c index 7135ae987e5..102f8b58235 100644 --- a/bin/varnishd/cache/cache_fetch.c +++ b/bin/varnishd/cache/cache_fetch.c @@ -1110,7 +1110,7 @@ vbf_fetch_thread(struct worker *wrk, void *priv) http_Teardown(bo->beresp); // can not make assumptions about the number of references here #3434 if (bo->bereq_body != NULL) - (void) HSH_DerefObjCore(bo->wrk, &bo->bereq_body, 0); + (void)HSH_DerefObjCore(bo->wrk, &bo->bereq_body); if (oc->boc->state == BOS_FINISHED) { AZ(oc->flags & OC_F_FAILED); @@ -1120,7 +1120,7 @@ vbf_fetch_thread(struct worker *wrk, void *priv) // AZ(oc->boc); // XXX if (bo->stale_oc != NULL) - (void)HSH_DerefObjCore(wrk, &bo->stale_oc, 0); + (void)HSH_DerefObjCore(wrk, &bo->stale_oc); wrk->vsl = NULL; HSH_DerefBoc(wrk, oc); @@ -1209,7 +1209,7 @@ VBF_Fetch(struct worker *wrk, struct req *req, struct objcore *oc, wrk->stats->bgfetch_no_thread++; (void)vbf_stp_fail(req->wrk, bo); if (bo->stale_oc != NULL) - (void)HSH_DerefObjCore(wrk, &bo->stale_oc, 0); + (void)HSH_DerefObjCore(wrk, &bo->stale_oc); HSH_DerefBoc(wrk, oc); SES_Rel(bo->sp); THR_SetBusyobj(NULL); @@ -1232,5 +1232,5 @@ VBF_Fetch(struct worker *wrk, struct req *req, struct objcore *oc, assert(oc->boc == boc); HSH_DerefBoc(wrk, oc); if (mode == VBF_BACKGROUND) - (void)HSH_DerefObjCore(wrk, &oc, HSH_RUSH_POLICY); + (void)HSH_DerefObjCore(wrk, &oc); } diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index 12a0496098f..d85494f7f78 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -758,7 +758,7 @@ HSH_Purge(struct worker *wrk, struct objhead *oh, vtim_real ttl_now, EXP_Remove(ocp[i], NULL); else EXP_Rearm(ocp[i], ttl_now, ttl, grace, keep); - (void)HSH_DerefObjCore(wrk, &ocp[i], 0); + (void)HSH_DerefObjCore(wrk, &ocp[i]); AZ(ocp[i]); total++; } @@ -900,7 +900,7 @@ HSH_Withdraw(struct worker *wrk, struct objcore **ocp) assert(oh->refcnt > 0); oc->flags = OC_F_WITHDRAWN; hsh_rush1(wrk, oc, &rush); - AZ(HSH_DerefObjCoreUnlock(wrk, &oc, 0)); + AZ(HSH_DerefObjCoreUnlock(wrk, &oc)); hsh_rush2(wrk, &rush); } @@ -1088,7 +1088,7 @@ HSH_DerefBoc(struct worker *wrk, struct objcore *oc) */ int -HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp, int rushmax) +HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp) { struct objcore *oc; struct objhead *oh; @@ -1101,11 +1101,11 @@ HSH_DerefObjCore(struct worker *wrk, struct objcore **ocp, int rushmax) CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); Lck_Lock(&oh->mtx); - return (HSH_DerefObjCoreUnlock(wrk, &oc, rushmax)); + return (HSH_DerefObjCoreUnlock(wrk, &oc)); } int -HSH_DerefObjCoreUnlock(struct worker *wrk, struct objcore **ocp, int rushmax) +HSH_DerefObjCoreUnlock(struct worker *wrk, struct objcore **ocp) { struct objcore *oc; struct objhead *oh; @@ -1115,8 +1115,6 @@ HSH_DerefObjCoreUnlock(struct worker *wrk, struct objcore **ocp, int rushmax) TAKE_OBJ_NOTNULL(oc, ocp, OBJCORE_MAGIC); assert(oc->refcnt > 0); - (void)rushmax; - oh = oc->objhead; CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); diff --git a/bin/varnishd/cache/cache_objhead.h b/bin/varnishd/cache/cache_objhead.h index 8d598d7b149..94ef81db527 100644 --- a/bin/varnishd/cache/cache_objhead.h +++ b/bin/varnishd/cache/cache_objhead.h @@ -76,11 +76,8 @@ int HSH_Snipe(const struct worker *, struct objcore *); struct boc *HSH_RefBoc(const struct objcore *); void HSH_DerefBoc(struct worker *wrk, struct objcore *); void HSH_DeleteObjHead(const struct worker *, struct objhead *); - -int HSH_DerefObjCore(struct worker *, struct objcore **, int rushmax); -#define HSH_RUSH_POLICY -1 - -int HSH_DerefObjCoreUnlock(struct worker *, struct objcore **, int rushmax); +int HSH_DerefObjCore(struct worker *, struct objcore **); +int HSH_DerefObjCoreUnlock(struct worker *, struct objcore **); enum lookup_e HSH_Lookup(struct req *, struct objcore **, struct objcore **); void HSH_Ref(struct objcore *o); void HSH_AddString(struct req *, void *ctx, const char *str); diff --git a/bin/varnishd/cache/cache_req_body.c b/bin/varnishd/cache/cache_req_body.c index db154dc01f7..109999a0294 100644 --- a/bin/varnishd/cache/cache_req_body.c +++ b/bin/varnishd/cache/cache_req_body.c @@ -81,7 +81,7 @@ vrb_pull(struct req *req, ssize_t maxsize, objiterate_f *func, void *priv) if (STV_NewObject(req->wrk, req->body_oc, stv, 0) == 0) { req->req_body_status = BS_ERROR; HSH_DerefBoc(req->wrk, req->body_oc); - AZ(HSH_DerefObjCore(req->wrk, &req->body_oc, 0)); + AZ(HSH_DerefObjCore(req->wrk, &req->body_oc)); (void)VFP_Error(vfc, "Object allocation failed:" " Ran out of space in %s", stv->vclname); return (-1); @@ -95,7 +95,7 @@ vrb_pull(struct req *req, ssize_t maxsize, objiterate_f *func, void *priv) if (VFP_Open(ctx, vfc) < 0) { req->req_body_status = BS_ERROR; HSH_DerefBoc(req->wrk, req->body_oc); - AZ(HSH_DerefObjCore(req->wrk, &req->body_oc, 0)); + AZ(HSH_DerefObjCore(req->wrk, &req->body_oc)); return (-1); } @@ -143,7 +143,7 @@ vrb_pull(struct req *req, ssize_t maxsize, objiterate_f *func, void *priv) VSLb_ts_req(req, "ReqBody", VTIM_real()); if (func != NULL) { HSH_DerefBoc(req->wrk, req->body_oc); - AZ(HSH_DerefObjCore(req->wrk, &req->body_oc, 0)); + AZ(HSH_DerefObjCore(req->wrk, &req->body_oc)); if (vfps == VFP_END && r == 0 && (flush & OBJ_ITER_END) == 0) r = func(priv, flush | OBJ_ITER_END, NULL, 0); if (vfps != VFP_END) { @@ -159,7 +159,7 @@ vrb_pull(struct req *req, ssize_t maxsize, objiterate_f *func, void *priv) if (vfps != VFP_END) { req->req_body_status = BS_ERROR; - AZ(HSH_DerefObjCore(req->wrk, &req->body_oc, 0)); + AZ(HSH_DerefObjCore(req->wrk, &req->body_oc)); return (-1); } @@ -280,7 +280,7 @@ VRB_Free(struct req *req) if (req->body_oc == NULL) return; - r = HSH_DerefObjCore(req->wrk, &req->body_oc, 0); + r = HSH_DerefObjCore(req->wrk, &req->body_oc); // each busyobj may have gained a reference assert (r >= 0); diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index fe84f4d7551..cb474d95f99 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -219,7 +219,7 @@ cnt_deliver(struct worker *wrk, struct req *req) ObjTouch(req->wrk, req->objcore, req->t_prev); if (Resp_Setup_Deliver(req)) { - (void)HSH_DerefObjCore(wrk, &req->objcore, HSH_RUSH_POLICY); + (void)HSH_DerefObjCore(wrk, &req->objcore); req->err_code = 500; req->req_step = R_STP_SYNTH; return (REQ_FSM_MORE); @@ -237,7 +237,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, HSH_RUSH_POLICY); + (void)HSH_DerefObjCore(wrk, &req->objcore); http_Teardown(req->resp); switch (wrk->vpi->handling) { @@ -412,7 +412,7 @@ cnt_synth(struct worker *wrk, struct req *req) VSLb(req->vsl, SLT_Error, "Could not get storage"); req->doclose = SC_OVERLOAD; VSLb_ts_req(req, "Resp", W_TIM_real(wrk)); - (void)HSH_DerefObjCore(wrk, &req->objcore, 1); + (void)HSH_DerefObjCore(wrk, &req->objcore); http_Teardown(req->resp); return (REQ_FSM_DONE); } @@ -510,7 +510,7 @@ cnt_transmit(struct worker *wrk, struct req *req) if (boc != NULL) HSH_DerefBoc(wrk, req->objcore); - (void)HSH_DerefObjCore(wrk, &req->objcore, HSH_RUSH_POLICY); + (void)HSH_DerefObjCore(wrk, &req->objcore); http_Teardown(req->resp); req->vdp_filter_list = NULL; @@ -537,7 +537,7 @@ cnt_fetch(struct worker *wrk, struct req *req) if (req->objcore->flags & OC_F_FAILED) { req->err_code = 503; req->req_step = R_STP_SYNTH; - (void)HSH_DerefObjCore(wrk, &req->objcore, 1); + (void)HSH_DerefObjCore(wrk, &req->objcore); AZ(req->objcore); return (REQ_FSM_MORE); } @@ -665,7 +665,7 @@ cnt_lookup(struct worker *wrk, struct req *req) } /* Drop our object, we won't need it */ - (void)HSH_DerefObjCore(wrk, &req->objcore, HSH_RUSH_POLICY); + (void)HSH_DerefObjCore(wrk, &req->objcore); if (busy != NULL) { HSH_Withdraw(wrk, &busy); @@ -695,7 +695,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, 0); + (void)HSH_DerefObjCore(wrk, &req->stale_oc); req->req_step = R_STP_FETCH; return (REQ_FSM_MORE); case VCL_RET_FAIL: @@ -715,7 +715,7 @@ cnt_miss(struct worker *wrk, struct req *req) } VRY_Clear(req); if (req->stale_oc != NULL) - (void)HSH_DerefObjCore(wrk, &req->stale_oc, 0); + (void)HSH_DerefObjCore(wrk, &req->stale_oc); HSH_Withdraw(wrk, &req->objcore); return (REQ_FSM_MORE); } diff --git a/bin/varnishd/cache/cache_vrt_var.c b/bin/varnishd/cache/cache_vrt_var.c index 9a14519b8ae..0d3bb16efc0 100644 --- a/bin/varnishd/cache/cache_vrt_var.c +++ b/bin/varnishd/cache/cache_vrt_var.c @@ -630,7 +630,7 @@ VRT_u_bereq_body(VRT_CTX) CHECK_OBJ_NOTNULL(ctx, VRT_CTX_MAGIC); CHECK_OBJ_NOTNULL(ctx->bo, BUSYOBJ_MAGIC); if (ctx->bo->bereq_body != NULL) { - (void)HSH_DerefObjCore(ctx->bo->wrk, &ctx->bo->bereq_body, 0); + (void)HSH_DerefObjCore(ctx->bo->wrk, &ctx->bo->bereq_body); http_Unset(ctx->bo->bereq, H_Content_Length); } diff --git a/bin/varnishd/storage/storage_lru.c b/bin/varnishd/storage/storage_lru.c index 5e046e79468..ec0369c8173 100644 --- a/bin/varnishd/storage/storage_lru.c +++ b/bin/varnishd/storage/storage_lru.c @@ -205,6 +205,6 @@ LRU_NukeOne(struct worker *wrk, struct lru *lru) ObjSlim(wrk, oc); VSLb(wrk->vsl, SLT_ExpKill, "LRU xid=%ju", VXID(ObjGetXID(wrk, oc))); - (void)HSH_DerefObjCore(wrk, &oc, 0); // Ref from HSH_Snipe + (void)HSH_DerefObjCore(wrk, &oc); // Ref from HSH_Snipe return (1); } diff --git a/bin/varnishd/storage/storage_persistent_silo.c b/bin/varnishd/storage/storage_persistent_silo.c index 81c24b4367f..eb96acd3adb 100644 --- a/bin/varnishd/storage/storage_persistent_silo.c +++ b/bin/varnishd/storage/storage_persistent_silo.c @@ -178,7 +178,7 @@ smp_load_seg(struct worker *wrk, const struct smp_sc *sc, HSH_Insert(wrk, so->hash, oc, ban); AN(oc->ban); HSH_DerefBoc(wrk, oc); // XXX Keep it an stream resurrection? - (void)HSH_DerefObjCore(wrk, &oc, HSH_RUSH_POLICY); + (void)HSH_DerefObjCore(wrk, &oc); wrk->stats->n_vampireobject++; } Pool_Sumstat(wrk); From 6b0a8c8e7dda3d19b8728a77ab822385879f77fd Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Mon, 22 Jan 2024 17:17:04 +0100 Subject: [PATCH 09/10] hash: Skip the cache ceremony for private objects This reduces a great deal of spurious activity on the private_oh waiting list and removes unncessary conditionals when dealing with cacheable (or at least searchable) objects. There is still some waiting list no-op activity via HSH_Fail() but it can be considered negligible compared to the happy path. --- bin/varnishd/cache/cache_hash.c | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index d85494f7f78..f3fb431561e 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -918,27 +918,36 @@ HSH_Unbusy(struct worker *wrk, struct objcore *oc) CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); CHECK_OBJ_NOTNULL(oc->boc, BOC_MAGIC); + AN(oc->flags & OC_F_BUSY); oh = oc->objhead; CHECK_OBJ(oh, OBJHEAD_MAGIC); + + /* NB: It is guaranteed that exactly one request is waiting for + * the objcore for pass objects. The other reference is held by + * the current fetch task. + */ + if (oc->flags & OC_F_PRIVATE) { + assert(oc->refcnt == 2); + oc->flags &= ~OC_F_BUSY; + return; + } + INIT_OBJ(&rush, RUSH_MAGIC); AN(oc->stobj->stevedore); - AN(oc->flags & OC_F_BUSY); + AZ(oc->flags & OC_F_PRIVATE); assert(oh->refcnt > 0); assert(oc->refcnt > 0); - if (!(oc->flags & OC_F_PRIVATE)) { - BAN_NewObjCore(oc); - AN(oc->ban); - } + BAN_NewObjCore(oc); + AN(oc->ban); /* XXX: pretouch neighbors on oh->objcs to prevent page-on under mtx */ Lck_Lock(&oh->mtx); assert(oh->refcnt > 0); assert(oc->refcnt > 0); - if (!(oc->flags & OC_F_PRIVATE)) - EXP_RefNewObjcore(oc); /* Takes a ref for expiry */ + EXP_RefNewObjcore(oc); /* Takes a ref for expiry */ /* XXX: strictly speaking, we should sort in Date: order. */ VTAILQ_REMOVE(&oh->objcs, oc, hsh_list); VTAILQ_INSERT_HEAD(&oh->objcs, oc, hsh_list); @@ -948,8 +957,7 @@ HSH_Unbusy(struct worker *wrk, struct objcore *oc) hsh_rush1(wrk, oc, &rush); } Lck_Unlock(&oh->mtx); - EXP_Insert(wrk, oc); /* Does nothing unless EXP_RefNewObjcore was - * called */ + EXP_Insert(wrk, oc); hsh_rush2(wrk, &rush); } From 0bd65aa9633e6b7fc2be18a8082c3d2975c85f4a Mon Sep 17 00:00:00 2001 From: Dridi Boukelmoune Date: Mon, 22 Jan 2024 23:17:44 +0100 Subject: [PATCH 10/10] hash: A waiting list match is a formal revalidation Not only does the waiting list operate on an objcore, it also passes a reference to requests before they reembark a worker. This way when an objcore is already present during lookup we can attempt a cache hit without holding the objhead lock. Instead of repurposing the req::hash_objhead field into an equivalent req::hash_objcore, the field is actually removed. In order to signal that a request comes back from its waiting list, the life time of the req::waitinglist flag is extended until cnt_lookup() is reentered. If the rushed objcore matches a request, the lookup can result in a hit without entering the regular lookup critical section. The objhead lock is briefly acquired to release req's reference on the objhead, to rely solely on the objcore's objhead reference like a normal hit. This shifts the infamous waiting list serialization phenomenon to the vary header match. The rush policy is applied wholesale on cacheable objects instead of exponentially. This improves waiting list latency when the outcome is a cache hit, but forces incompatible variants to reenter a lookup and potentially disembark into the waiting list again. Since most spurious rushes at every corner of objhead activity are gone, this change puts all the spurious activity on the incompatible variants alone instead of all objects on more occasions. If a cacheable object was inserted in the cache, but already expired, this behavior enables cache hits. This can be common with multi-tier Varnish setups where one Varnish server may serve a graced object to an other, but true of any origin server that may serve stale yet valid responses. The waiting list enables a proper response-wide no-cache behavior from now on, but the built-in VCL prevents it by default. This is also the first step towards implementing no-cache and private support at the header field granularity. --- bin/varnishd/cache/cache.h | 3 - bin/varnishd/cache/cache_hash.c | 80 ++++++++++++--- bin/varnishd/cache/cache_req_fsm.c | 15 +-- bin/varnishd/cache/cache_vary.c | 3 +- bin/varnishtest/tests/c00125.vtc | 156 +++++++++++++++++++++++++++++ 5 files changed, 234 insertions(+), 23 deletions(-) create mode 100644 bin/varnishtest/tests/c00125.vtc diff --git a/bin/varnishd/cache/cache.h b/bin/varnishd/cache/cache.h index 7d6b684571c..6f80c14a698 100644 --- a/bin/varnishd/cache/cache.h +++ b/bin/varnishd/cache/cache.h @@ -491,9 +491,6 @@ struct req { struct objcore *body_oc; - /* The busy objhead we sleep on */ - struct objhead *hash_objhead; - /* Built Vary string == workspace reservation */ uint8_t *vary_b; uint8_t *vary_e; diff --git a/bin/varnishd/cache/cache_hash.c b/bin/varnishd/cache/cache_hash.c index f3fb431561e..4f26f1de8d6 100644 --- a/bin/varnishd/cache/cache_hash.c +++ b/bin/varnishd/cache/cache_hash.c @@ -355,6 +355,41 @@ hsh_insert_busyobj(const struct worker *wrk, struct objhead *oh) return (oc); } +/*--------------------------------------------------------------------- +*/ + +static unsigned +hsh_rush_match(struct req *req) +{ + struct objhead *oh; + struct objcore *oc; + const uint8_t *vary; + + oc = req->objcore; + CHECK_OBJ_NOTNULL(oc, OBJCORE_MAGIC); + + AZ(oc->flags & OC_F_BUSY); + if (oc->flags & (OC_F_WITHDRAWN|OC_F_HFM|OC_F_HFP|OC_F_CANCEL| + OC_F_PRIVATE|OC_F_FAILED|OC_F_DYING)) + + return (0); + + if (req->vcf != NULL) /* NB: must operate under oh lock. */ + return (0); + + oh = oc->objhead; + CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC); + + if (req->hash_ignore_vary) + return (1); + if (!ObjHasAttr(req->wrk, oc, OA_VARY)) + return (1); + + vary = ObjGetAttr(req->wrk, oc, OA_VARY, NULL); + AN(vary); + return (VRY_Match(req, vary)); +} + /*--------------------------------------------------------------------- */ @@ -383,6 +418,7 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(wrk->wpriv, WORKER_PRIV_MAGIC); CHECK_OBJ_NOTNULL(req->http, HTTP_MAGIC); + CHECK_OBJ_ORNULL(req->objcore, OBJCORE_MAGIC); CHECK_OBJ_ORNULL(req->vcf, VCF_MAGIC); AN(hash); @@ -390,15 +426,32 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) if (DO_DEBUG(DBG_HASHEDGE)) hsh_testmagic(req->digest); - if (req->hash_objhead != NULL) { + if (req->objcore != NULL && hsh_rush_match(req)) { + TAKE_OBJ_NOTNULL(oc, &req->objcore, OBJCORE_MAGIC); + *ocp = oc; + oh = oc->objhead; + Lck_Lock(&oh->mtx); + oc->hits++; + boc_progress = oc->boc == NULL ? -1 : oc->boc->fetched_so_far; + AN(hsh_deref_objhead_unlock(wrk, &oh, NULL)); + Req_LogHit(wrk, req, oc, boc_progress); + /* NB: since this hit comes from the waiting list instead of + * a regular lookup, grace is not considered. The object is + * fresh in the context of the waiting list, even expired: it + * was successfully just [re]validated by a fetch task. + */ + return (HSH_HIT); + } + + if (req->objcore != NULL) { /* * This req came off the waiting list, and brings an - * oh refcnt with it. + * oh refcnt and an incompatible oc refcnt with it, + * the latter acquired during rush hour. */ - CHECK_OBJ_NOTNULL(req->hash_objhead, OBJHEAD_MAGIC); - oh = req->hash_objhead; + oh = req->objcore->objhead; + (void)HSH_DerefObjCore(wrk, &req->objcore); Lck_Lock(&oh->mtx); - req->hash_objhead = NULL; } else { AN(wrk->wpriv->nobjhead); oh = hash->lookup(wrk, req->digest, &wrk->wpriv->nobjhead); @@ -581,11 +634,12 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp) AZ(req->hash_ignore_busy); /* - * The objhead reference transfers to the sess, we get it - * back when the sess comes off the waiting list and - * calls us again + * The objhead reference is held by req while it is parked on the + * waiting list. The oh pointer is taken back from the objcore that + * triggers a rush of req off the waiting list. */ - req->hash_objhead = oh; + assert(oh->refcnt > 1); + req->wrk = NULL; req->waitinglist = 1; @@ -640,8 +694,10 @@ hsh_rush1(const struct worker *wrk, struct objcore *oc, struct rush *r) AZ(req->wrk); VTAILQ_REMOVE(&oh->waitinglist, req, w_list); VTAILQ_INSERT_TAIL(&r->reqs, req, w_list); - req->waitinglist = 0; + req->objcore = oc; } + + oc->refcnt += i; } /*--------------------------------------------------------------------- @@ -899,8 +955,8 @@ HSH_Withdraw(struct worker *wrk, struct objcore **ocp) assert(oc->refcnt == 1); assert(oh->refcnt > 0); oc->flags = OC_F_WITHDRAWN; - hsh_rush1(wrk, oc, &rush); - AZ(HSH_DerefObjCoreUnlock(wrk, &oc)); + hsh_rush1(wrk, oc, &rush); /* grabs up to 1 oc ref */ + assert(HSH_DerefObjCoreUnlock(wrk, &oc) <= 1); hsh_rush2(wrk, &rush); } diff --git a/bin/varnishd/cache/cache_req_fsm.c b/bin/varnishd/cache/cache_req_fsm.c index cb474d95f99..4f42fb65775 100644 --- a/bin/varnishd/cache/cache_req_fsm.c +++ b/bin/varnishd/cache/cache_req_fsm.c @@ -556,20 +556,23 @@ cnt_lookup(struct worker *wrk, struct req *req) { struct objcore *oc, *busy; enum lookup_e lr; - int had_objhead = 0; + int had_objcore = 0; CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC); CHECK_OBJ_NOTNULL(req, REQ_MAGIC); - AZ(req->objcore); AZ(req->stale_oc); AN(req->vcl); VRY_Prep(req); - AZ(req->objcore); - if (req->hash_objhead) - had_objhead = 1; + if (req->waitinglist) { + CHECK_OBJ_NOTNULL(req->objcore, OBJCORE_MAGIC); + req->waitinglist = 0; + had_objcore = 1; + } else + AZ(req->objcore); + wrk->strangelove = 0; lr = HSH_Lookup(req, &oc, &busy); if (lr == HSH_BUSY) { @@ -585,7 +588,7 @@ cnt_lookup(struct worker *wrk, struct req *req) if ((unsigned)wrk->strangelove >= cache_param->vary_notice) VSLb(req->vsl, SLT_Notice, "vsl: High number of variants (%d)", wrk->strangelove); - if (had_objhead) + if (had_objcore) VSLb_ts_req(req, "Waitinglist", W_TIM_real(wrk)); if (req->vcf != NULL) { diff --git a/bin/varnishd/cache/cache_vary.c b/bin/varnishd/cache/cache_vary.c index 101eca231b0..9865769d768 100644 --- a/bin/varnishd/cache/cache_vary.c +++ b/bin/varnishd/cache/cache_vary.c @@ -224,8 +224,7 @@ vry_cmp(const uint8_t *v1, const uint8_t *v2) void VRY_Prep(struct req *req) { - if (req->hash_objhead == NULL) { - /* Not a waiting list return */ + if (!req->waitinglist) { AZ(req->vary_b); AZ(req->vary_e); (void)WS_ReserveAll(req->ws); diff --git a/bin/varnishtest/tests/c00125.vtc b/bin/varnishtest/tests/c00125.vtc new file mode 100644 index 00000000000..8fbca4f46b2 --- /dev/null +++ b/bin/varnishtest/tests/c00125.vtc @@ -0,0 +1,156 @@ +varnishtest "successful expired waiting list hit" + +barrier b1 cond 2 +barrier b2 cond 2 +barrier b3 cond 2 +barrier b4 cond 2 + + +server s1 { + rxreq + expect req.http.user-agent == c1 + expect req.http.bgfetch == false + barrier b1 sync + barrier b2 sync + txresp -hdr "Cache-Control: max-age=60" -hdr "Age: 120" + + rxreq + expect req.http.user-agent == c3 + expect req.http.bgfetch == true + txresp + + # The no-cache case only works with a complicit VCL, for now. + rxreq + expect req.http.user-agent == c4 + expect req.http.bgfetch == false + barrier b3 sync + barrier b4 sync + txresp -hdr "Cache-Control: no-cache" + + rxreq + expect req.http.user-agent == c6 + expect req.http.bgfetch == false + txresp -hdr "Cache-Control: no-cache" +} -start + +varnish v1 -cliok "param.set default_grace 1h" +varnish v1 -cliok "param.set thread_pools 1" +varnish v1 -cliok "param.set debug +syncvsl,+waitinglist" +varnish v1 -vcl+backend { + sub vcl_backend_fetch { + set bereq.http.bgfetch = bereq.is_bgfetch; + } + sub vcl_beresp_stale { + # We just validated a stale object, do not mark it as + # uncacheable. The object remains available for grace + # hits and background fetches. + return; + } + sub vcl_beresp_control { + if (beresp.http.cache-control == "no-cache") { + # Keep beresp.uncacheable clear. + return; + } + } + sub vcl_deliver { + set resp.http.obj-hits = obj.hits; + set resp.http.obj-ttl = obj.ttl; + } +} -start + +client c1 { + txreq -url "/stale-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == 1001 + expect resp.http.obj-hits == 0 + expect resp.http.obj-ttl < 0 +} -start + +barrier b1 sync + +client c2 { + txreq -url "/stale-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == "1004 1002" + expect resp.http.obj-hits == 1 + expect resp.http.obj-ttl < 0 +} -start + +varnish v1 -expect busy_sleep == 1 +barrier b2 sync + +client c1 -wait +client c2 -wait + +varnish v1 -vsl_catchup + +varnish v1 -expect cache_miss == 1 +varnish v1 -expect cache_hit == 1 +varnish v1 -expect cache_hit_grace == 0 +varnish v1 -expect s_bgfetch == 0 + +client c3 { + txreq -url "/stale-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == "1006 1002" + expect resp.http.obj-hits == 2 + expect resp.http.obj-ttl < 0 +} -run + +varnish v1 -vsl_catchup + +varnish v1 -expect cache_miss == 1 +varnish v1 -expect cache_hit == 2 +varnish v1 -expect cache_hit_grace == 1 +varnish v1 -expect s_bgfetch == 1 + +# The only way for a plain no-cache to be hit is to have a non-zero keep. +varnish v1 -cliok "param.set default_ttl 0" +varnish v1 -cliok "param.set default_grace 0" +varnish v1 -cliok "param.set default_keep 1h" + +client c4 { + txreq -url "/no-cache-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == 1009 + expect resp.http.obj-hits == 0 + expect resp.http.obj-ttl <= 0 +} -start + +barrier b3 sync + +client c5 { + txreq -url "/no-cache-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == "1012 1010" + expect resp.http.obj-hits == 1 + expect resp.http.obj-ttl <= 0 +} -start + +varnish v1 -expect busy_sleep == 2 +barrier b4 sync + +client c4 -wait +client c5 -wait + +varnish v1 -vsl_catchup + +varnish v1 -expect cache_miss == 2 +varnish v1 -expect cache_hit == 3 +varnish v1 -expect cache_hit_grace == 1 +varnish v1 -expect s_bgfetch == 1 + +# No hit when not on the waiting list +client c6 { + txreq -url "/no-cache-hit" + rxresp + expect resp.status == 200 + expect resp.http.x-varnish == 1014 + expect resp.http.obj-hits == 0 + expect resp.http.obj-ttl <= 0 +} -run