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

h2: Make requests walk away from the waiting list #3835

Closed
dridi wants to merge 5 commits into from
Closed

h2: Make requests walk away from the waiting list #3835

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
808c673
transport: Add a waiting list methods for req_top
dridi Nov 4, 2020
9200141
transport: Remove the vtr_reembark_f typedef
dridi Sep 15, 2022
7d53b8b
hash: New HSH_WalkAway() function
dridi Nov 4, 2020
2e9bcc3
h2: Keep track of streams entering waiting lists
dridi Nov 4, 2020
f6ebc5a
h2: Make requests walk away from the waiting list
dridi Aug 12, 2022
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
4 changes: 3 additions & 1 deletion bin/varnishd/cache/cache.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -485,11 +485,13 @@ struct req {
void *transport_priv;

VTAILQ_ENTRY(req) w_list;
VTAILQ_ENTRY(req) t_list;

@mbgrydeland mbgrydeland Sep 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This feels like quite the waste of memory adding 24 bytes for the list entry and the objhead reference flag on struct req. I think I prefer the original way of doing the workspace allocation, perhaps replaced/expanded with a fallback malloc.

@dridi dridi Sep 14, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I had considered this, and decided it would be simpler, especially with the generalization of the feature to include HTTP/1 too.

I can ponder something similar to what we did downstream but more general (not tied to h2 code).

@dridi dridi Sep 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I just remembered why I didn't use the workspace: it is reserved for vary processing.

@dridi dridi Sep 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I got some results on the generalization side, but didn't manage to without allocation space somewhere. Since workspace is reserved during waiting list operations, it is either a heap allocation or preallocated storage in struct req (current solution). Another possibility could be to opportunistically allocate an "entry" for transport tracking on the workspace before the vary reservation, if the top transport has waiting list methods.

To be continued.

@dridi dridi Sep 19, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Considering that eventually both h2 and http/1 (100% of client top transports) will track requests and sub-requests in waiting lists, it would be much simpler to go with the current design and have the tracking state directly in struct req.


struct objcore *body_oc;

/* The busy objhead we sleep on */
/* The busy objhead we sleep on, referenced up to twice */
struct objhead *hash_objhead;
struct objhead *transport_objhead;

/* Built Vary string == workspace reservation */
uint8_t *vary_b;
Expand Down
4 changes: 2 additions & 2 deletions bin/varnishd/cache/cache_esi_deliver.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
#include "vgz.h"

static vtr_deliver_f ved_deliver;
static vtr_reembark_f ved_reembark;
static vtr_waitlist_f ved_reembark;

static const uint8_t gzip_hdr[] = {
0x1f, 0x8b, 0x08,
Expand Down Expand Up @@ -91,7 +91,7 @@ static const struct transport VED_transport = {

/*--------------------------------------------------------------------*/

static void v_matchproto_(vtr_reembark_f)
static void v_matchproto_(vtr_waitlist_f)
ved_reembark(struct worker *wrk, struct req *req)
{
struct ecx *ecx;
Expand Down
131 changes: 104 additions & 27 deletions bin/varnishd/cache/cache_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -355,6 +355,63 @@ hsh_insert_busyobj(const struct worker *wrk, struct objhead *oh)
return (oc);
}

/*---------------------------------------------------------------------
*/

static void
hsh_waitlist(struct worker *wrk, struct req *req, struct objhead *oh)
{

CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
Lck_AssertHeld(&oh->mtx);
AZ(req->hash_ignore_busy);
AZ(req->hash_objhead);

DSLb(DBG_WAITINGLIST, "on waiting list <%p>", oh);

/*
* The objhead reference transfers to the sess, we get it
* back when the sess comes off the waiting list and
* calls us again
*/
req->hash_objhead = oh;
req->wrk = NULL;
req->waitinglist = 1;

if (req->top->topreq->transport->top_waitlist != NULL)
req->top->topreq->transport->top_waitlist(wrk, req);
}

static void
hsh_reembark(struct worker *wrk, struct req *req)
{

CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);

if (DO_DEBUG(DBG_WAITINGLIST))
VSLb(req->vsl, SLT_Debug, "off waiting list");

if (req->top->topreq->transport->top_reembark != NULL)
req->top->topreq->transport->top_reembark(wrk, req);

if (req->transport->reembark != NULL) {
// For ESI includes
req->transport->reembark(wrk, req);
return;
}

/*
* We ignore the queue limits which apply to new
* requests because if we fail to reschedule there
* may be vmod_privs to cleanup and we need a proper
* worker thread for that.
*/
AZ(Pool_Task(req->sp->pool, req->task, TASK_QUEUE_RUSH));
}

/*---------------------------------------------------------------------
*/

Expand Down Expand Up @@ -407,6 +464,12 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)
CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);
Lck_AssertHeld(&oh->mtx);

if (req->walkaway) {
assert(oh->refcnt > 0);
(void)hsh_deref_objhead_unlock(wrk, &oh, 0);
return (HSH_WALKAWAY);
}

