Re: [RFC v3 7/8] pasta: Implement pasta vhost TX (pasta->guest) prerequisites

On Sun, Aug 02, 2026 at 01:21:54PM +0000, Ammar Yasser wrote:

- tap_send_single was changed to directly call tap_send_frames_passt/pasta instead of relying on tap_send_frames because protocols that use tap_send_single won't use vhost acceleration, only tcp and udp will. The function was also moved lower in the file to by the time its called tap_send_frames_* functions are defined and a forward declaration isn't needed

Sorry, I'm not really following why the vhost change requires tap_send_single() to bypass tap_send_frames().

- extend udp_meta_t to instead of always carrying a tap_hdr, to carry a union of a tap_hdr and a virtio_net header because if vhost is used the will the latter type of header and then build an iov from this virtio net header in udp.c

In fact, the intended role of tap_hdr itself was to be a union of whatever "below L2" headers we might need for all our backends. It was never very obvious because there was just the vnet_len (for the qemu -net socket protocol) and nothing at all (for tuntap via the chardev). With vhost-user, in theory it should have gained a virtio_net_mrg_rxbuf branch, but the vhost-user paths are different enough that we never quite needed it. Here for vhost-kernel, though, we should add the virtio_net header inside tap_hdr, rather than making a union including tap_hdr. tap_hdr_iov() and tap_hdr_update() should be updated to handle that case as well.

- create iov_from_virtio_net_hdr to build an iov from the vnet header - in the case that vhost is requested, set the first frame fragment to be a virtio_net header through calling iov_from_virtio_net_hdr - convert the tcp_payload_tap_hdr array to be an array of virtio_net header. in the case where we aren't using vhost, we will just be using the first 4 bytes of it - add a boolean argumen to tap_send_frames_pasta to determine if vhost will be used

Signed-off-by: Ammar Yasser --- tap.c | 100 +++++++++++++++++++++++++++++++----------------------- tcp_buf.c | 39 +++++++++++++++++---- tcp_buf.h | 4 +-- udp.c | 28 ++++++++++++++- udp.h | 6 +++- 5 files changed, 125 insertions(+), 52 deletions(-)

diff --git a/tap.c b/tap.c index 9d601a2..e177eef 100644 --- a/tap.c +++ b/tap.c @@ -124,45 +124,6 @@ unsigned long tap_l2_max_len(const struct ctx *c) return 0; /* Unreachable, for cppcheck's sake */ }

-/** - * tap_send_single() - Send a single frame - * @c: Execution context - * @data: Packet buffer - * @l2len: Total L2 packet length - */ -void tap_send_single(const struct ctx *c, const void *data, size_t l2len) -{ - uint8_t padded[ETH_ZLEN] = { 0 }; - struct iovec iov[2]; - size_t iovcnt = 0; - uint32_t vnet_len; - - if (l2len < ETH_ZLEN) { - memcpy(padded, data, l2len); - data = padded; - l2len = ETH_ZLEN; - } - - vnet_len = htonl(l2len); - - switch (c->mode) { - case MODE_PASST: - iov[iovcnt] = IOV_OF_LVALUE(vnet_len); - iovcnt++; - /* fall through */ - case MODE_PASTA: - iov[iovcnt].iov_base = (void *)data; - iov[iovcnt].iov_len = l2len; - iovcnt++; - - tap_send_frames(c, iov, iovcnt, 1); - break; - case MODE_VU: - vu_send_single(c, data, l2len); - break; - } -} - /** * tap_push_l2h() - Build an L2 header for an inbound packet * @c: Execution context @@ -415,7 +376,7 @@ void tap_icmp6_send(const struct ctx *c, */ static size_t tap_send_frames_pasta(const struct ctx *c, const struct iovec *iov, - size_t bufs_per_frame, size_t nframes) + size_t bufs_per_frame, size_t nframes, bool vhost) { size_t nbufs = bufs_per_frame * nframes; size_t i; @@ -498,6 +459,59 @@ static size_t tap_send_frames_passt(const struct ctx *c, return i / bufs_per_frame; }

+/** + * tap_send_single() - Send a single frame + * @c: Execution context + * @data: Packet buffer + * @l2len: Total L2 packet length + */ +void tap_send_single(const struct ctx *c, const void *data, size_t l2len) +{ + uint8_t padded[ETH_ZLEN] = { 0 }; + struct iovec iov[2]; + size_t iovcnt = 0; + int m = 0; + uint32_t vnet_len; + + if (l2len < ETH_ZLEN) { + memcpy(padded, data, l2len); + data = padded; + l2len = ETH_ZLEN; + } + + vnet_len = htonl(l2len); + switch (c->mode) { + case MODE_PASST: + /* create an iov for the length */ + iov[iovcnt] = IOV_OF_LVALUE(vnet_len); + iovcnt++; + /* create the data iov */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + m = tap_send_frames_passt(c, iov, iovcnt, 1); + break; + case MODE_PASTA: + /* don't create a length iov in the case of pasta */ + iov[iovcnt].iov_base = (void *)data; + iov[iovcnt].iov_len = l2len; + iovcnt++; + + m = tap_send_frames_pasta(c, iov, iovcnt, 1, false);

You explicitly disable vhost here, even if available. As I've said before, tap_send_single() is a slow path that doesn't really need vhost acceleration. However, there's also not really a reason *not* to use vhost unless it simplifies things. So far this seems to be adding (slightly) complexity to avoid using vhost, and it's not clear to me if there's a simplification elsewhere that makes it worth it.

+ break; + case MODE_VU: + m = vu_send_single(c, data, l2len); + break; + } + + if (m < 1) + debug("tap: failed to send a single frame"); + + pcap_multiple(iov, iovcnt, m, + c->mode == MODE_PASST ? sizeof(uint32_t) : + (c->fd_vhost != -1) ? VNET_HLEN : 0); +} + /** * tap_send_frames() - Send out multiple prepared frames * @c: Execution context @@ -523,7 +537,8 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov,

switch (c->mode) { case MODE_PASTA: - m = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes); + /* use vhost in pasta sending only if the vhost setup was actually successful, indicated by a non-zero fd_vhost */ + m = tap_send_frames_pasta(c, iov, bufs_per_frame, nframes, ((c->fd_vhost != -1))); break; case MODE_PASST: m = tap_send_frames_passt(c, iov, bufs_per_frame, nframes); @@ -539,7 +554,8 @@ size_t tap_send_frames(const struct ctx *c, const struct iovec *iov, nframes - m, nframes);

pcap_multiple(iov, bufs_per_frame, m, - c->mode == MODE_PASST ? sizeof(uint32_t) : 0); + c->mode == MODE_PASST ? sizeof(uint32_t) : + (c->fd_vhost != -1) ? VNET_HLEN : 0);

