- dev - passt

[PATCH] tap: Use single counter for iov elements in tap_send_frames_pasta()
by Stefano Brivio 16 Feb '23

16 Feb '23

David points out that using multiple counters to go over the iov array, namely 'i' and 'iov', makes mistakes easier. We can't just use 'iov', unless we reserve an element with zero iov_len at the end, which isn't really justified. Simply use 'i' to iterate over the array. Link: https://archives.passt.top/passt-dev/Y+mfenvLn3VJ7Dg5@yekko/ Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tap.c b/tap.c index 02da84d..e4c5b5b 100644 --- a/tap.c +++ b/tap.c @@ -316,13 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c, { size_t i; - for (i = 0; i < n; i++, iov++) { - if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) { + for (i = 0; i < n; i++) { + if (write(c->fd_tap, (char *)iov[i].iov_base, + iov[i].iov_len) < 0) { debug("tap write: %s", strerror(errno)); if (errno != EAGAIN && errno != EWOULDBLOCK) tap_handler(c, c->fd_tap, EPOLLERR, NULL); i--; - iov--; } } } -- 2.35.1

2 1

[PATCH v3 0/9] error logging fixes
by Laine Stump 15 Feb '23

15 Feb '23

There are two topics covered here: 1) If a logFile is set, passt's behavior has been to send all error messages there, and *not* to stderr. This makes it difficult for another program that is exec'ing passt (and setting it to log to a file) to report useful error messages when passt fails - everything after the point that the logfile is opened is sent only to the logfile. The first patch makes a simple change to the logging functions that uses the value of the system logmask to decide if it should force writing messages to stderr even when a logfile has been specified. Change from "v2": I'm using Stefano's suggestion of "abusing" logmask, rather than adding a static bool to keep track. 2) All the rest of the patches eliminate use of the blanket usage() function when a commandline error is encountered (and add in specific/details error messages when previously usage() was all that was logged), and replace calls to err() followed by exit() with a single call to the new function die(). Change from "v2": I changed the name of the "log and exit" function from "errexit()" to "die()" at the suggestion of Dave Gibson (Stefano concurred). Although it seems a bit more violent, it does make moot many/most of Stefano's nits about needing to split lines to eliminate > 80 characters (I did address all the rest of the things he pointed out, though) NB: Yes, this says it is v3, and the previous version I sent was v2, and there *was no v1* - this is because I didn't realize that git-publish is automatically incrementing the version number every time I run it, and I had done a test-drive sending the patches to my personal address prior to sending them to the list - *that* was v1. Laine Stump (9): log to stderr until process is daemonized, even if a logfile is set add die() to log an error message and exit with a single call eliminate most calls to usage() in conf() make conf_ports() exit immediately after logging error make conf_pasta_ns() exit immediately after logging error make conf_ugid() exit immediately after logging error make conf_netns_opt() exit immediately after logging error log a detailed error (not usage()) when there are extra non-option arguments convert all remaining err() followed by exit() to die() conf.c | 468 ++++++++++++++++++++-------------------------------- isolation.c | 67 +++----- log.c | 24 +-- log.h | 1 + netlink.c | 3 +- passt.c | 29 ++-- pasta.c | 20 +-- tap.c | 30 ++-- 8 files changed, 244 insertions(+), 398 deletions(-) -- 2.39.1

1 9

Problem with IPv4 performance tests
by David Gibson 15 Feb '23

15 Feb '23

Hi, I've been running the tests for the first time since getting back from holiday. I seem to be getting a consistent failure on the first batch of IPv4 throughput tests, although the IPv6 tests before it worked fine: === perf/passt_tcp > passt: throughput and latency Throughput in Gbps, latency in µs, one thread at 4.8 GHz, 8 streams MTU: | 256B | 576B | 1280B | 1500B | 9000B | 65520B | |--------|--------|--------|--------|--------|--------| TCP throughput over IPv6: guest to host | - | - | 6.5 | 7.6 | 20.7 | 28.1 | TCP RR latency over IPv6: guest to host | - | - | - | - | - | 74 | TCP CRR latency over IPv6: guest to host | - | - | - | - | - | 227 | |--------|--------|--------|--------|--------|--------| TCP throughput over IPv4: guest to host | 0 | 0 | 0 | 0 | 0 | 0 | TCP RR latency over IPv4: guest to host | - | - | - | - | - | I haven't seen a failure like this before, has anyone else seen something like this? I'm not getting any messages from the iperf client at all - like it has no connectivity whatsoever, but the non-perf tests are all passing. Haven't done much troubleshooting yet. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson

2 2

