On Thu Aug 13, 2026 at 5:01 AM EEST, David Gibson wrote:
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.
Got it. Will clarify this
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")
Haha sorry, typing fast is the culprit here. will take note in the future
+/** + * 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.
Will do
/** 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.
Ok. will do this in the next revision