From e388f2fbca40197590bd15dce0f4eb4d6cded20a Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Tue, 2 Mar 2021 16:51:09 +0100 Subject: [PATCH] MEDIUM: muxes: mark idle conns tasklets with TASK_F_USR1 The muxes are touching the idle_conns_lock all the time now because they need to be careful that no other thread has stolen their tasklet's context. This patch changes this a little bit by setting the TASK_F_USR1 flag on the tasklet before marking a connection idle, and removing it once it's not idle anymore. Thanks to this we have the guarantee that a tasklet without this flag cannot be present in an idle list and does not need to go through this costly lock. This is especially true for front connections. --- src/mux_fcgi.c | 56 +++++++++++++++++++++++++++----------------- src/mux_h1.c | 61 ++++++++++++++++++++++++++++++------------------ src/mux_h2.c | 63 +++++++++++++++++++++++++++++++------------------- 3 files changed, 112 insertions(+), 68 deletions(-) diff --git a/src/mux_fcgi.c b/src/mux_fcgi.c index d5302a004..be063cf40 100644 --- a/src/mux_fcgi.c +++ b/src/mux_fcgi.c @@ -2975,36 +2975,42 @@ schedule: } /* this is the tasklet referenced in fconn->wait_event.tasklet */ -struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int status) +struct task *fcgi_io_cb(struct task *t, void *ctx, unsigned int state) { struct connection *conn; - struct fcgi_conn *fconn; + struct fcgi_conn *fconn = ctx; struct tasklet *tl = (struct tasklet *)t; int conn_in_list; int ret = 0; - - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - if (tl->context == NULL) { - /* The connection has been taken over by another thread, - * we're no longer responsible for it, so just free the - * tasklet, and do nothing. + if (state & TASK_F_USR1) { + /* the tasklet was idling on an idle connection, it might have + * been stolen, let's be careful! */ + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (tl->context == NULL) { + /* The connection has been taken over by another thread, + * we're no longer responsible for it, so just free the + * tasklet, and do nothing. + */ + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + tasklet_free(tl); + return NULL; + } + conn = fconn->conn; + TRACE_POINT(FCGI_EV_FCONN_WAKE, conn); + + conn_in_list = conn->flags & CO_FL_LIST_MASK; + if (conn_in_list) + conn_delete_from_tree(&conn->hash_node->node); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - tasklet_free(tl); - return NULL; - + } else { + /* we're certain the connection was not in an idle list */ + conn = fconn->conn; + TRACE_ENTER(FCGI_EV_FCONN_WAKE, conn); + conn_in_list = 0; } - fconn = ctx; - conn = fconn->conn; - - TRACE_POINT(FCGI_EV_FCONN_WAKE, conn); - - conn_in_list = conn->flags & CO_FL_LIST_MASK; - if (conn_in_list) - conn_delete_from_tree(&conn->hash_node->node); - - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (!(fconn->wait_event.events & SUB_RETRY_SEND)) ret = fcgi_send(fconn); @@ -3480,6 +3486,10 @@ static struct conn_stream *fcgi_attach(struct connection *conn, struct session * cs_free(cs); goto err; } + + /* the connection is not idle anymore, let's mark this */ + HA_ATOMIC_AND(&fconn->wait_event.tasklet->state, ~TASK_F_USR1); + TRACE_LEAVE(FCGI_EV_FSTRM_NEW, conn, fstrm); return cs; @@ -3606,6 +3616,10 @@ static void fcgi_detach(struct conn_stream *cs) fconn->conn->owner = NULL; } + /* mark that the tasklet may lose its context to another thread and + * that the handler needs to check it under the idle conns lock. + */ + HA_ATOMIC_OR(&fconn->wait_event.tasklet->state, TASK_F_USR1); if (!srv_add_to_idle_list(objt_server(fconn->conn->target), fconn->conn, 1)) { /* The server doesn't want it, let's kill the connection right away */ fconn->conn->mux->destroy(fconn); diff --git a/src/mux_h1.c b/src/mux_h1.c index d5b9e9944..7d1bed48d 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -2799,38 +2799,45 @@ static int h1_process(struct h1c * h1c) return -1; } -struct task *h1_io_cb(struct task *t, void *ctx, unsigned int status) +struct task *h1_io_cb(struct task *t, void *ctx, unsigned int state) { struct connection *conn; struct tasklet *tl = (struct tasklet *)t; int conn_in_list; - struct h1c *h1c; + struct h1c *h1c = ctx; int ret = 0; - - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - if (tl->context == NULL) { - /* The connection has been taken over by another thread, - * we're no longer responsible for it, so just free the - * tasklet, and do nothing. + if (state & TASK_F_USR1) { + /* the tasklet was idling on an idle connection, it might have + * been stolen, let's be careful! */ + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (tl->context == NULL) { + /* The connection has been taken over by another thread, + * we're no longer responsible for it, so just free the + * tasklet, and do nothing. + */ + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + tasklet_free(tl); + return NULL; + } + conn = h1c->conn; + TRACE_POINT(H1_EV_H1C_WAKE, conn); + + /* Remove the connection from the list, to be sure nobody attempts + * to use it while we handle the I/O events + */ + conn_in_list = conn->flags & CO_FL_LIST_MASK; + if (conn_in_list) + conn_delete_from_tree(&conn->hash_node->node); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - tasklet_free(tl); - return NULL; + } else { + /* we're certain the connection was not in an idle list */ + conn = h1c->conn; + TRACE_ENTER(H1_EV_H1C_WAKE, conn); + conn_in_list = 0; } - h1c = ctx; - conn = h1c->conn; - - TRACE_POINT(H1_EV_H1C_WAKE, conn); - - /* Remove the connection from the list, to be sure nobody attempts - * to use it while we handle the I/O events - */ - conn_in_list = conn->flags & CO_FL_LIST_MASK; - if (conn_in_list) - conn_delete_from_tree(&conn->hash_node->node); - - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (!(h1c->wait_event.events & SUB_RETRY_SEND)) ret = h1_send(h1c); @@ -2998,6 +3005,9 @@ static struct conn_stream *h1_attach(struct connection *conn, struct session *se goto err; } + /* the connection is not idle anymore, let's mark this */ + HA_ATOMIC_AND(&h1c->wait_event.tasklet->state, ~TASK_F_USR1); + TRACE_LEAVE(H1_EV_STRM_NEW, conn, h1s); return cs; err: @@ -3090,6 +3100,11 @@ static void h1_detach(struct conn_stream *cs) else { if (h1c->conn->owner == sess) h1c->conn->owner = NULL; + + /* mark that the tasklet may lose its context to another thread and + * that the handler needs to check it under the idle conns lock. + */ + HA_ATOMIC_OR(&h1c->wait_event.tasklet->state, TASK_F_USR1); h1c->conn->xprt->subscribe(h1c->conn, h1c->conn->xprt_ctx, SUB_RETRY_RECV, &h1c->wait_event); if (!srv_add_to_idle_list(objt_server(h1c->conn->target), h1c->conn, is_not_first)) { /* The server doesn't want it, let's kill the connection right away */ diff --git a/src/mux_h2.c b/src/mux_h2.c index 4fcd2d6d9..2ac61f7ac 100644 --- a/src/mux_h2.c +++ b/src/mux_h2.c @@ -3773,39 +3773,46 @@ schedule: } /* this is the tasklet referenced in h2c->wait_event.tasklet */ -struct task *h2_io_cb(struct task *t, void *ctx, unsigned int status) +struct task *h2_io_cb(struct task *t, void *ctx, unsigned int state) { struct connection *conn; struct tasklet *tl = (struct tasklet *)t; int conn_in_list; - struct h2c *h2c; + struct h2c *h2c = ctx; int ret = 0; - - HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - if (t->context == NULL) { - /* The connection has been taken over by another thread, - * we're no longer responsible for it, so just free the - * tasklet, and do nothing. + if (state & TASK_F_USR1) { + /* the tasklet was idling on an idle connection, it might have + * been stolen, let's be careful! */ + HA_SPIN_LOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + if (t->context == NULL) { + /* The connection has been taken over by another thread, + * we're no longer responsible for it, so just free the + * tasklet, and do nothing. + */ + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); + tasklet_free(tl); + goto leave; + } + conn = h2c->conn; + TRACE_ENTER(H2_EV_H2C_WAKE, conn); + + conn_in_list = conn->flags & CO_FL_LIST_MASK; + + /* Remove the connection from the list, to be sure nobody attempts + * to use it while we handle the I/O events + */ + if (conn_in_list) + conn_delete_from_tree(&conn->hash_node->node); + HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); - tasklet_free(tl); - goto leave; + } else { + /* we're certain the connection was not in an idle list */ + conn = h2c->conn; + TRACE_ENTER(H2_EV_H2C_WAKE, conn); + conn_in_list = 0; } - h2c = ctx; - conn = h2c->conn; - - TRACE_ENTER(H2_EV_H2C_WAKE, conn); - - conn_in_list = conn->flags & CO_FL_LIST_MASK; - - /* Remove the connection from the list, to be sure nobody attempts - * to use it while we handle the I/O events - */ - if (conn_in_list) - conn_delete_from_tree(&conn->hash_node->node); - - HA_SPIN_UNLOCK(IDLE_CONNS_LOCK, &idle_conns[tid].idle_conns_lock); if (!(h2c->wait_event.events & SUB_RETRY_SEND)) ret = h2_send(h2c); @@ -4096,6 +4103,10 @@ static struct conn_stream *h2_attach(struct connection *conn, struct session *se cs_free(cs); return NULL; } + + /* the connection is not idle anymore, let's mark this */ + HA_ATOMIC_AND(&h2c->wait_event.tasklet->state, ~TASK_F_USR1); + TRACE_LEAVE(H2_EV_H2S_NEW, conn, h2s); return cs; } @@ -4239,6 +4250,10 @@ static void h2_detach(struct conn_stream *cs) h2c->conn->owner = NULL; } + /* mark that the tasklet may lose its context to another thread and + * that the handler needs to check it under the idle conns lock. + */ + HA_ATOMIC_OR(&h2c->wait_event.tasklet->state, TASK_F_USR1); if (!srv_add_to_idle_list(objt_server(h2c->conn->target), h2c->conn, 1)) { /* The server doesn't want it, let's kill the connection right away */ h2c->conn->mux->destroy(h2c);