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