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

["Ammar Yasser" <aerosound161@gmail.com>]

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 <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.
>> 
>> 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