From 4684f603446b1f5da20f242006eedb4fb9eced95 Mon Sep 17 00:00:00 2001 From: David Gibson Date: Fri, 26 Jul 2024 17:20:29 +1000 Subject: [PATCH] tap: Don't use EPOLLET on Qemu sockets Currently we set EPOLLET (edge trigger) on the epoll flags for the connected Qemu Unix socket. It's not clear that there's a reason for doing this: for TCP sockets we need to use EPOLLET, because we leave data in the socket buffers for our flow control handling. That consideration doesn't apply to the way we handle the qemu socket however. Furthermore, using EPOLLET causes additional complications: 1) We don't set EPOLLET when opening /dev/net/tun for pasta mode, however we *do* set it when using pasta mode with --fd. This inconsistency doesn't seem to have broken anything, but it's odd. 2) EPOLLET requires that tap_handler_passt() loop until all data available is read (otherwise we may have data in the buffer but never get an event causing us to read it). We do that with a rather ugly goto. Worse, our condition for that goto appears to be incorrect. We'll only loop if rem is non-zero, which will only happen if we perform a blocking recv() for a partially received frame. We'll only perform that second recv() if the original recv() resulted in a partially read frame. As far as I can tell the original recv() could end on a frame boundary (never triggering the second recv()) even if there is additional data in the socket buffer. In that circumstance we wouldn't goto redo and could leave unprocessed frames in the qemu socket buffer indefinitely. This doesn't seem to have caused any problems in practice, but since there's no obvious reason to use EPOLLET here anyway, we might as well get rid of it. Signed-off-by: David Gibson Signed-off-by: Stefano Brivio --- tap.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/tap.c b/tap.c index 18aad9a..a47f9d3 100644 --- a/tap.c +++ b/tap.c @@ -989,7 +989,7 @@ static void tap_sock_reset(struct ctx *c) void tap_handler_passt(struct ctx *c, uint32_t events, const struct timespec *now) { - ssize_t n, rem; + ssize_t n; char *p; if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) { @@ -997,9 +997,7 @@ void tap_handler_passt(struct ctx *c, uint32_t events, return; } -redo: p = pkt_buf; - rem = 0; tap_flush_pools(); @@ -1028,7 +1026,7 @@ redo: * needs to be blocking. */ if (l2len > n) { - rem = recv(c->fd_tap, p + n, l2len - n, 0); + ssize_t rem = recv(c->fd_tap, p + n, l2len - n, 0); if ((n += rem) != l2len) return; } @@ -1040,10 +1038,6 @@ redo: } tap_handler(c, now); - - /* We can't use EPOLLET otherwise. */ - if (rem) - goto redo; } /** @@ -1228,7 +1222,7 @@ void tap_listen_handler(struct ctx *c, uint32_t events) trace("tap: failed to set SO_SNDBUF to %i", v); ref.fd = c->fd_tap; - ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; + ev.events = EPOLLIN | EPOLLRDHUP; ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); } @@ -1317,7 +1311,7 @@ void tap_sock_init(struct ctx *c) else ref.type = EPOLL_TYPE_TAP_PASTA; - ev.events = EPOLLIN | EPOLLET | EPOLLRDHUP; + ev.events = EPOLLIN | EPOLLRDHUP; ev.data.u64 = ref.u64; epoll_ctl(c->epollfd, EPOLL_CTL_ADD, c->fd_tap, &ev); return;