On Wed, Aug 12, 2026 at 09:39:33PM +0300, Ammar Yasser wrote: > On Mon Aug 10, 2026 at 12:01 PM EEST, David Gibson wrote: > > 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(). > > In a previous review from you on the original work by Eugenio you said > you don't want tap_send_frames to take a vhost boolean parameter because > its a pasta only value to a function that is used by passt and pasta. > If tap_send_single relies on tap_send_frames (assuming it takes no vhost > bool), and deducing whether to use vhost or no is based on c->vhost then > arp, icmp, ndp.. and other single send protos end up using vhost as > well. which won't work since those protos don't have a concrete memory > region i can make the kernel aware of. Ah, I see. This is the key point the commit message needs to explain: some of the users of tap_send_single() *can't* use the vhost path, because of the shared buffers. > And obvioiusly doing a protocol > check inside tap_send_frames is not super cool either. > > Let me know your opinion on this! > > > > >> - 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. > > To make sure i understand, you're saying that tap_hdr itself should be a > union of vnet_len and virtio_net_mrg_rxbuf ? Yes. > i don't disagree in > principal, (English usage nit: in this context it's "principle", not "principal") > but will need to check that frame length accounting doesn't go > wrong in any area as a consequence of this. Right, doing this will certainly need some updating in places to handle it. I don't think it should be super hard, since it was introduced with a future extension like this in mind. Note that in a sense you can already think of tap_hdr as a union between vnet_len (passt) and an empty structure (pasta). > >> +/** > >> + * 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. > > As per above comment, its a correctness problem. single send protocols > won't work with vhost in the current state, thats why i went the extra > mile to avoid it. let me know if i am misunderstanding anything Right, I missed that constraint. Might be worth putting it in a comment, since it's not necessarily obvious from this point in the code. > > > > >> + 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. > > Noted. will do > > > >> +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. > > So the if statement goes into tap_hdr_iov? ok noted > > > > >> 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. > > The static declarations were removed in the second patch. This addition > is misplaced, will fix. Ok. > But yeah they do need to be non static because of the need to share them > with the kernel in virtio.c Does that mean the *only* reference in virtio.c is just to get their address/size to put into the shared memory table? If that's the case delegating to a helper within udp.c to register them into the table seems like a better approach. > >> #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(). > > Got it. > -- 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