From 3381bf89e39f020855e41ac00489d4f79d2308b7 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Fri, 17 Jan 2020 17:39:35 +0100 Subject: [PATCH] MEDIUM: connection: get rid of CO_FL_CURR_* flags These ones used to serve as a set of switches between CO_FL_SOCK_* and CO_FL_XPRT_*, and now that the SOCK layer is gone, they're always a copy of the last know CO_FL_XPRT_* ones that is resynchronized before I/O events by calling conn_refresh_polling_flags(), and that are pushed back to FDs when detecting changes with conn_xprt_polling_changes(). While these functions are not particularly heavy, what they do is totally redundant by now because the fd_want_*/fd_stop_*() actions already perform test-and-set operations to decide to create an entry or not, so they do the exact same thing that is done by conn_xprt_polling_changes(). As such it is pointless to call that one, and given that the only reason to keep CO_FL_CURR_* is to detect changes there, we can now remove them. Even if this does only save very few cycles, this removes a significant complexity that has been responsible for many bugs in the past, including the last one affecting FreeBSD. All tests look good, and no performance regressions were observed. --- contrib/debug/flags.c | 2 -- include/proto/connection.h | 62 +++++--------------------------------- include/types/connection.h | 17 +++++------ src/connection.c | 25 +++++---------- src/raw_sock.c | 8 +---- src/ssl_sock.c | 3 -- 6 files changed, 22 insertions(+), 95 deletions(-) diff --git a/contrib/debug/flags.c b/contrib/debug/flags.c index 08ca054f9..585dea625 100644 --- a/contrib/debug/flags.c +++ b/contrib/debug/flags.c @@ -139,9 +139,7 @@ void show_conn_flags(unsigned int f) SHOW_FLAG(f, CO_FL_WILL_UPDATE); SHOW_FLAG(f, CO_FL_XPRT_READY); SHOW_FLAG(f, CO_FL_CTRL_READY); - SHOW_FLAG(f, CO_FL_CURR_WR_ENA); SHOW_FLAG(f, CO_FL_XPRT_WR_ENA); - SHOW_FLAG(f, CO_FL_CURR_RD_ENA); SHOW_FLAG(f, CO_FL_XPRT_RD_ENA); if (f) { diff --git a/include/proto/connection.h b/include/proto/connection.h index cd6e62d8d..dd3782273 100644 --- a/include/proto/connection.h +++ b/include/proto/connection.h @@ -161,55 +161,12 @@ static inline void conn_stop_tracking(struct connection *conn) } /* Update polling on connection 's file descriptor depending on its current - * state as reported in the connection's CO_FL_CURR_* flags, reports of EAGAIN - * in CO_FL_WAIT_*, and the upper layer expectations indicated by CO_FL_XPRT_*. - * The connection flags are updated with the new flags at the end of the - * operation. Polling is totally disabled if an error was reported. + * state as reported in the connection's CO_FL_XPRT_* flags. The connection + * flags are updated with the new flags at the end of the operation. Polling + * is totally disabled if an error was reported. */ void conn_update_xprt_polling(struct connection *c); -/* Refresh the connection's polling flags from its file descriptor status. - * This should be called at the beginning of a connection handler. It does - * nothing if CO_FL_WILL_UPDATE is present, indicating that an upper caller - * has already done it. - */ -static inline void conn_refresh_polling_flags(struct connection *conn) -{ - if (conn_ctrl_ready(conn) && !(conn->flags & CO_FL_WILL_UPDATE)) { - unsigned int flags = conn->flags; - - flags &= ~(CO_FL_CURR_RD_ENA | CO_FL_CURR_WR_ENA); - if (fd_recv_active(conn->handle.fd)) - flags |= CO_FL_CURR_RD_ENA; - if (fd_send_active(conn->handle.fd)) - flags |= CO_FL_CURR_WR_ENA; - conn->flags = flags; - } -} - -/* inspects c->flags and returns non-zero if XPRT ENA changes from the CURR ENA - * or if the WAIT flags are set with their respective ENA flags. Additionally, - * non-zero is also returned if an error was reported on the connection. This - * function is used quite often and is inlined. In order to proceed optimally - * with very little code and CPU cycles, the bits are arranged so that a change - * can be detected by a few left shifts, a xor, and a mask. These operations - * detect when W&D are both enabled for either direction, when C&D differ for - * either direction and when Error is set. The trick consists in first keeping - * only the bits we're interested in, since they don't collide when shifted, - * and to perform the AND at the end. In practice, the compiler is able to - * replace the last AND with a TEST in boolean conditions. This results in - * checks that are done in 4-6 cycles and less than 30 bytes. - */ -static inline unsigned int conn_xprt_polling_changes(const struct connection *c) -{ - unsigned int f = c->flags; - f &= CO_FL_XPRT_WR_ENA | CO_FL_XPRT_RD_ENA | CO_FL_CURR_WR_ENA | - CO_FL_CURR_RD_ENA | CO_FL_ERROR; - - f = (f ^ (f << 1)) & (CO_FL_CURR_WR_ENA|CO_FL_CURR_RD_ENA); /* test C ^ D */ - return f & (CO_FL_CURR_WR_ENA | CO_FL_CURR_RD_ENA | CO_FL_ERROR); -} - /* Automatically updates polling on connection depending on the XPRT flags. * It does nothing if CO_FL_WILL_UPDATE is present, indicating that an upper * caller is going to do it again later. @@ -217,8 +174,7 @@ static inline unsigned int conn_xprt_polling_changes(const struct connection *c) static inline void conn_cond_update_xprt_polling(struct connection *c) { if (!(c->flags & CO_FL_WILL_UPDATE)) - if (conn_xprt_polling_changes(c)) - conn_update_xprt_polling(c); + conn_update_xprt_polling(c); } /* Stop all polling on the fd. This might be used when an error is encountered @@ -228,8 +184,7 @@ static inline void conn_cond_update_xprt_polling(struct connection *c) */ static inline void conn_stop_polling(struct connection *c) { - c->flags &= ~(CO_FL_CURR_RD_ENA | CO_FL_CURR_WR_ENA | - CO_FL_XPRT_RD_ENA | CO_FL_XPRT_WR_ENA); + c->flags &= ~(CO_FL_XPRT_RD_ENA | CO_FL_XPRT_WR_ENA); if (!(c->flags & CO_FL_WILL_UPDATE) && conn_ctrl_ready(c)) fd_stop_both(c->handle.fd); } @@ -245,10 +200,8 @@ static inline void conn_cond_update_polling(struct connection *c) { if (unlikely(c->flags & CO_FL_ERROR)) conn_stop_polling(c); - else if (!(c->flags & CO_FL_WILL_UPDATE)) { - if (conn_xprt_polling_changes(c)) - conn_update_xprt_polling(c); - } + else if (!(c->flags & CO_FL_WILL_UPDATE)) + conn_update_xprt_polling(c); } /***** Event manipulation primitives for use by DATA I/O callbacks *****/ @@ -333,7 +286,6 @@ static inline void conn_sock_read0(struct connection *c) static inline void conn_sock_shutw(struct connection *c, int clean) { c->flags |= CO_FL_SOCK_WR_SH; - conn_refresh_polling_flags(c); __conn_xprt_stop_send(c); conn_cond_update_xprt_polling(c); diff --git a/include/types/connection.h b/include/types/connection.h index 3f9fb3cf9..d5852aae1 100644 --- a/include/types/connection.h +++ b/include/types/connection.h @@ -118,17 +118,16 @@ enum cs_shw_mode { CS_SHW_SILENT = 1, /* imminent close, don't notify peer */ }; -/* For each direction, we have a CO_FL_{SOCK,DATA}__ENA flag, which +/* For each direction, we have a CO_FL_XPRT__ENA flag, which * indicates if read or write is desired in that direction for the respective * layers. The current status corresponding to the current layer being used is - * remembered in the CO_FL_CURR__ENA flag. The need to poll (ie receipt of + * remembered in the CO_FL_XPRT__ENA flag. The need to poll (ie receipt of * EAGAIN) is remembered at the file descriptor level so that even when the * activity is stopped and restarted, we still remember whether it was needed * to poll before attempting the I/O. * - * The CO_FL_CURR__ENA flag is set from the FD status in - * conn_refresh_polling_flags(). The FD state is updated according to these - * flags in conn_cond_update_polling(). + * The FD state is updated according to CO_FL_XPRT__ENA in + * conn_cond_update_polling(). */ /* flags for use in connection->flags */ @@ -136,15 +135,13 @@ enum { CO_FL_NONE = 0x00000000, /* Just for initialization purposes */ /* Do not change these values without updating conn_*_poll_changes() ! */ - /* unusued : 0x00000001 */ + /* unused : 0x00000001 */ CO_FL_XPRT_RD_ENA = 0x00000002, /* receiving data is allowed */ - CO_FL_CURR_RD_ENA = 0x00000004, /* receiving is currently allowed */ - /* unused : 0x00000008 */ + /* unused : 0x00000004, 0x00000008 */ /* unused : 0x00000010 */ CO_FL_XPRT_WR_ENA = 0x00000020, /* sending data is desired */ - CO_FL_CURR_WR_ENA = 0x00000040, /* sending is currently desired */ - /* unused : 0x00000080 */ + /* unused : 0x00000040, 0x00000080 */ /* These flags indicate whether the Control and Transport layers are initialized */ CO_FL_CTRL_READY = 0x00000100, /* FD was registered, fd_delete() needed */ diff --git a/src/connection.c b/src/connection.c index 3337538c8..bf55f867f 100644 --- a/src/connection.c +++ b/src/connection.c @@ -55,7 +55,6 @@ void conn_fd_handler(int fd) return; } - conn_refresh_polling_flags(conn); conn->flags |= CO_FL_WILL_UPDATE; flags = conn->flags & ~CO_FL_ERROR; /* ensure to call the wake handler upon error */ @@ -150,10 +149,9 @@ void conn_fd_handler(int fd) } /* Update polling on connection 's file descriptor depending on its current - * state as reported in the connection's CO_FL_CURR_* flags, reports of EAGAIN - * in CO_FL_WAIT_*, and the data layer expectations indicated by CO_FL_XPRT_*. - * The connection flags are updated with the new flags at the end of the - * operation. Polling is totally disabled if an error was reported. + * state as reported in the connection's CO_FL_XPRT_* flags. The connection + * flags are updated with the new flags at the end of the operation. Polling + * is totally disabled if an error was reported. */ void conn_update_xprt_polling(struct connection *c) { @@ -163,25 +161,16 @@ void conn_update_xprt_polling(struct connection *c) return; /* update read status if needed */ - if (unlikely((f & (CO_FL_CURR_RD_ENA|CO_FL_XPRT_RD_ENA)) == CO_FL_XPRT_RD_ENA)) { + if (f & CO_FL_XPRT_RD_ENA) fd_want_recv(c->handle.fd); - f |= CO_FL_CURR_RD_ENA; - } - else if (unlikely((f & (CO_FL_CURR_RD_ENA|CO_FL_XPRT_RD_ENA)) == CO_FL_CURR_RD_ENA)) { + else fd_stop_recv(c->handle.fd); - f &= ~CO_FL_CURR_RD_ENA; - } /* update write status if needed */ - if (unlikely((f & (CO_FL_CURR_WR_ENA|CO_FL_XPRT_WR_ENA)) == CO_FL_XPRT_WR_ENA)) { + if (f & CO_FL_XPRT_WR_ENA) fd_want_send(c->handle.fd); - f |= CO_FL_CURR_WR_ENA; - } - else if (unlikely((f & (CO_FL_CURR_WR_ENA|CO_FL_XPRT_WR_ENA)) == CO_FL_CURR_WR_ENA)) { + else fd_stop_send(c->handle.fd); - f &= ~CO_FL_CURR_WR_ENA; - } - c->flags = f; } /* This is the callback which is set when a connection establishment is pending diff --git a/src/raw_sock.c b/src/raw_sock.c index 63c8f6f48..91a3593d9 100644 --- a/src/raw_sock.c +++ b/src/raw_sock.c @@ -73,7 +73,6 @@ int raw_sock_to_pipe(struct connection *conn, void *xprt_ctx, struct pipe *pipe, return 0; conn->flags &= ~CO_FL_WAIT_ROOM; - conn_refresh_polling_flags(conn); errno = 0; /* Under Linux, if FD_POLL_HUP is set, we have reached the end. @@ -186,7 +185,6 @@ int raw_sock_from_pipe(struct connection *conn, void *xprt_ctx, struct pipe *pip if (!fd_send_ready(conn->handle.fd)) return 0; - conn_refresh_polling_flags(conn); done = 0; while (pipe->data) { ret = splice(pipe->cons, NULL, conn->handle.fd, NULL, pipe->data, @@ -241,7 +239,6 @@ static size_t raw_sock_to_buf(struct connection *conn, void *xprt_ctx, struct bu return 0; conn->flags &= ~CO_FL_WAIT_ROOM; - conn_refresh_polling_flags(conn); errno = 0; if (unlikely(!(fdtab[conn->handle.fd].ev & FD_POLL_IN))) { @@ -354,7 +351,6 @@ static size_t raw_sock_from_buf(struct connection *conn, void *xprt_ctx, const s if (!fd_send_ready(conn->handle.fd)) return 0; - conn_refresh_polling_flags(conn); done = 0; /* send the largest possible block. For this we perform only one call * to send() unless the buffer wraps and we exactly fill the first hunk, @@ -380,10 +376,8 @@ static size_t raw_sock_from_buf(struct connection *conn, void *xprt_ctx, const s /* if the system buffer is full, don't insist */ if (ret < try) break; - if (!count) { + if (!count) conn_xprt_stop_send(conn); - conn_refresh_polling_flags(conn); - } } else if (ret == 0 || errno == EAGAIN || errno == ENOTCONN || errno == EINPROGRESS) { /* nothing written, we need to poll for write first */ diff --git a/src/ssl_sock.c b/src/ssl_sock.c index ff8af2395..f2bce2191 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -6505,8 +6505,6 @@ static size_t ssl_sock_to_buf(struct connection *conn, void *xprt_ctx, struct bu ssize_t ret; size_t try, done = 0; - conn_refresh_polling_flags(conn); - if (!ctx) goto out_error; @@ -6631,7 +6629,6 @@ static size_t ssl_sock_from_buf(struct connection *conn, void *xprt_ctx, const s size_t try, done; done = 0; - conn_refresh_polling_flags(conn); if (!ctx) goto out_error;