]> git.ipfire.org Git - thirdparty/haproxy.git/commitdiff
MEDIUM: fd: merge fdtab[].ev and state for FD_EV_* and FD_POLL_* into state
authorWilly Tarreau <w@1wt.eu>
Tue, 6 Apr 2021 15:23:40 +0000 (17:23 +0200)
committerWilly Tarreau <w@1wt.eu>
Wed, 7 Apr 2021 16:04:39 +0000 (18:04 +0200)
For a long time we've had fdtab[].ev and fdtab[].state which contain two
arbitrary sets of information, one is mostly the configuration plus some
shutdown reports and the other one is the latest polling status report
which also contains some sticky error and shutdown reports.

These ones used to be stored into distinct chars, complicating certain
operations and not even allowing to clearly see concurrent accesses (e.g.
fd_delete_orphan() would set the state to zero while fd_insert() would
only set the event to zero).

This patch creates a single uint with the two sets in it, still delimited
at the byte level for better readability. The original FD_EV_* values
remained at the lowest bit levels as they are also known by their bit
value. The next step will consist in merging the remaining bits into it.

The whole bits are now cleared both in fd_insert() and _fd_delete_orphan()
because after a complete check, it is certain that in both cases these
functions are the only ones touching these areas. Indeed, for
_fd_delete_orphan(), the thread_mask has already been zeroed before a
poller can call fd_update_event() which would touch the state, so it
is certain that _fd_delete_orphan() is alone. Regarding fd_insert(),
only one thread will get an FD at any moment, and it as this FD has
already been released by _fd_delete_orphan() by definition it is certain
that previous users have definitely stopped touching it.

Strictly speaking there's no need for clearing the state again in
fd_insert() but it's cheap and will remove some doubts during some
troubleshooting sessions.

include/haproxy/fd-t.h
include/haproxy/fd.h
src/cli.c
src/dns.c
src/log.c
src/proto_sockpair.c
src/quic_sock.c
src/raw_sock.c
src/sock.c
src/xprt_quic.c

index 27266dce90c543fe4fefa66f30314958396319af..97ae5002b7bff00214376f36fe3654e04eefacca 100644 (file)
@@ -31,23 +31,18 @@ enum {
        DIR_WR=1,
 };
 
-/* Polling status flags returned in fdtab[].ev :
- * FD_POLL_IN remains set as long as some data is pending for read.
- * FD_POLL_OUT remains set as long as the fd accepts to write data.
- * FD_POLL_ERR and FD_POLL_ERR remain set forever (until processed).
- */
-#define FD_POLL_IN     0x00000100
-#define FD_POLL_PRI    0x00000200
-#define FD_POLL_OUT    0x00000400
-#define FD_POLL_ERR    0x00000800
-#define FD_POLL_HUP    0x00001000
-
-#define FD_POLL_UPDT_MASK   (FD_POLL_IN | FD_POLL_PRI | FD_POLL_OUT)
 
-/* FD_EV_* are the values used in fdtab[].state to define the polling states in
- * each direction. Most of them are manipulated using test-and-set operations
- * which require the bit position in the mask, which is given in the _BIT
- * variant.
+/* fdtab[].state is a composite state describing what is known about the FD.
+ * For now, the following information are stored in it:
+ *   - event configuration and status for each direction (R,W) split into
+ *     active, ready, shutdown categories (FD_EV_*). These are known by their
+ *     bit values as well so that test-and-set bit operations are possible.
+ *
+ *   - last known polling status (FD_POLL_*). For ease of troubleshooting,
+ *     avoid visually mixing these ones with the other ones above. 3 of these
+ *     flags are updated on each poll() report (FD_POLL_IN, FD_POLL_OUT,
+ *     FD_POLL_PRI). FD_POLL_HUP and FD_POLL_ERR are "sticky" in that once they
+ *     are reported, they will not be cleared until the FD is closed.
  */
 
 /* bits positions for a few flags */
