From a9786b6f043774f66f23d40cc0cb6317b433bbb3 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Thu, 25 Jan 2018 07:22:13 +0100 Subject: [PATCH] MINOR: fd: pass the iocb and owner to fd_insert() fd_insert() is currently called just after setting the owner and iocb, but proceeding like this prevents the operation from being atomic and requires a lock to protect the maxfd computation in another thread from meeting an incompletely initialized FD and computing a wrong maxfd. Fortunately for now all fdtab[].owner are set before calling fd_insert(), and the first lock in fd_insert() enforces a memory barrier so the code is safe. This patch moves the initialization of the owner and iocb to fd_insert() so that the function will be able to properly arrange its operations and remain safe even when modified to become lockless. There's no other change beyond the internal API. --- include/proto/connection.h | 4 +--- include/proto/fd.h | 4 +++- src/dns.c | 4 +--- src/haproxy.c | 4 +--- src/hathreads.c | 5 +---- src/proto_tcp.c | 9 +++------ src/proto_uxst.c | 11 ++++------- src/ssl_sock.c | 7 ++----- 8 files changed, 16 insertions(+), 32 deletions(-) diff --git a/include/proto/connection.h b/include/proto/connection.h index 64117641c..bc8d88484 100644 --- a/include/proto/connection.h +++ b/include/proto/connection.h @@ -111,9 +111,7 @@ static inline void conn_ctrl_init(struct connection *conn) if (!conn_ctrl_ready(conn)) { int fd = conn->handle.fd; - fdtab[fd].owner = conn; - fdtab[fd].iocb = conn_fd_handler; - fd_insert(fd, tid_bit); + fd_insert(fd, conn, conn_fd_handler, tid_bit); /* mark the fd as ready so as not to needlessly poll at the beginning */ fd_may_recv(fd); fd_may_send(fd); diff --git a/include/proto/fd.h b/include/proto/fd.h index 0dd3e84a8..a7e70b7fd 100644 --- a/include/proto/fd.h +++ b/include/proto/fd.h @@ -393,9 +393,11 @@ static inline void fd_update_events(int fd, int evts) } /* Prepares for being polled */ -static inline void fd_insert(int fd, unsigned long thread_mask) +static inline void fd_insert(int fd, void *owner, void (*iocb)(int fd), unsigned long thread_mask) { HA_SPIN_LOCK(FD_LOCK, &fdtab[fd].lock); + fdtab[fd].owner = owner; + fdtab[fd].iocb = iocb; fdtab[fd].ev = 0; fdtab[fd].update_mask &= ~tid_bit; fdtab[fd].linger_risk = 0; diff --git a/src/dns.c b/src/dns.c index a957710ed..280bc155f 100644 --- a/src/dns.c +++ b/src/dns.c @@ -193,9 +193,7 @@ static int dns_connect_namesaver(struct dns_nameserver *ns) /* Add the fd in the fd list and update its parameters */ dgram->t.sock.fd = fd; - fdtab[fd].owner = dgram; - fdtab[fd].iocb = dgram_fd_handler; - fd_insert(fd, (unsigned long)-1); + fd_insert(fd, dgram, dgram_fd_handler, MAX_THREADS_MASK); fd_want_recv(fd); return 0; } diff --git a/src/haproxy.c b/src/haproxy.c index 11849fe22..f1a2fb9d4 100644 --- a/src/haproxy.c +++ b/src/haproxy.c @@ -2347,9 +2347,7 @@ void mworker_pipe_register() return; fcntl(mworker_pipe[0], F_SETFL, O_NONBLOCK); - fdtab[mworker_pipe[0]].owner = mworker_pipe; - fdtab[mworker_pipe[0]].iocb = mworker_pipe_handler; - fd_insert(mworker_pipe[0], MAX_THREADS_MASK); + fd_insert(mworker_pipe[0], mworker_pipe, mworker_pipe_handler, MAX_THREADS_MASK); fd_want_recv(mworker_pipe[0]); } diff --git a/src/hathreads.c b/src/hathreads.c index fea4ffeb0..fd0eb6846 100644 --- a/src/hathreads.c +++ b/src/hathreads.c @@ -48,10 +48,7 @@ int thread_sync_init(unsigned long mask) rfd = threads_sync_pipe[0]; fcntl(rfd, F_SETFL, O_NONBLOCK); - - fdtab[rfd].owner = thread_sync_io_handler; - fdtab[rfd].iocb = thread_sync_io_handler; - fd_insert(rfd, MAX_THREADS_MASK); + fd_insert(rfd, thread_sync_io_handler, thread_sync_io_handler, MAX_THREADS_MASK); all_threads_mask = mask; return 0; diff --git a/src/proto_tcp.c b/src/proto_tcp.c index f4ff698fd..9a3689c71 100644 --- a/src/proto_tcp.c +++ b/src/proto_tcp.c @@ -1105,12 +1105,9 @@ int tcp_bind_listener(struct listener *listener, char *errmsg, int errlen) listener->fd = fd; listener->state = LI_LISTEN; - fdtab[fd].owner = listener; /* reference the listener instead of a task */ - fdtab[fd].iocb = listener->proto->accept; - if (listener->bind_conf->bind_thread[relative_pid-1]) - fd_insert(fd, listener->bind_conf->bind_thread[relative_pid-1]); - else - fd_insert(fd, MAX_THREADS_MASK); + fd_insert(fd, listener, listener->proto->accept, + listener->bind_conf->bind_thread[relative_pid-1] ? + listener->bind_conf->bind_thread[relative_pid-1] : MAX_THREADS_MASK); tcp_return: if (msg && errlen) { diff --git a/src/proto_uxst.c b/src/proto_uxst.c index 754acd2c7..3ab637f20 100644 --- a/src/proto_uxst.c +++ b/src/proto_uxst.c @@ -331,13 +331,10 @@ static int uxst_bind_listener(struct listener *listener, char *errmsg, int errle listener->fd = fd; listener->state = LI_LISTEN; - /* the function for the accept() event */ - fdtab[fd].iocb = listener->proto->accept; - fdtab[fd].owner = listener; /* reference the listener instead of a task */ - if (listener->bind_conf->bind_thread[relative_pid-1]) - fd_insert(fd, listener->bind_conf->bind_thread[relative_pid-1]); - else - fd_insert(fd, MAX_THREADS_MASK); + fd_insert(fd, listener, listener->proto->accept, + listener->bind_conf->bind_thread[relative_pid-1] ? + listener->bind_conf->bind_thread[relative_pid-1] : MAX_THREADS_MASK); + return err; err_rename: diff --git a/src/ssl_sock.c b/src/ssl_sock.c index aecf3ddb7..c2fa45f52 100644 --- a/src/ssl_sock.c +++ b/src/ssl_sock.c @@ -488,7 +488,7 @@ static void ssl_async_fd_free(int fd) */ static void inline ssl_async_process_fds(struct connection *conn, SSL *ssl) { - OSSL_ASYNC_FD add_fd[32], afd; + OSSL_ASYNC_FD add_fd[32]; OSSL_ASYNC_FD del_fd[32]; size_t num_add_fds = 0; size_t num_del_fds = 0; @@ -509,10 +509,7 @@ static void inline ssl_async_process_fds(struct connection *conn, SSL *ssl) /* We add new fds to the fdtab */ for (i=0 ; i < num_add_fds ; i++) { - afd = add_fd[i]; - fdtab[afd].owner = conn; - fdtab[afd].iocb = ssl_async_fd_handler; - fd_insert(afd, tid_bit); + fd_insert(add_fd[i], conn, ssl_async_fd_handler, tid_bit); } num_add_fds = 0;