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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 14583 bytes --]

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 <aerosound161@gmail.com>
> ---
>  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 <netinet/ip.h>
>  
>  #include <netinet/tcp.h>
> +#include <linux/virtio_net.h>
>  
>  #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 <time.h>
>  #include <arpa/inet.h>
>  #include <linux/errqueue.h>
> +#include <linux/virtio_net.h>
>  
>  #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 <stdint.h>
>  #include <netinet/in.h>
>  #include <netinet/udp.h>
> +#include <linux/virtio_net.h>
>  
>  #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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]