[PATCH v3 00/13] Clean up to tap errors and epoll dispatch
Getting from an epoll event to the relevant handler function is currently several levels of functions and tests. This series simplifies this to be pretty close to a single switch on a value in the epoll ref dispatching directly to the appropriate handler. Doing this requires some preliminary cleaning up of the handling of errors or disconnects on the tap device. Changes since v2: * Removed a (pre-existing) redundant close() and EPOLL_CTL_DEL in one of the tap reset paths * Fixed incorrect parameter to trace() message Changes since v1: * Give listening TCP sockets their own reference type * Fold "tap reset" series into this one Changes since v2 of tap reset series: * More thorough cleanup of handling error events on the listening Unix socket. Changes since v1 of the tap reset series: * Two extra patches that further clean up the reset path David Gibson (13): tap: Clean up tap reset path tap: Clean up behaviour for errors on listening Unix socket tap: Fold reset handling into tap_handler_pasta() tap: Fold reset handling into tap_handler_passt() epoll: Generalize epoll_ref to cover things other than sockets epoll: Always use epoll_ref for the epoll data variable epoll: Fold sock_handler into general switch on epoll event fd epoll: Split handling of ICMP and ICMPv6 sockets epoll: Tiny cleanup to udp_sock_handler() epoll: Split handling of TCP timerfds into its own handler function epoll: Split handling of listening TCP sockets into their own handler epoll: Split listening Unix domain socket into its own type epoll: Use different epoll types for passt and pasta tap fds icmp.c | 118 ++++++++++++++++++++++++------------------- icmp.h | 9 ++-- passt.c | 90 +++++++++++++++++++-------------- passt.h | 56 ++++++++++++++++----- pasta.c | 8 ++- tap.c | 138 +++++++++++++++++++++++++-------------------------- tap.h | 7 ++- tcp.c | 84 +++++++++++++------------------ tcp.h | 28 +++++++---- tcp_conn.h | 4 +- tcp_splice.c | 8 +-- tcp_splice.h | 2 +- udp.c | 16 +++--- util.c | 27 +++++++--- 14 files changed, 334 insertions(+), 261 deletions(-) -- 2.41.0
In tap_handler() if we get an error on the tap device or socket, we use
tap_sock_init() to re-initialise it. However, what we actually need for
this reset case has remarkably little in common with the case where we're
initialising for the first time:
* Re-initialising the packet pools is unnecessary
* The case of a passed in fd (--fd) isn't relevant
* We don't even call this for pasta mode
* We will never re-call tap_sock_unix_init() because we never clear
fd_tap_listen
In fact the only thing we do in tap_sock_init() relevant to the reset case
is to remove the fd from the epoll and close it... which isn't used in the
first initialisation case.
So make a new tap_sock_reset() function just for this case, and simplify
tap_sock_init() slightly as being used only for the first time case.
Signed-off-by: David Gibson
We call tap_sock_unix_new() to handle a new connection to the qemu socket
if we get an EPOLLIN event on c->fd_tap_listen. If we get any other event
on the fd, we'll fall through to the "tap reset" path. But that won't do
anything relevant to the listening socket, it will just close the already
connected socket. Furthermore, the only other event we're subscribed to
for the listening socket is EPOLLRDHUP, which doesn't apply to a non
connected socket.
Remove EPOLLRDHUP from the subscribed events. We don't need to explicitly
add EPOLLERR, because errors are always reported. There's no obvious case
that would cause an error on a listening socket anyway, and it's not
obvious how we'd recover, treat it as a fatal error if it ever does happen.
Finally, fold all this handling into the tap_sock_unix_new() function,
there's no real reason to split it between there and tap_handler().
Signed-off-by: David Gibson
If tap_handler_pasta() fails, we reset the connection. But in the case of
pasta the "reset" is just a fatal error. Fold the die() calls directly
into tap_handler_pasta() for simplicity.
Signed-off-by: David Gibson
We call tap_sock_reset() if tap_handler_passt() fails, or if we get an
error event on the socket. Fold that logic into tap_handler() passt itself
which simplifies the caller. It also makes it clearer that we had a
redundant EPOLL_CTL_DEL and close() in one of the reset paths, so fix that
too.
Signed-off-by: David Gibson
The epoll_ref type includes fields for the IP protocol of a socket, and the
socket fd. However, we already have a few things in the epoll which aren't
protocol sockets, and we may have more in future. Rename these fields to
an abstract "fd type" and file descriptor for more generality.
Similarly, rather than using existing IP protocol numbers for the type,
introduce our own number space. For now these just correspond to the
supported protocols, but we'll expand on that in future.
Signed-off-by: David Gibson
epoll_ref contains a variety of information useful when handling epoll
events on our sockets, and we place it in the epoll_event data field
returned by epoll. However, for a few other things we use the 'fd' field
in the standard union of types for that data field.
This actually introduces a bug which is vanishingly unlikely to hit in
practice, but very nasty if it ever did: theoretically if we had a very
large file descriptor number for fd_tap or fd_tap_listen it could overflow
into bits that overlap with the 'proto' field in epoll_ref. With some
very bad luck this could mean that we mistakenly think an event on a
regular socket is an event on fd_tap or fd_tap_listen.
More practically, using different (but overlapping) fields of the
epoll_data means we can't unify dispatch for the various different objects
in the epoll. Therefore use the same epoll_ref as the data for the tap
fds and the netns quit fd, adding new fd type values to describe them.
Signed-off-by: David Gibson
When we process events from epoll_wait(), we check for a number of special
cases before calling sock_handler() which then dispatches based on the
protocol type of the socket in the event.
Fold these cases together into a single switch on the fd type recorded in
the epoll data field.
Signed-off-by: David Gibson
We have different epoll type values for ICMP and ICMPv6 sockets, but they
both call the same handler function, icmp_sock_handler(). However that
function does essentially nothing in common for the two cases. So, split
it into icmp_sock_handler() and icmpv6_sock_handler() and dispatch them
separately from the top level.
While we're there remove some parameters that the function was never using
anyway. Also move the test for c->no_icmp into the functions, so that all
the logic specific to ICMP is within the handler, rather than in the top
level dispatch code.
Signed-off-by: David Gibson
Move the test for c->no_udp into the function itself, rather than in the
dispatching switch statement to better localize the UDP specific logic, and
make for greated consistency with other handler functions.
Signed-off-by: David Gibson
tcp_sock_handler() actually handles several different types of fd events.
This includes timerfds that aren't sockets at all. The handling of these
has essentially nothing in common with the other cases. So, give the
TCP timers there own epoll_type value and dispatch directly to their
handler. This also means we can remove the timer field from tcp_epoll_ref,
the information it encoded is now implicit in the epoll_type value.
Signed-off-by: David Gibson
tcp_sock_handler() handles both listening TCP sockets, and connected TCP
sockets, but what it needs to do in those cases has essentially nothing in
common. Therefore, give listening sockets their own epoll_type value and
dispatch directly to their own handler from the top level. Furthermore,
the two handlers need essentially entirely different information from the
reference: we re-(ab)used the index field in the tcp_epoll_ref to indicate
the port for the listening socket, but that's not the same meaning. So,
switch listening sockets to their own reference type which we can lay out
as we please. That lets us remove the listen and outbound fields from the
normal (connected) tcp_epoll_ref, reducing it to just the connection table
index.
Signed-off-by: David Gibson
tap_handler() actually handles events on three different types of object:
the /dev/tap character device (pasta), a connected Unix domain socket
(passt) or a listening Unix domain socket (passt).
The last, in particular, really has no handling in common with the others,
so split it into its own epoll type and directly dispatch to the relevant
handler from the top level.
Signed-off-by: David Gibson
Currently we have a single epoll event type for the "tap" fd, which could
be either a handle on a /dev/net/tun device (pasta) or a connected Unix
socket (passt). However for the two modes we call different handler
functions. Simplify this a little by using different epoll types and
dispatching directly to the correct handler function.
Signed-off-by: David Gibson
On Fri, 11 Aug 2023 15:12:16 +1000
David Gibson
Getting from an epoll event to the relevant handler function is currently several levels of functions and tests. This series simplifies this to be pretty close to a single switch on a value in the epoll ref dispatching directly to the appropriate handler.
Doing this requires some preliminary cleaning up of the handling of errors or disconnects on the tap device.
Changes since v2: * Removed a (pre-existing) redundant close() and EPOLL_CTL_DEL in one of the tap reset paths * Fixed incorrect parameter to trace() message
Changes since v1: * Give listening TCP sockets their own reference type * Fold "tap reset" series into this one
Changes since v2 of tap reset series: * More thorough cleanup of handling error events on the listening Unix socket. Changes since v1 of the tap reset series: * Two extra patches that further clean up the reset path
Applied, thanks! -- Stefano
participants (2)
-
David Gibson
-
Stefano Brivio