Rather than this nested ?: expression we should make a helper that gives the necessary length to exclude tap_hdr. tap_hdr_iov() should then use the same helper.

return m; } diff --git a/tcp_buf.c b/tcp_buf.c index 4452337..080e4cb 100644 --- a/tcp_buf.c +++ b/tcp_buf.c @@ -21,6 +21,7 @@ #include

#include +#include

#include "util.h" #include "ip.h" @@ -38,7 +39,7 @@ /* Ethernet header for IPv4 and IPv6 frames */ struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM];

-struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; +struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM];

/* IP headers for IPv4 and IPv6 */ struct iphdr tcp4_payload_ip[TCP_FRAMES_MEM]; @@ -71,6 +72,14 @@ void tcp_update_l2_buf(const unsigned char *eth_d) eth_update_mac(&tcp_eth_hdr[i], eth_d, NULL); }

+static inline struct iovec iov_from_virtio_net_hdr(struct virtio_net_hdr_mrg_rxbuf *hdr) +{ + return (struct iovec){ + .iov_base = hdr, + .iov_len = sizeof(*hdr), + }; +}

As noted, this should become a new branch of tap_hdr_iov()...

/** * tcp_sock_iov_init() - Initialise scatter-gather L2 buffers for IPv4 sockets * @c: Execution context @@ -89,7 +98,14 @@ void tcp_sock_iov_init(const struct ctx *c) for (i = 0; i < TCP_FRAMES_MEM; i++) { struct iovec *iov = tcp_l2_iov[i];

- iov[TCP_IOV_TAP] = tap_hdr_iov(c, &tcp_payload_tap_hdr[i]); + /* If we are using pasta with vhost acceleration, the first entry in the tcp buffers + * should point to a virtio_net header, otherwise a tap header + */ + if ((c->fd_vhost != -1)) + iov[TCP_IOV_TAP] = iov_from_virtio_net_hdr(&tcp_payload_tap_hdr[i]); + else + iov[TCP_IOV_TAP] = tap_hdr_iov(c, (struct tap_hdr *)&tcp_payload_tap_hdr[i]);

..absorbing this if.