if (req->hash_always_miss) {
/* XXX: should we do predictive Vary in this case ? */
/* Insert new objcore in objecthead and release mutex */
Expand Down Expand Up @@ -577,21 +640,11 @@ HSH_Lookup(struct req *req, struct objcore **ocp, struct objcore **bocp)

/* There are one or more busy objects, wait for them */
VTAILQ_INSERT_TAIL(&oh->waitinglist, req, w_list);
hsh_waitlist(wrk, req, oh);

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
*/
req->hash_objhead = oh;
req->wrk = NULL;
req->waitinglist = 1;

if (DO_DEBUG(DBG_WAITINGLIST))
VSLb(req->vsl, SLT_Debug, "on waiting list <%p>", oh);

/* NOTE OBS: We are going on the waitinglist. No changing of
* anything on struct req after releasing the mutex, as we may be
* rescheduled immediately. */
Lck_Unlock(&oh->mtx);

wrk->stats->busy_sleep++;
Expand Down Expand Up @@ -648,19 +701,43 @@ hsh_rush2(struct worker *wrk, struct rush *r)
req = VTAILQ_FIRST(&r->reqs);
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
VTAILQ_REMOVE(&r->reqs, req, w_list);
DSL(DBG_WAITINGLIST, req->vsl->wid, "off waiting list");
if (req->transport->reembark != NULL) {
// For ESI includes
req->transport->reembark(wrk, req);
} else {
/*
* We ignore the queue limits which apply to new
* requests because if we fail to reschedule there
* may be vmod_privs to cleanup and we need a proper
* workerthread for that.
*/
AZ(Pool_Task(req->sp->pool, req->task, TASK_QUEUE_RUSH));
}
hsh_reembark(wrk, req);
}
}

void
HSH_WalkAway(struct worker *wrk, struct objhead **ohp, struct req *req)
{
struct objhead *oh;
struct rush r[1];

CHECK_OBJ_NOTNULL(wrk, WORKER_MAGIC);
CHECK_OBJ_NOTNULL(req, REQ_MAGIC);
TAKE_OBJ_NOTNULL(oh, ohp, OBJHEAD_MAGIC);

INIT_OBJ(r, RUSH_MAGIC);
VTAILQ_INIT(&r->reqs);

if (DO_DEBUG(DBG_WAITINGLIST))
VSLb(req->vsl, SLT_Debug, "walking away <%p>", oh);

Lck_Lock(&oh->mtx);
if (req->waitinglist) {
assert(oh == req->hash_objhead);
assert(oh->refcnt > 1);
oh->refcnt--;
VTAILQ_REMOVE(&oh->waitinglist, req, w_list);
VTAILQ_INSERT_TAIL(&r->reqs, req, w_list);
Lck_Unlock(&oh->mtx);

wrk->stats->busy_killed++;
AZ(req->walkaway);
req->walkaway = 1;
hsh_rush2(wrk, r);
} else {
AZ(req->hash_objhead);
assert(oh->refcnt > 0);
(void)hsh_deref_objhead_unlock(wrk, &oh, 0);
}
}