@@ -61,6 +56,13 @@ enum {
 #define FD_EV_SHUT_W_BIT   6
 #define FD_EV_ERR_RW_BIT   7
 
+#define FD_POLL_IN_BIT     8
+#define FD_POLL_PRI_BIT    9
+#define FD_POLL_OUT_BIT   10
+#define FD_POLL_ERR_BIT   11
+#define FD_POLL_HUP_BIT   12
+
+
 /* and flag values */
 #define FD_EV_ACTIVE_R  (1U << FD_EV_ACTIVE_R_BIT)
 #define FD_EV_ACTIVE_W  (1U << FD_EV_ACTIVE_W_BIT)
@@ -80,6 +82,18 @@ enum {
  */
 #define FD_EV_ERR_RW    (1U << FD_EV_ERR_RW_BIT)
 
+/* mask covering all use cases above */
+#define FD_EV_ANY       (FD_EV_ACTIVE_RW | FD_EV_READY_RW | FD_EV_SHUT_RW | FD_EV_ERR_RW)
+
+/* polling status */
+#define FD_POLL_IN          (1U << FD_POLL_IN_BIT)
+#define FD_POLL_PRI         (1U << FD_POLL_PRI_BIT)
+#define FD_POLL_OUT         (1U << FD_POLL_OUT_BIT)
+#define FD_POLL_ERR         (1U << FD_POLL_ERR_BIT)
+#define FD_POLL_HUP         (1U << FD_POLL_HUP_BIT)
+#define FD_POLL_UPDT_MASK   (FD_POLL_IN | FD_POLL_PRI | FD_POLL_OUT)
+#define FD_POLL_ANY_MASK    (FD_POLL_IN | FD_POLL_PRI | FD_POLL_OUT | FD_POLL_ERR | FD_POLL_HUP)
+
 
 /* This is the value used to mark a file descriptor as dead. This value is
  * negative, this is important so that tests on fd < 0 properly match. It
@@ -127,8 +141,7 @@ struct fdtab {
        struct fdlist_entry update;          /* Entry in the global update list */
        void (*iocb)(int fd);                /* I/O handler */
        void *owner;                         /* the connection or listener associated with this fd, NULL if closed */
-       unsigned char state;                 /* FD state for read and write directions (FD_EV_*) */
-       unsigned int  ev;                    /* event seen in return of poll() : FD_POLL_* */
+       unsigned int state;                  /* FD state for read and write directions (FD_EV_*) + FD_POLL_* */
        unsigned char linger_risk:1;         /* 1 if we must kill lingering before closing */
        unsigned char cloned:1;              /* 1 if a cloned socket, requires EPOLL_CTL_DEL on close */
        unsigned char initialized:1;         /* 1 if init phase was done on this fd (e.g. set non-blocking) */
index 4863c4aa26e736988909d868f1d6b5197042e55a..6d6da691898e460856e52d26f29cded8f82a59d6 100644 (file)
@@ -367,7 +367,7 @@ static inline void fd_update_events(int fd, uint evts)
              ((evts & FD_EV_ERR_RW)  ? FD_POLL_ERR : 0);
 
        /* SHUTW reported while FD was active for writes is an error */
-       if ((fdtab[fd].ev & FD_EV_ACTIVE_W) && (evts & FD_EV_SHUT_W))
+       if ((fdtab[fd].state & FD_EV_ACTIVE_W) && (evts & FD_EV_SHUT_W))
                new_flags |= FD_POLL_ERR;
 
        /* compute the inactive events reported late that must be stopped */
@@ -385,22 +385,22 @@ static inline void fd_update_events(int fd, uint evts)
                must_stop = FD_POLL_OUT;
        }
 
-       old = fdtab[fd].ev;
+       old = fdtab[fd].state;
        new = (old & ~FD_POLL_UPDT_MASK) | new_flags;
 
        if (unlikely(locked)) {
                /* Locked FDs (those with more than 2 threads) are atomically updated */
-               while (unlikely(new != old && !_HA_ATOMIC_CAS(&fdtab[fd].ev, &old, new)))
+               while (unlikely(new != old && !_HA_ATOMIC_CAS(&fdtab[fd].state, &old, new)))
                        new = (old & ~FD_POLL_UPDT_MASK) | new_flags;
        } else {
                if (new != old)
-                       fdtab[fd].ev = new;
+                       fdtab[fd].state = new;
        }
 
-       if (fdtab[fd].ev & (FD_POLL_IN | FD_POLL_HUP | FD_POLL_ERR))
+       if (fdtab[fd].state & (FD_POLL_IN | FD_POLL_HUP | FD_POLL_ERR))
                fd_may_recv(fd);
 
-       if (fdtab[fd].ev & (FD_POLL_OUT | FD_POLL_ERR))
+       if (fdtab[fd].state & (FD_POLL_OUT | FD_POLL_ERR))
                fd_may_send(fd);
 
        if (fdtab[fd].iocb && fd_active(fd)) {
@@ -432,7 +432,7 @@ static inline void fd_insert(int fd, void *owner, void (*iocb)(int fd), unsigned
 
        fdtab[fd].owner = owner;
        fdtab[fd].iocb = iocb;
-       fdtab[fd].ev = 0;
+       fdtab[fd].state = 0;
        fdtab[fd].linger_risk = 0;
        fdtab[fd].cloned = 0;
        fdtab[fd].et_possible = 0;
index 1e4520353b1983cc6baf77918c9713791f6cb9ed..76956e5c2d98cf147c39d5e29589e273fdacd263 100644 (file)
--- a/src/cli.c
+++ b/src/cli.c
@@ -1190,19 +1190,18 @@ static int cli_io_handler_show_fd(struct appctx *appctx)
                        suspicious = 1;
 
                chunk_printf(&trash,
-                            "  %5d : st=0x%02x(R:%c%c W:%c%c) ev=0x%02x(%c%c%c%c%c) [%c%c] tmask=0x%lx umask=0x%lx owner=%p iocb=%p(",
+                            "  %5d : st=0x%04x(R:%c%c W:%c%c %c%c%c%c%c) [%c%c] tmask=0x%lx umask=0x%lx owner=%p iocb=%p(",
                             fd,
                             fdt.state,
                             (fdt.state & FD_EV_READY_R)  ? 'R' : 'r',
                             (fdt.state & FD_EV_ACTIVE_R) ? 'A' : 'a',
                             (fdt.state & FD_EV_READY_W)  ? 'R' : 'r',
                             (fdt.state & FD_EV_ACTIVE_W) ? 'A' : 'a',
-                            fdt.ev >> 8,
-                            (fdt.ev & FD_POLL_HUP) ? 'H' : 'h',
-                            (fdt.ev & FD_POLL_ERR) ? 'E' : 'e',
-                            (fdt.ev & FD_POLL_OUT) ? 'O' : 'o',
-                            (fdt.ev & FD_POLL_PRI) ? 'P' : 'p',
-                            (fdt.ev & FD_POLL_IN)  ? 'I' : 'i',
+                            (fdt.state & FD_POLL_HUP) ? 'H' : 'h',
+                            (fdt.state & FD_POLL_ERR) ? 'E' : 'e',
+                            (fdt.state & FD_POLL_OUT) ? 'O' : 'o',
+                            (fdt.state & FD_POLL_PRI) ? 'P' : 'p',
+                            (fdt.state & FD_POLL_IN)  ? 'I' : 'i',
                             fdt.linger_risk ? 'L' : 'l',
                             fdt.cloned ? 'C' : 'c',
                             fdt.thread_mask, fdt.update_mask,
index 56a19457e2b4ecc255c7e48927101400b4bbc970..19915cc23eb9e5724cfb33f5eb20d92bc660b9fa 100644 (file)
--- a/src/dns.c
+++ b/src/dns.c
@@ -251,7 +251,7 @@ static void dns_resolve_recv(struct dgram_conn *dgram)
 
        /* no need to go further if we can't retrieve the nameserver */
        if ((ns = dgram->owner) == NULL) {
-               _HA_ATOMIC_AND(&fdtab[fd].ev, ~(FD_POLL_HUP|FD_POLL_ERR));
+               _HA_ATOMIC_AND(&fdtab[fd].state, ~(FD_POLL_HUP|FD_POLL_ERR));
                fd_stop_recv(fd);
                return;
        }
@@ -277,7 +277,7 @@ static void dns_resolve_send(struct dgram_conn *dgram)
 
        /* no need to go further if we can't retrieve the nameserver */
        if ((ns = dgram->owner) == NULL) {
-               _HA_ATOMIC_AND(&fdtab[fd].ev, ~(FD_POLL_HUP|FD_POLL_ERR));
+               _HA_ATOMIC_AND(&fdtab[fd].state, ~(FD_POLL_HUP|FD_POLL_ERR));
                fd_stop_send(fd);
                return;
        }
index 2b4b8388d353ec316dcf5678033c58ad05c039a4..21a75a75029c5d6e09f513fc90ac07dfc5e62ed0 100644 (file)
--- a/src/log.c
+++ b/src/log.c
@@ -3679,7 +3679,7 @@ void syslog_fd_handler(int fd)
        if(!l)
                ABORT_NOW();
 
-       if (fdtab[fd].ev & FD_POLL_IN) {
+       if (fdtab[fd].state & FD_POLL_IN) {
 
                if (!fd_recv_ready(fd))
                        return;
index a1e9d2540e0bf67967a7c1efe6e3c8e72e634ba9..48659c769ba52faa210be808cff218e0009209b2 100644 (file)
@@ -495,13 +495,13 @@ struct connection *sockpair_accept_conn(struct listener *l, int *status)
        switch (errno) {
        case EAGAIN:
                ret = CO_AC_DONE; /* nothing more to accept */
-               if (fdtab[l->rx.fd].ev & (FD_POLL_HUP|FD_POLL_ERR)) {
+               if (fdtab[l->rx.fd].state & (FD_POLL_HUP|FD_POLL_ERR)) {
                        /* the listening socket might have been disabled in a shared
                         * process and we're a collateral victim. We'll just pause for
                         * a while in case it comes back. In the mean time, we need to
                         * clear this sticky flag.
                         */
-                       _HA_ATOMIC_AND(&fdtab[l->rx.fd].ev, ~(FD_POLL_HUP|FD_POLL_ERR));
+                       _HA_ATOMIC_AND(&fdtab[l->rx.fd].state, ~(FD_POLL_HUP|FD_POLL_ERR));
                        ret = CO_AC_PAUSE;
                }
                fd_cant_recv(l->rx.fd);
index 56e7ee220bdf0d42dc535205f330d5291b2a24a3..8ded8289038c7ebdf55ee651d728ad45ba597bd4 100644 (file)
@@ -204,7 +204,7 @@ void quic_sock_fd_iocb(int fd)
        if (!l)
                ABORT_NOW();
 
-       if (!(fdtab[fd].ev & FD_POLL_IN) || !fd_recv_ready(fd))
+       if (!(fdtab[fd].state & FD_POLL_IN) || !fd_recv_ready(fd))
                return;
 
        buf = get_trash_chunk();
index 4fbc1a521aa18c4bc90d7b573fdf186a80013f45..6d5c8855111123923480902d6acea684ead466b6 100644 (file)
@@ -71,13 +71,13 @@ int raw_sock_to_pipe(struct connection *conn, void *xprt_ctx, struct pipe *pipe,
         * Since older splice() implementations were buggy and returned
         * EAGAIN on end of read, let's bypass the call to splice() now.
         */
-       if (unlikely(!(fdtab[conn->handle.fd].ev & FD_POLL_IN))) {
+       if (unlikely(!(fdtab[conn->handle.fd].state & FD_POLL_IN))) {
                /* stop here if we reached the end of data */
-               if ((fdtab[conn->handle.fd].ev & (FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_HUP)
+               if ((fdtab[conn->handle.fd].state & (FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_HUP)
                        goto out_read0;
 
                /* report error on POLL_ERR before connection establishment */
-               if ((fdtab[conn->handle.fd].ev & FD_POLL_ERR) && (conn->flags & CO_FL_WAIT_L4_CONN)) {
+               if ((fdtab[conn->handle.fd].state & FD_POLL_ERR) && (conn->flags & CO_FL_WAIT_L4_CONN)) {
                        conn->flags |= CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH;
                        errno = 0; /* let the caller do a getsockopt() if it wants it */
                        goto leave;
@@ -239,13 +239,13 @@ static size_t raw_sock_to_buf(struct connection *conn, void *xprt_ctx, struct bu
        conn->flags &= ~CO_FL_WAIT_ROOM;
        errno = 0;
 
-       if (unlikely(!(fdtab[conn->handle.fd].ev & FD_POLL_IN))) {
+       if (unlikely(!(fdtab[conn->handle.fd].state & FD_POLL_IN))) {
                /* stop here if we reached the end of data */
-               if ((fdtab[conn->handle.fd].ev & (FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_HUP)
+               if ((fdtab[conn->handle.fd].state & (FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_HUP)
                        goto read0;
 
                /* report error on POLL_ERR before connection establishment */
-               if ((fdtab[conn->handle.fd].ev & FD_POLL_ERR) && (conn->flags & CO_FL_WAIT_L4_CONN)) {
+               if ((fdtab[conn->handle.fd].state & FD_POLL_ERR) && (conn->flags & CO_FL_WAIT_L4_CONN)) {
                        conn->flags |= CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH;
                        goto leave;
                }
@@ -282,7 +282,7 @@ static size_t raw_sock_to_buf(struct connection *conn, void *xprt_ctx, struct bu
                                 * to read an unlikely close from the client since we'll
                                 * close first anyway.
                                 */
-                               if (fdtab[conn->handle.fd].ev & FD_POLL_HUP)
+                               if (fdtab[conn->handle.fd].state & FD_POLL_HUP)
                                        goto read0;
 
                                if ((!fdtab[conn->handle.fd].linger_risk) ||
@@ -326,7 +326,7 @@ static size_t raw_sock_to_buf(struct connection *conn, void *xprt_ctx, struct bu
         * of recv()'s return value 0, so we have no way to tell there was
         * an error without checking.
         */
-       if (unlikely(fdtab[conn->handle.fd].ev & FD_POLL_ERR))
+       if (unlikely(fdtab[conn->handle.fd].state & FD_POLL_ERR))
                conn->flags |= CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH;
        goto leave;
 }
index 9ad5b67c9955599d21e2357668b15762b160a31e..19191c82e073e2cc68cfec2e3634f076feba0aa9 100644 (file)
@@ -103,13 +103,13 @@ struct connection *sock_accept_conn(struct listener *l, int *status)
        switch (errno) {
        case EAGAIN:
                ret = CO_AC_DONE; /* nothing more to accept */
-               if (fdtab[l->rx.fd].ev & (FD_POLL_HUP|FD_POLL_ERR)) {
+               if (fdtab[l->rx.fd].state & (FD_POLL_HUP|FD_POLL_ERR)) {
                        /* the listening socket might have been disabled in a shared
                         * process and we're a collateral victim. We'll just pause for
                         * a while in case it comes back. In the mean time, we need to
                         * clear this sticky flag.
                         */
-                       _HA_ATOMIC_AND(&fdtab[l->rx.fd].ev, ~(FD_POLL_HUP|FD_POLL_ERR));
+                       _HA_ATOMIC_AND(&fdtab[l->rx.fd].state, ~(FD_POLL_HUP|FD_POLL_ERR));
                        ret = CO_AC_PAUSE;
                }
                fd_cant_recv(l->rx.fd);
@@ -683,11 +683,11 @@ int sock_conn_check(struct connection *conn)
         */
        if (cur_poller.flags & HAP_POLL_F_ERRHUP) {
                /* modern poller, able to report ERR/HUP */
-               if ((fdtab[fd].ev & (FD_POLL_IN|FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_IN)
+               if ((fdtab[fd].state & (FD_POLL_IN|FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_IN)
                        goto done;
-               if ((fdtab[fd].ev & (FD_POLL_OUT|FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_OUT)
+               if ((fdtab[fd].state & (FD_POLL_OUT|FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_OUT)
                        goto done;
-               if (!(fdtab[fd].ev & (FD_POLL_ERR|FD_POLL_HUP)))
+               if (!(fdtab[fd].state & (FD_POLL_ERR|FD_POLL_HUP)))
                        goto wait;
                /* error present, fall through common error check path */
        }
@@ -832,7 +832,7 @@ int sock_drain(struct connection *conn)
        int fd = conn->handle.fd;
        int len;
 
-       if (fdtab[fd].ev & (FD_POLL_ERR|FD_POLL_HUP))
+       if (fdtab[fd].state & (FD_POLL_ERR|FD_POLL_HUP))
                goto shut;
 
        if (!fd_recv_ready(fd))
index f57506ffd636a1febc9b65d376be8d4507960bb0..862499ccd7fbb7adca07e35bc17872403570f80f 100644 (file)
@@ -3890,13 +3890,13 @@ static size_t quic_conn_to_buf(struct connection *conn, void *xprt_ctx, struct b
        conn->flags &= ~CO_FL_WAIT_ROOM;
        errno = 0;
 
-       if (unlikely(!(fdtab[conn->handle.fd].ev & FD_POLL_IN))) {
+       if (unlikely(!(fdtab[conn->handle.fd].state & FD_POLL_IN))) {
                /* stop here if we reached the end of data */
-               if ((fdtab[conn->handle.fd].ev & (FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_HUP)
+               if ((fdtab[conn->handle.fd].state & (FD_POLL_ERR|FD_POLL_HUP)) == FD_POLL_HUP)
                        goto read0;
 
                /* report error on POLL_ERR before connection establishment */
-               if ((fdtab[conn->handle.fd].ev & FD_POLL_ERR) && (conn->flags & CO_FL_WAIT_L4_CONN)) {
+               if ((fdtab[conn->handle.fd].state & FD_POLL_ERR) && (conn->flags & CO_FL_WAIT_L4_CONN)) {
                        conn->flags |= CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH;
                        goto leave;
                }
@@ -3930,7 +3930,7 @@ static size_t quic_conn_to_buf(struct connection *conn, void *xprt_ctx, struct b
                                 * to read an unlikely close from the client since we'll
                                 * close first anyway.
                                 */
-                               if (fdtab[conn->handle.fd].ev & FD_POLL_HUP)
+                               if (fdtab[conn->handle.fd].state & FD_POLL_HUP)
                                        goto read0;
 
                                if ((!fdtab[conn->handle.fd].linger_risk) ||
@@ -3970,7 +3970,7 @@ static size_t quic_conn_to_buf(struct connection *conn, void *xprt_ctx, struct b
         * of recv()'s return value 0, so we have no way to tell there was
         * an error without checking.
         */
-       if (unlikely(fdtab[conn->handle.fd].ev & FD_POLL_ERR))
+       if (unlikely(fdtab[conn->handle.fd].state & FD_POLL_ERR))
                conn->flags |= CO_FL_ERROR | CO_FL_SOCK_RD_SH | CO_FL_SOCK_WR_SH;
        goto leave;
 }
@@ -4297,7 +4297,7 @@ static size_t quic_conn_handler(int fd, void *ctx, qpkt_read_func *func)
  */
 void quic_fd_handler(int fd)
 {
-       if (fdtab[fd].ev & FD_POLL_IN)
+       if (fdtab[fd].state & FD_POLL_IN)
                quic_conn_handler(fd, fdtab[fd].owner, &qc_lstnr_pkt_rcv);
 }
 
@@ -4306,7 +4306,7 @@ void quic_fd_handler(int fd)
  */
 void quic_conn_fd_handler(int fd)
 {
-       if (fdtab[fd].ev & FD_POLL_IN)
+       if (fdtab[fd].state & FD_POLL_IN)
                quic_conn_handler(fd, fdtab[fd].owner, &qc_srv_pkt_rcv);
 }