From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: passt.top; dkim=pass (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.a=rsa-sha256 header.s=20251104 header.b=NQNFF53F; dkim-atps=neutral Received: from mail-wr1-x436.google.com (mail-wr1-x436.google.com [IPv6:2a00:1450:4864:20::436]) by passt.top (Postfix) with ESMTPS id C47D85A026E for ; Wed, 12 Aug 2026 20:39:36 +0200 (CEST) Received: by mail-wr1-x436.google.com with SMTP id ffacd0b85a97d-47f96c5b722so679908f8f.0 for ; Wed, 12 Aug 2026 11:39:36 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786559976; x=1787164776; darn=passt.top; h=in-reply-to:references:subject:cc:to:from:message-id:date :content-type:content-transfer-encoding:mime-version:from:to:cc :subject:date:message-id:reply-to:content-type; bh=Cw5tVAICtBMsuAJFKttfLlCmO2ogI8bSwlzniCc44Jo=; b=NQNFF53FBV1DTaDIYGbzL371hBmc7aDVHZk3DQn4DsebyJAu0TYN6pysvwMYvXexcy 5h99FliHOraSmvlFRQ2wUY9YbtudDXF+z0tJAluWacb9jeF9ggovl/bbTw1NWXkB16K6 huWYXVN3VdgO4gR2ZZZotN6/NDZkNCyZwXnHfj62rGYVhnnxO7BrO5pDSmCj+qMAoimq hfGAfn+0i4cAiVZ9AODfFDZe4td0M+iIRQ8n5uDmelWKhBwAOWtVt+v/pgTEqECu9Oir 1pq6NVi2Z9szVMWpl4e/y/5d99SFvZSKiTf6SIjJErv7o5Cqyt/SWC85eFUernWf76iy RAeg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786559976; x=1787164776; h=in-reply-to:references:subject:cc:to:from:message-id:date :content-type:content-transfer-encoding:mime-version:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to :content-type; bh=Cw5tVAICtBMsuAJFKttfLlCmO2ogI8bSwlzniCc44Jo=; b=sNtEfbTJAtPZxbvihORatcCHyLQz/gY64RLWumLLhz9nFMFrkyFdBYv2dOjZM5hRwE R4DeU8nTiVihNzd3T++E4jLEHLXM4s+9Lo5OYM1/GsUz9ozxWJMrZgodkHJkqQsocA5r mdw457r7z8iiJsZ5JXQ0JTEINSOlU3j3b16VA3AHonEcfp2x6RH64jDhFjJdRjTPVPIJ E52uRbc3w+uXOYKgBbwlzkvnCL6tKYy/cyIS/J0RA9lhtTsltZ4UwaUDu1zti55qI3Md aVP55H3yVG18m/IrBNnCfsaQvmsbMnIF+B7+c1HRxgdHU8aN9VQAR+UoLAWTBzyD4DF2 CcLg== X-Gm-Message-State: AOJu0YyCsbBqJyntOXFwifND8ZpAteMjakmBxXQAicPOkl/2vF1xXXI8 smw4VybTJBAmf7p3HE3EtR3NNWEaKEUmqYO1p5hdx5u86PtQ6tIP6193UPl5qPxF X-Gm-Gg: AR+sD11I1E6wR3sZ0SAaQd1TKBr/ftCIbCNsoQ5ab9CGMJk5hFTUopdpOrr9ZIu9Qtt VUKnD6N2pGqAbUPEmiQAij/EK8hLMX71j+6f4v3TwwhHZ1JrUkmSHPK26jZV2YDX+pmghphpEn5 s8MamfpPFd2yRzTKHqY0ddpM2ESY2oI8eyKmN2yFqxMd1EuG2rqqH6WY2pKTiSeVllW/jsyZbML 3JjedZBJQp6u4zLEmjndNu8AiRJOhSF5Esty8d3Msn5qHGxXWTx0ct+bHbsDb54KAmh5fwPeoR9 9lc2J3Czvex1rXr8OpIRDbEAaSW4yMehTQqu7CHHDasbvRkFp7QUEUksOl6YXoX1Z27gAZ2lAlP xlRsaflpH0iSN0d4j+jKrCF5tOo7tbuo64Ur5dORQquZCb+pI94ptRaoFGKEnN2fS4R/PPVVNew eZdQD0V8Y91wkSgHcw/ZEFiFlMzj9cWZ1Sm9yc24w44sYGyY16FaCdXfw4QSuMuzk0dNqo6xZWT uw8skIXZqmw6hHhJTCuIi6HwRIbix0= X-Received: by 2002:a05:6000:4715:b0:47f:93e2:a07b with SMTP id ffacd0b85a97d-4815a00d89cmr307940f8f.30.1786559976033; Wed, 12 Aug 2026 11:39:36 -0700 (PDT) Received: from localhost ([196.157.64.51]) by smtp.gmail.com with ESMTPSA id ffacd0b85a97d-48150d4e400sm9976680f8f.21.2026.08.12.11.39.34 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Aug 2026 11:39:35 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 12 Aug 2026 21:39:33 +0300 Message-Id: From: "Ammar Yasser" To: "David Gibson" , "Ammar Yasser" Subject: Re: [RFC v3 7/8] pasta: Implement pasta vhost TX (pasta->guest) prerequisites X-Mailer: aerc 0.21.0 References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-8-aerosound161@gmail.com> In-Reply-To: Message-ID-Hash: PY2IEN3JUJSEEP5HIX42D76RBYMFIPJU X-Message-ID-Hash: PY2IEN3JUJSEEP5HIX42D76RBYMFIPJU X-MailFrom: aerosound161@gmail.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, eperezma@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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. 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 ? i don't disagree in principal, but will need to check that frame length accounting doesn't go wrong in any area as a consequence of this. =20 >> +/** >> + * 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 l2le= n) >> +{ >> + uint8_t padded[ETH_ZLEN] =3D { 0 }; >> + struct iovec iov[2]; >> + size_t iovcnt =3D 0; >> + int m =3D 0; >> + uint32_t vnet_len; >> + >> + if (l2len < ETH_ZLEN) { >> + memcpy(padded, data, l2len); >> + data =3D padded; >> + l2len =3D ETH_ZLEN; >> + } >> + >> + vnet_len =3D htonl(l2len); >> + switch (c->mode) { >> + case MODE_PASST: >> + /* create an iov for the length */ >> + iov[iovcnt] =3D IOV_OF_LVALUE(vnet_len); >> + iovcnt++; >> + /* create the data iov */ >> + iov[iovcnt].iov_base =3D (void *)data; >> + iov[iovcnt].iov_len =3D l2len; >> + iovcnt++; >> + m =3D 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 =3D (void *)data; >> + iov[iovcnt].iov_len =3D l2len; >> + iovcnt++; >> + >> + m =3D 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 > >> + break; >> + case MODE_VU: >> + m =3D 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 =3D=3D MODE_PASST ? sizeof(uint32_t) : >> + (c->fd_vhost !=3D -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 st= ruct iovec *iov, >> =20 >> switch (c->mode) { >> case MODE_PASTA: >> - m =3D tap_send_frames_pasta(c, iov, bufs_per_frame, nframes); >> + /* use vhost in pasta sending only if the vhost setup was actually su= ccessful, indicated by a non-zero fd_vhost */ >> + m =3D tap_send_frames_pasta(c, iov, bufs_per_frame, nframes, ((c->fd_= vhost !=3D -1))); >> break; >> case MODE_PASST: >> m =3D tap_send_frames_passt(c, iov, bufs_per_frame, nframes); >> @@ -539,7 +554,8 @@ size_t tap_send_frames(const struct ctx *c, const st= ruct iovec *iov, >> nframes - m, nframes); >> =20 >> pcap_multiple(iov, bufs_per_frame, m, >> - c->mode =3D=3D MODE_PASST ? sizeof(uint32_t) : 0); >> + c->mode =3D=3D MODE_PASST ? sizeof(uint32_t) : >> + (c->fd_vhost !=3D -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 =20 >> +static inline struct iovec iov_from_virtio_net_hdr(struct virtio_net_hd= r_mrg_rxbuf *hdr) >> +{ >> + return (struct iovec){ >> + .iov_base =3D hdr, >> + .iov_len =3D 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 =3D 0; i < TCP_FRAMES_MEM; i++) { >> struct iovec *iov =3D tcp_l2_iov[i]; >> =20 >> - iov[TCP_IOV_TAP] =3D 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=20 >> + */ =09 >> + if ((c->fd_vhost !=3D -1)) >> + iov[TCP_IOV_TAP] =3D iov_from_virtio_net_hdr(&tcp_payload_tap_hdr[i]= ); >> + else=20 >> + iov[TCP_IOV_TAP] =3D tap_hdr_iov(c, (struct tap_hdr *)&tcp_payload_t= ap_hdr[i]); > > ..absorbing this if. So the if statement goes into tap_hdr_iov? ok noted > >> iov[TCP_IOV_ETH].iov_len =3D sizeof(struct ethhdr); >> iov[TCP_IOV_PAYLOAD].iov_base =3D &tcp_payload[i]; >> iov[TCP_IOV_ETH_PAD].iov_base =3D eth_pad; >> @@ -135,8 +151,7 @@ void tcp_payload_flush(const struct ctx *c, const st= ruct timespec *now) >> { >> size_t m; >> =20 >> - m =3D tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, >> - tcp_payload_used); >> + m =3D tap_send_frames(c, &tcp_l2_iov[0][0], TCP_NUM_IOVS, tcp_payload_= used); >> if (m !=3D 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 =3D IOV_TAIL(&iov[TCP_IOV_PAYLOAD], 1, 0); >> struct tcphdr th_storage, *th =3D IOV_REMOVE_HEADER(&tail, th_storage)= ; >> - struct tap_hdr *taph =3D iov[TCP_IOV_TAP].iov_base; >> + >> const struct flowside *tapside =3D TAPFLOW(conn); >> const struct in_addr *a4 =3D inany_v4(&tapside->oaddr); >> struct ethhdr *eh =3D iov[TCP_IOV_ETH].iov_base; >> @@ -192,7 +207,19 @@ static void tcp_l2_buf_fill_headers(const struct ct= x *c, >> =20 >> l2len =3D 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 th= is >> + * 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 =3D=3D MODE_PASST) { >> + iov[TCP_IOV_TAP].iov_len =3D sizeof(struct tap_hdr); >> + struct tap_hdr *taph =3D iov[TCP_IOV_TAP].iov_base; >> + tap_hdr_update(taph, l2len); >> + } else if (c->fd_vhost =3D=3D -1) { /* pasta mode but without vhost */ >> + iov[TCP_IOV_TAP].iov_len =3D 0; >> + } > > Similarly this if should go inside tap_hdr_update(). > >> } >> =20 >> /** >> 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" >> =20 >> -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 *co= nn, >> 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 =3D=3D MODE_PASTA ? 1 : TCP_FRAMES_MEM) >> =20 >> -extern struct tap_hdr tcp_payload_tap_hdr[TCP_FRAMES_MEM]; >> +extern struct virtio_net_hdr_mrg_rxbuf tcp_payload_tap_hdr[TCP_FRAMES_M= EM]; >> extern struct ethhdr tcp_eth_hdr[TCP_FRAMES_MEM]; >> extern struct tcp_payload_t tcp_payload[TCP_FRAMES_MEM]; >> =20 >> 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 >> =20 >> #include "checksum.h" >> #include "util.h" >> @@ -119,6 +120,18 @@ >> #include "udp_vu.h" >> #include "epoll_ctl.h" >> =20 >> +/* 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. But yeah they do need to be non static because of the need to share them with the kernel in virtio.c > >> #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, s= ize_t i) >> *siov =3D IOV_OF_LVALUE(payload->data); >> =20 >> tiov[UDP_IOV_ETH] =3D IOV_OF_LVALUE(udp_eth_hdr[i]); >> - tiov[UDP_IOV_TAP] =3D 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 !=3D -1) { >> + struct iovec vnet_iov =3D { >> + .iov_base =3D (void *)(&meta->vnet_hdr), >> + .iov_len =3D sizeof(meta->vnet_hdr) >> + }; >> + tiov[UDP_IOV_TAP] =3D vnet_iov; >> + } else { >> + tiov[UDP_IOV_TAP] =3D tap_hdr_iov(c, &meta->taph); >> + } > > Again, this logic should go inside tap_hdr_iov(). Got it.