iov[TCP_IOV_ETH].iov_len = sizeof(struct ethhdr); iov[TCP_IOV_PAYLOAD].iov_base = &tcp_payload[i]; iov[TCP_IOV_ETH_PAD].iov_base = eth_pad; @@ -135,8 +151,7 @@ void tcp_payload_flush(const struct ctx *c, const struct timespec *now) { size_t m;

- m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, - tcp_payload_used); + m = tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, tcp_payload_used); if (m != tcp_payload_used) { tcp_revert_seq(c, &tcp_frame_conns[m], &tcp_l2_iov[m], tcp_payload_used - m, now); @@ -177,7 +192,7 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c, { struct iov_tail tail = IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); struct tcphdr th_storage, *th = IOV_REMOVE_HEADER(&tail, th_storage); - struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; + const struct flowside *tapside = TAPFLOW(conn); const struct in_addr *a4 = inany_v4(&tapside->oaddr); struct ethhdr *eh = iov[TCP_IOV_ETH].iov_base; @@ -192,7 +207,19 @@ static void tcp_l2_buf_fill_headers(const struct ctx *c,

l2len = tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &tail, iov_tail_size(&tail), csum_flags, seq); - tap_hdr_update(taph, l2len); + + /* when in pasta mode, the length of the tap header iov is zero, so this + * l2len write doesn't do anything. But this function gets called from both + * pasta and passt. Make the l2len write in case we are in passt mode, denoted by + * fd_vhost being -1 since this field won't get initialized at all in passt. + */ + if (c->mode == MODE_PASST) { + iov[TCP_IOV_TAP].iov_len = sizeof(struct tap_hdr); + struct tap_hdr *taph = iov[TCP_IOV_TAP].iov_base; + tap_hdr_update(taph, l2len); + } else if (c->fd_vhost == -1) { /* pasta mode but without vhost */ + iov[TCP_IOV_TAP].iov_len = 0; + }

Similarly this if should go inside tap_hdr_update().

}

/** diff --git a/tcp_buf.h b/tcp_buf.h index c749038..da48cd0 100644 --- a/tcp_buf.h +++ b/tcp_buf.h @@ -9,7 +9,7 @@ #include "tcp_conn.h" #include "tcp_internal.h"

-void tcp_sock_iov_init(); +void tcp_sock_iov_init(const struct ctx *c); void tcp_payload_flush(const struct ctx *c, const struct timespec *now); int tcp_buf_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn, uint32_t already_sent, const struct timespec *now); @@ -20,7 +20,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags, #define TCP_FRAMES \ (c->mode == MODE_PASTA ? 1 : TCP_FRAMES_MEM)

-extern struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; +extern struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_MEM]; extern struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM]; extern struct tcp_payload_t tcp_payload[TCP_FRAMES_MEM];

diff --git a/udp.c b/udp.c index d05ee66..5f90dce 100644 --- a/udp.c +++ b/udp.c @@ -103,6 +103,7 @@ #include #include #include +#include

#include "checksum.h" #include "util.h" @@ -119,6 +120,18 @@ #include "udp_vu.h" #include "epoll_ctl.h"

+/* UDP header and data for inbound messages */ +struct udp_payload_t udp_payload[UDP_MAX_FRAMES]; + +/* Ethernet headers for IPv4 and IPv6 frames */ +struct ethhdr udp_eth_hdr[UDP_MAX_FRAMES]; + +/* IOVs and msghdr arrays for receiving datagrams from sockets */ +struct iovec udp_iov_recv [UDP_MAX_FRAMES]; +struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; + +/* Pre-cooked headers for UDP packets */ +struct udp_meta_t udp_meta[UDP_MAX_FRAMES];

I'm slightly confused. I see these added, but I don't see the existing static declarations removed. It's also not clear to me why these need to become non-static.

#define UDP_TIMEOUT "/proc/sys/net/netfilter/nf_conntrack_udp_timeout" #define UDP_TIMEOUT_STREAM \ @@ -216,7 +229,20 @@ static void udp_iov_init_one(const struct ctx *c, size_t i) *siov = IOV_OF_LVALUE(payload->data);

tiov[UDP_IOV_ETH] = IOV_OF_LVALUE(udp_eth_hdr[i]); - tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph); + /* if the vhost tap fd is initialized, this is a sign for us that + * we will be using virtio transport. make the iov that is supposed + * to point to a tap header point instead to a virtio_net_mrg_rxbuf + */ + if (c->fd_vhost != -1) { + struct iovec vnet_iov = { + .iov_base = (void *)(&meta->vnet_hdr), + .iov_len = sizeof(meta->vnet_hdr) + }; + tiov[UDP_IOV_TAP] = vnet_iov; + } else { + tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &meta->taph); + }

Again, this logic should go inside tap_hdr_iov().

+ tiov[UDP_IOV_PAYLOAD].iov_base = payload; tiov[UDP_IOV_ETH_PAD].iov_base = eth_pad;

diff --git a/udp.h b/udp.h index b7a367c..3464b74 100644 --- a/udp.h +++ b/udp.h @@ -9,6 +9,7 @@ #include #include #include +#include

#include "tap_hdr.h" #include "fwd.h" @@ -49,7 +50,10 @@ extern struct mmsghdr udp_mh_recv [UDP_MAX_FRAMES]; struct udp_meta_t { struct ipv6hdr ip6h; struct iphdr ip4h; - struct tap_hdr taph; + union { + struct virtio_net_hdr_mrg_rxbuf vnet_hdr; + struct tap_hdr taph; +}; #ifdef __AVX2__ } __attribute__((aligned(32))); #else -- 2.34.1

-- David Gibson (he or they) | 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