Expand Down
1 change: 1 addition & 0 deletions bin/varnishd/cache/cache_objhead.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,3 +76,4 @@ unsigned HSH_Purge(struct worker *, struct objhead *, vtim_real ttl_now,
vtim_dur ttl, vtim_dur grace, vtim_dur keep);
struct objcore *HSH_Private(const struct worker *wrk);
void HSH_Cancel(struct worker *, struct objcore *, struct boc *);
void HSH_WalkAway(struct worker *wrk, struct objhead **ohp, struct req *req);
15 changes: 15 additions & 0 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 @@ -562,6 +562,21 @@ cnt_lookup(struct worker *wrk, struct req *req)
had_objhead = 1;
wrk->strangelove = 0;
lr = HSH_Lookup(req, &oc, &busy);
if (lr == HSH_WALKAWAY) {
VRY_Finish(req, DISCARD);
AZ(req->objcore);
AZ(req->stale_oc);
VSLb_ts_req(req, "Waitinglist", W_TIM_real(wrk));
VSLb(req->vsl, SLT_Error, "The client is going away");
if (req->esi_level > 0) {

@mbgrydeland mbgrydeland Sep 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why should we only go to VCLFAIL for subrequests? This implies that HSH_WalkAway will only ever be called as part of a broken connection, and we have to close. Could it not be useful also for a possible future waitinglist timeout feature, and we need to produce a 503?

@dridi dridi Sep 14, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

First, we can add a dedicated HSH_TIMEOUT. Otherwise I can use the same argument for the error message preventing us to use this branch for future timeouts. I think we will be fine if we dedicated HSH_WALKAWAY to the case where the client went away while a request was in a waiting list.

For the timeout case, we probably want to have different semantics than plainly ending the transaction. Maybe a waiting list timeout would be a deferred hash_ignore_busy?

I must admit I don't remember precisely why we did that downstream. I think we needed this to interrupt ESI delivery and prevent it from recursing as the client is gone anyway.

@dridi dridi Sep 15, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's actually a comment a few lines below explaining why we fail sub-requests:

/* NB: Interrupt delivery upwards to avoid engaging
 * new sub-request tasks.
 */

Granted, it doesn't give many details.

/* NB: Interrupt delivery upwards to avoid engaging
* new sub-request tasks.
*/
req->req_step = R_STP_VCLFAIL;
return (REQ_FSM_MORE);
}
return (REQ_FSM_DONE);
}
if (lr == HSH_BUSY) {
/*
* We lost the session to a busy object, disembark the
Expand Down
6 changes: 4 additions & 2 deletions bin/varnishd/cache/cache_transport.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ typedef void vtr_req_body_f (struct req *);
typedef void vtr_sess_panic_f (struct vsb *, const struct sess *);
typedef void vtr_req_panic_f (struct vsb *, const struct req *);
typedef void vtr_req_fail_f (struct req *, stream_close_t);
typedef void vtr_reembark_f (struct worker *, struct req *);
typedef void vtr_waitlist_f (struct worker *, struct req *);
typedef int vtr_minimal_response_f (struct req *, uint16_t status);

struct transport {
Expand All @@ -63,7 +63,9 @@ struct transport {
vtr_deliver_f *deliver;
vtr_sess_panic_f *sess_panic;
vtr_req_panic_f *req_panic;
vtr_reembark_f *reembark;
vtr_waitlist_f *top_waitlist;
vtr_waitlist_f *top_reembark;
vtr_waitlist_f *reembark;
vtr_minimal_response_f *minimal_response;

VTAILQ_ENTRY(transport) list;
Expand Down
2 changes: 1 addition & 1 deletion bin/varnishd/hash/hash_slinger.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ struct hash_slinger {
};

enum lookup_e {
HSH_CONTINUE,
HSH_WALKAWAY,
HSH_MISS,
HSH_BUSY,
HSH_HIT,
Expand Down
1 change: 1 addition & 0 deletions bin/varnishd/http2/cache_http2.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ struct h2_req {
int counted;
struct h2_sess *h2sess;
struct req *req;
VTAILQ_HEAD(, req) waitinglist;
double t_send;
double t_winupd;
pthread_cond_t *cond;
Expand Down
32 changes: 28 additions & 4 deletions bin/varnishd/http2/cache_http2_proto.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ h2_new_req(struct h2_sess *h2, unsigned stream, struct req *req)
r2->r_window = h2->local_settings.initial_window_size;
r2->t_window = h2->remote_settings.initial_window_size;
req->transport_priv = r2;
VTAILQ_INIT(&r2->waitinglist);
Lck_Lock(&h2->sess->mtx);
if (stream)
h2->open_streams++;
Expand All @@ -182,6 +183,7 @@ h2_del_req(struct worker *wrk, struct h2_req *r2)
ASSERT_RXTHR(h2);
sp = h2->sess;
Lck_Lock(&sp->mtx);
assert(VTAILQ_EMPTY(&r2->waitinglist));
assert(h2->refcnt > 0);
--h2->refcnt;
/* XXX: PRIORITY reshuffle */
Expand All @@ -207,9 +209,13 @@ void
h2_kill_req(struct worker *wrk, struct h2_sess *h2,
struct h2_req *r2, h2_error h2e)
{
VTAILQ_HEAD(, req) wl;
struct req *req, *t;
struct objhead *oh;

ASSERT_RXTHR(h2);
AN(h2e);
VTAILQ_INIT(&wl);
Lck_Lock(&h2->sess->mtx);
VSLb(h2->vsl, SLT_Debug, "KILL st=%u state=%d sched=%d",
r2->stream, r2->state, r2->scheduled);
Expand All @@ -221,16 +227,34 @@ h2_kill_req(struct worker *wrk, struct h2_sess *h2,
if (r2->error == NULL)
r2->error = h2e;
if (r2->scheduled) {
if (r2->cond != NULL)
AZ(pthread_cond_signal(r2->cond));
r2 = NULL;
VTAILQ_FOREACH_SAFE(req, &r2->waitinglist, t_list, t) {
CHECK_OBJ(req, REQ_MAGIC);
VTAILQ_REMOVE(&r2->waitinglist, req, t_list);
VTAILQ_INSERT_TAIL(&wl, req, t_list);
}
if (VTAILQ_EMPTY(&wl)) {
if (r2->cond != NULL)
AZ(pthread_cond_signal(r2->cond));
r2 = NULL;
}
} else {
if (r2->state == H2_S_OPEN && h2->new_req == r2->req)
(void)h2h_decode_fini(h2);
}
Lck_Unlock(&h2->sess->mtx);
if (r2 != NULL)
if (VTAILQ_EMPTY(&wl) && r2 != NULL) {
h2_del_req(wrk, r2);
return;
}
VTAILQ_FOREACH_SAFE(req, &wl, t_list, t) {
Lck_Lock(&h2->sess->mtx);
VTAILQ_REMOVE(&wl, req, t_list);
oh = req->transport_objhead;
req->transport_objhead = NULL;
Lck_Unlock(&h2->sess->mtx);
HSH_WalkAway(wrk, &oh, req);
AZ(oh);
}
}

/**********************************************************************/
Expand Down
Loading