[PATCH] Makefile: Explict int type in FALLOC_FL_COLLAPSE_RANGE probe
by Stefano Brivio 15 Feb '23

15 Feb '23

From: Florian Weimer <fweimer(a)redhat.com> Future compilers will not support implicit ints by default, causing the probe to always fail. Link: https://bugs.passt.top/show_bug.cgi?id=42 Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- I'm just posting Florian's patch here for ease of review -- I just applied this by the way. Thanks Florian for the report and for the patch! Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index dd222a8..080c748 100644 --- a/Makefile +++ b/Makefile @@ -101,7 +101,7 @@ ifeq ($(shell :|$(CC) -fstack-protector-strong -S -xc - -o - >/dev/null 2>&1; ec FLAGS += -fstack-protector-strong endif -C := \#define _GNU_SOURCE\n\#include <fcntl.h>\nx = FALLOC_FL_COLLAPSE_RANGE; +C := \#define _GNU_SOURCE\n\#include <fcntl.h>\nint x = FALLOC_FL_COLLAPSE_RANGE; ifeq ($(shell printf "$(C)" | $(CC) -S -xc - -o - >/dev/null 2>&1; echo $$?),0) EXTRA_SYSCALLS += fallocate endif -- 2.35.1

2 1

[PATCH v2 0/5] Cleanups to tcp socket pool handling
by David Gibson 14 Feb '23

14 Feb '23

This is a bit of a diversion from what I'm notionally working on at the moment. While thinking about what we'd need to use the IP_TRANSPARENT socket option to broaden the cases where we can "splice", I noticed some inelegancies in how we handle the pool of pre-opened sockets in the TCP code, and well, here we are. This makes a number of cleanups to the handling of these pools. Most notably, tcp_splice_connect() now has simpler semantics: it now always runs in the init namespace, and is always given a pre-opened socket (which could be in the guest ns). Changes since v1: * Rebased * Improved wording of some commit messages David Gibson (5): tcp: Make a helper to refill each socket pool tcp: Split init and ns cases for tcp_sock_refill() tcp: Move socket pool declarations around tcp: Split pool lookup from creating new sockets in tcp_conn_new_sock() tcp: Improve handling of fallback if socket pool is empty on new splice tcp.c | 138 ++++++++++++++++++------------------------------- tcp.h | 2 - tcp_conn.h | 12 ++++- tcp_splice.c | 141 ++++++++++++++++++++++++++------------------------- 4 files changed, 132 insertions(+), 161 deletions(-) -- 2.39.1

2 6

[PATCH 0/2] Update locations for some downloaded images
by David Gibson 14 Feb '23

14 Feb '23

We're not running the distro tests by default at the moment, but the Makefile still downloads and prepares the images for them. I haven't actually done the downloads for some time, because I kept the images around, but I finally happened to do a full clean and needed to re-download. At this point I discovered some of the things we're trying to download have moved, so some URLs need updating. David Gibson (2): test: Update location for Debian ppc64 images test: Fedora 32-35 have moved to the archives test/Makefile | 13 ++++++++----- test/distro/debian | 2 +- 2 files changed, 9 insertions(+), 6 deletions(-) -- 2.39.1

2 3

Improved handling of changing DNS resolvers
by Noah Gold 14 Feb '23

14 Feb '23

Hi folks, libslirp and Passt have different approaches to sharing DNS resolvers with the guest system, each with their own benefits & drawbacks. On the libslirp project, we're discussing [1] how to support DNS failover. Passt already has support for this, but there is a drawback to its solution which prevents us from taking a similar approach: the resolvers are read exactly once, so if the host changes networks at runtime, the guest will not receive the updated resolvers and thus its connectivity will break. libslirp's current approach is to DNAT a single address exposed to the guest to one of the resolvers configured on the host. The problem here is that if that one resolver goes down, the guest can't resolve DNS names. We're considering changing so that instead of a single address, we expose a set of MAXNS addresses, and DNAT those 1:1 to the DNS resolvers registered with the host. Because the DNAT table lives on the host side, we can refresh the guest's resolvers whenever the host's resolvers change, but without the need to expire a DHCP lease (even with short leases, the guest will still lose connectivity for a time). Does this sound like an approach Passt would be open to adopting as well? Thanks. - Noah [1] https://gitlab.freedesktop.org/slirp/libslirp/-/issues/26

3 7

[libvirt PATCH] qemu: allow passt to self-daemonize
by Laine Stump 14 Feb '23

14 Feb '23

