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=FxneXIJl; dkim-atps=neutral Received: from mail-ej1-x630.google.com (mail-ej1-x630.google.com [IPv6:2a00:1450:4864:20::630]) by passt.top (Postfix) with ESMTPS id 1DF585A0272 for ; Sun, 16 Aug 2026 19:55:12 +0200 (CEST) Received: by mail-ej1-x630.google.com with SMTP id a640c23a62f3a-c15ba3a2b4bso337275766b.1 for ; Sun, 16 Aug 2026 10:55:12 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786902911; x=1787507711; 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=dwnNkiOwT5AHdgZ9Mv7I5SC2O1EW66EiFleVdHMpRC0=; b=FxneXIJlpb/fnDnXaJJLLHe/utv/nGBoKON3PjkNa0G92KmBpe09iy6Cj9IQiY8lz5 4wiEw+6l/twCrI23F1p2LGEeqvZueJfrg3cy6X6mR8Dbx0eNWtnt0msDx2QcgCuYyUZA rcx9aoeq7hdf+QueTfoKoczbwRbAmukij6f6K+yjR3yOrCTUqq0Fxtqg0OkelVrwpfSb y+hwzEtH1ia+IWKwbU5FBWCjfWElYHNNlfyHbq19HAh1DdEpbynJwoQg0cj6/Or85Ejf EPbXaKO7w67Oagdq1ZLVkR7FMpUFg4SuSa9vcNlYQ3zXHGwZPFM9lf1ulfj6wLk6G9Na zUBg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786902911; x=1787507711; 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=dwnNkiOwT5AHdgZ9Mv7I5SC2O1EW66EiFleVdHMpRC0=; b=WQ3iL9g/7q3rK1YayaD0MpctzMjuc6ZtRiRam+KeEvLLpIhnAZR/RLn/ZN5FcmSCP2 3+l9IjSbdUgjI5DzE6nz9EsPs4NaoN0UDhaubq5cI4CzUtL0s+9BdkeQh3IMom4P82iW WDTvTQPgwfRS3lbHomIf/Rf2Z1f2rCRf4Zjf9xGauElXqJWoLY8gQPppeZCL/OSRiURd Lk6yFvCtYTyLw3hJwOd31O+FtO1rXVtBm54wqUQBJLp/VGI+xEU8UwY9P6AUG8Vg0kfq K4bKK1oIlDHs+4EUFs4OdTVaDQJCJ+1POHITJNfDpvXNVq6Ixc9lcLS0E4ZaZFKFDZGA y9EQ== X-Gm-Message-State: AOJu0YzR/7pJHc8PQ/wCXbPu67s0O8xWm46dQ/5BCpZMGeOqGrJfNqi3 pDq6kLp+oBeHAuJxT2ZXf4/oPhH5Xk78Z6e4R6PORPhv/o0tAeUIVN9mMVORPRoc X-Gm-Gg: AR+sD11gDRQvb7zBEUgItqwPh1xoZ83Pm11VIIOpEwUmzHVE1QfMF1INeESYFwHlXE4 EDBduOnrQZbZNmJ82VqAJLPUv3PcUfP1zyjaAvwR5lfWI/R71GUXjTue1jzbu5Vku0JgRHZC4Cr By+b6brm7lfLPrJTPIe31+tjsB4XIypmLn3FgSXInG+pfdXWv/S8m7TRO18Wm4SH3Pa96kxMdTZ 9bzfI1rtuIaeoF9YtO2Jt2hJ9NTQYKzxkReUIuKjZMOOBWlBLm/vMjdAQp74xGMLoby4yrFjsXR 6Yx2w2urz4uaURzNTCLPS0R2LcH0uKVfRg0YN2xowZ25h/15pcFyuSbtt/Gyul78ZaET6D67bAA LdiTQJeCsBLkOdhJM1bYv9pzCtWSN3N19Xh1mqMRhrycU8kDe1RdrZRYPTvjdXWvPyoVwcoawBl MjanbAarTpyVhMu6T9eBPWMqqdhCLs4U0qVzmF8ROsCS6qHRMiC0uxChQFmY/qM7edPEsoO+8uU 1PoI1NEas2knzT4dGz+Z7aV+Le2G2IOF5mNNoMb12Y= X-Received: by 2002:a17:907:94d2:b0:c16:8799:fcb4 with SMTP id a640c23a62f3a-c212a113fa6mr953158266b.19.1786902911152; Sun, 16 Aug 2026 10:55:11 -0700 (PDT) Received: from localhost ([196.137.36.17]) by smtp.gmail.com with ESMTPSA id a640c23a62f3a-c21234bff06sm369457966b.16.2026.08.16.10.55.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sun, 16 Aug 2026 10:55:09 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Sun, 16 Aug 2026 20:55:08 +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: KF4CTX4N5GEYNAAXQUNAOIKAX7TOO4NB X-Message-ID-Hash: KF4CTX4N5GEYNAAXQUNAOIKAX7TOO4NB 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 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 lo= wer >> >> in the file to by the time its called tap_send_frames_* functions a= re >> >> 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(). >>=20 >> 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. >>=20 >> Let me know your opinion on this! >>=20 >> > >> >> - 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. >>=20 >> 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 l= 2len) >> >> +{ >> >> + 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. >>=20 >> 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 >> >> =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 *n= ow); >> >> 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 t= cp_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_FRAME= S_MEM]; >> >> 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. >>=20 >> 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