I initially had the passt process being started in an identical fashion to the slirp-helper - libvirt was daemonizing the new process and recording its pid in a pidfile. The problem with this is that, since it is daemonized immediately, any startup error in passt happens after the daemonization, and thus isn't seen by libvirt - libvirt believes that the process has started successfully and continues on its merry way. The result was that sometimes a guest would be started, but there would be no passt process for qemu to use for network traffic. Instead, we should be starting passt in the same manner we start dnsmasq - we just exec it as normal (along with a request that passt create the pidfile, which is just another option on the passt commandline) and wait for the child process to exit; passt then has a chance to parse its commandline and complete all the setup prior to daemonizing itself; if it encounters an error and exits with a non-0 code, libvirt will see the code and know about the failure. We can then grab the output from stderr, log that so the "user" has some idea of what went wrong, and then fail the guest startup. Signed-off-by: Laine Stump <laine(a)redhat.com> --- src/qemu/qemu_passt.c | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/qemu/qemu_passt.c b/src/qemu/qemu_passt.c index 0f09bf3db8..f640a69c00 100644 --- a/src/qemu/qemu_passt.c +++ b/src/qemu/qemu_passt.c @@ -141,24 +141,23 @@ qemuPasstStart(virDomainObj *vm, g_autofree char *passtSocketName = qemuPasstCreateSocketPath(vm, net); g_autoptr(virCommand) cmd = NULL; g_autofree char *pidfile = qemuPasstCreatePidFilename(vm, net); + g_autofree char *errbuf = NULL; char macaddr[VIR_MAC_STRING_BUFLEN]; size_t i; pid_t pid = (pid_t) -1; int exitstatus = 0; int cmdret = 0; - VIR_AUTOCLOSE errfd = -1; cmd = virCommandNew(PASST); virCommandClearCaps(cmd); - virCommandSetPidFile(cmd, pidfile); - virCommandSetErrorFD(cmd, &errfd); - virCommandDaemonize(cmd); + virCommandSetErrorBuffer(cmd, &errbuf); virCommandAddArgList(cmd, "--one-off", "--socket", passtSocketName, "--mac-addr", virMacAddrFormat(&net->mac, macaddr), + "--pid", pidfile, NULL); if (net->mtu) { @@ -264,7 +263,7 @@ qemuPasstStart(virDomainObj *vm, if (cmdret < 0 || exitstatus != 0) { virReportError(VIR_ERR_INTERNAL_ERROR, - _("Could not start 'passt'. exitstatus: %d"), exitstatus); + _("Could not start 'passt': %s"), errbuf); goto error; } -- 2.39.1

6 14

[PATCH v2 0/9] error logging fixes
by Laine Stump 14 Feb '23

14 Feb '23

There are two topics covered here: 1) If a logFile is set, passt's behavior has been to send all error messages there, and *not* to stderr. This makes it difficult for another program that is exec'ing passt (and setting it to log to a file) to report useful error messages when passt fails. The first patch makes a simple change to the logging functions that checks a global bool that is set true after all initialization is completed. 2) All the rest of the patches eliminate use of the blanket usage() function when a commandline error is encountered (and add in specific/details error messages when previously usage() was all that was logged). Laine Stump (9): log to stderr until process is daemonized, even if a logfile is set add errexit() to log an error message and exit with a single call eliminate most calls to usage() in conf() make conf_ports() exit immediately after logging error make conf_pasta_ns() exit immediately after logging error make conf_ugid() exit immediately after logging error make conf_netns_opt() exit immediately after logging error log a detailed error (not usage()) when there are extra non-option arguments convert all remaining err() followed by exit() to errexit() conf.c | 471 ++++++++++++++++++++-------------------------------- isolation.c | 78 ++++----- log.c | 33 ++-- log.h | 2 + netlink.c | 3 +- passt.c | 18 +- pasta.c | 21 +-- tap.c | 30 ++-- 8 files changed, 258 insertions(+), 398 deletions(-) -- 2.39.1

4 20

[PATCH] tap: Send frames after the first one in tap_send_frames_pasta()
by Stefano Brivio 14 Feb '23

14 Feb '23

...instead of repeatedly sending out the first one in iov. Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- I just applied this, to unblock a series by David which was pending for way too long. The commit reference in Fixes: refers to a commit from said series which I'm pushing out together with this patch. Posting anyway for reviews. tap.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tap.c b/tap.c index af9bc15..716d887 100644 --- a/tap.c +++ b/tap.c @@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c, { size_t i; - for (i = 0; i < n; i++) { + for (i = 0; i < n; i++, iov++) { if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) { debug("tap write: %s", strerror(errno)); if (errno != EAGAIN && errno != EWOULDBLOCK) tap_handler(c, c->fd_tap, EPOLLERR, NULL); i--; + iov--; } } } -- 2.35.1

2 3