From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202608 header.b=kZpg0DVI; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 6AA985A0271 for ; Thu, 13 Aug 2026 04:09:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202608; t=1786586982; bh=M6QmdKZfe9qAfCzBtm+Yvwb4gAoN1KgifkjCeQxDYTM=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=kZpg0DVIs381x/4FeobHkNdMda4kkzA+OxhQTysDb48QXZOqnJPs+OREAbdj3CUSq FmNk8WdD/0Y8wWIv5W+ah18LMuv3TnMRISYh2Wq1rIO4gEAK69hHC9A3Kh9sBDh8MZ RWxDcHeV+j0L5Hz1XgAb9UEeH20zGhsc2Dqt6WSz/HGPco0Iajo2JZL+axNsMxqic7 dp7EbpiQ3Lfaf+DAUahLawW85L8T3gc9Se3+qFw3bbN6vUB1odRPA9qxGRbMfpuP6B 4ueC3haG8blR8iDkvft01GhyBMajQD6Tj1C2N/a86KgW/6Ocw4Q1VTeHSQkEbzUHcx ThtEHo9bbSKJA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4hL81y1bxnz4wDx; Thu, 13 Aug 2026 12:09:42 +1000 (AEST) Date: Thu, 13 Aug 2026 11:35:07 +1000 From: David Gibson To: Ammar Yasser Subject: Re: [RFC v3 6/8] virtio: Implement pasta vhost acceleration guest->pasta path Message-ID: References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-7-aerosound161@gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="i5LCkKY6W5c3jTE1" Content-Disposition: inline In-Reply-To: Message-ID-Hash: WJJSZE4T4WODQRNV7NC5G65QZ6Q34ZC6 X-Message-ID-Hash: WJJSZE4T4WODQRNV7NC5G65QZ6Q34ZC6 X-MailFrom: dgibson@gandalf.ozlabs.org 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: --i5LCkKY6W5c3jTE1 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Aug 12, 2026 at 09:17:30PM +0300, Ammar Yasser wrote: > On Mon Aug 10, 2026 at 10:34 AM EEST, David Gibson wrote: > > On Sun, Aug 02, 2026 at 01:21:53PM +0000, Ammar Yasser wrote: > >> Add a new function called tap_vhost_input that will be used to indicate > >> that the guest is trying to send data to us through the rx queue and t= he > >> kernel is informing us to handle this data. > >>=20 > >> We will however still recieve epoll events on the fd normally. We shou= ld > > > > "the fd" is not very clear here, there are a bunch of fds. I'm > > guessing you mean the character device fd? >=20 > Yeah. And tying it back to your comment about the fact that we > shouldn't call epoll add with the device fd if the user requested vhost > means that this comment will be redundant anyways, will delete it. >=20 > For the record, while debugging performance, the fact that we received a > double kick on the device fd and the queue fd was a major slow down > reason. I did remove epoll registration for the device in this > experimental branch. Will include it in my next revision. Oh, excellent :). > =20 > >> @@ -1359,7 +1362,8 @@ void tap_handler_pasta(struct ctx *c, uint32_t e= vents, > >> if (events & (EPOLLRDHUP | EPOLLHUP | EPOLLERR)) > >> die("Disconnect event on /dev/net/tun device, exiting"); > >> =20 > >> - if (events & EPOLLIN) > >> + /* don't proceed with the normal tap processing in case vhost accele= ration was required */ > >> + if (events & EPOLLIN && !c->vhost) > > > > Seems like it would be more elegant to remove the character device fd > > from the epoll set (or at least remove EPOLLIN) when vhost is > > activated, rather than take it's events then ignore the. >=20 > Yeah, agreed as per my comment above. >=20 > > > >> tap_pasta_input(c, now); > >> } > >> =20 > >> @@ -1514,6 +1518,90 @@ void tap_listen_handler(struct ctx *c, uint32_t= events) > >> tap_start_connection(c); > >> } > >> =20 > >> +/** > >> + * consume_one_rx_descriptor() - Consume one used RX descriptor from = the kernel > > > > I'd suggest avoiding "Rx" or "Tx" terminology throughout, using > > "toguest" or "fromguest" instead. Note that for extra confusion Rx > > here is meaning "fromguest", but for vhost-user it means "toguest". >=20 > Noted >=20 > > > >> + * @len: Set to the length of data written by the kernel > >> + * > >> + * Pops a single entry from the used ring. Advances vqs[0].last_used_= idx > >> + * (the number of entries we have consumed) and vqs[0].num_free (the = count > >> + * of descriptors awaiting refill announcement). > >> + *=20 > >> + * NOTE: This function assumes the kernel is going to post single des= criptors > >> + * always, No chains. If that changes, we would need to increment num= _free > >> + * as we advertise back to the kernel the free descriptors by the len= gth of the chain. > >> + * > >> + * Return: pointer to the packet buffer, or NULL if no data is availa= ble > >> + */ > >> +static void *consume_one_rx_descriptor(unsigned *len) > > > > For a length of data, we'd typically use a size_t rather than a bare > > unsigned. >=20 > Noted >=20 > >> + /* read the last index we consumed */ > >> + last_used =3D vqs[0].last_used_idx % VHOST_NDESCS; > >> + /* read the index of what the */ > >> + i =3D le32toh(used->ring[last_used].id); > >> + *len =3D le32toh(used->ring[last_used].len); > >> + > >> + if (i !=3D last_used) { > >> + die("vhost: id %u at used position %u !=3D %u", i, last_used, i); > >> + } > > > > Braces not necessary for single line if blocks. >=20 > Ok >=20 > > > >> + > >> + /* the kernel has queued for us something we cannot receive */ > >> + if (*len > PKT_BUF_BYTES/VHOST_NDESCS) { > > > > This seems oddly circular: deriving the maximum frame size from the > > buffer and number of descriptors, when the number of descriptors was > > derived from the buffer size and maximum frame size. Better to use > > (or add) an L2_MAX_LEN_XX constant. >=20 > Will do >=20 > > > >> + die("vhost: id %d len %u > %zu", i, *len, PKT_BUF_BYTES/VHOST_NDESC= S); > >> + } > >> + > >> + vqs[0].last_used_idx++; > >> + vqs[0].num_free++; > >> + return pkt_buf + i * (PKT_BUF_BYTES/VHOST_NDESCS); > >> +} > >> + > >> + > >> +/** > >> + * tap_vhost_input() - Handler for new data on the tun socket to hype= rvisor vq > >> + * @c: Execution context > >> + * @ref: epoll reference > >> + * @now: Current timestamp > >> + */ > >> +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct= timespec *now) > >> +{ > >> + eventfd_read(ref.fd, (eventfd_t[]){ 0 }); > >> + > >> + tap_flush_pools(); > >> + > >> + struct virtio_net_hdr_mrg_rxbuf *hdr; > >> + struct iov_tail data; > >> + unsigned len; > > > > My convention, we don't use inline declarations, even though we could > > in C11 - move these up to the top. > > > >> + > >> + while ((hdr =3D consume_one_rx_descriptor(&len))) { > >> + if (len < sizeof(*hdr)) { > > > > I think you want to check that at least an ethernet (L2) header is > > there as well as the virtio_net_hdr_mrg_rxbuf: both tap_pasta_input() > > and tap_passt_input() appear to check for this. >=20 > Sure, i can add that. But just for my knowledge is there any case where > there can be a virtio_net header without an ethernet header ? Maybe. It might be possible to pass in tap device in with --fd, but have it misconfigured as a "tun" device (raw IP) instead of "tap" (virtual ethernet). Not sure if that's currently possible to combine with vhost-kernel, but it's close enough to possible that it makes sense to be cautious. I'm also not sure what would happen if the guest used an AF_PACKET socket to send malformed Ethernet frames. There's a good chance it would get filtered before reaching us, but still, better to check. >=20 > > > >> + warn("vhost: invalid len %u", len); > >> + continue; > >> + } > >> + =09 > >> + /* skip over the vnet header, we wanna add the packet without it*/ > >> + data =3D IOV_TAIL_FROM_BUF((void *)(hdr+1), len - sizeof(*hdr), 0); > >> + tap_add_packet(c, &data, now); > >> + } > >> + > >> + tap_handler(c, now); > >> + rx_descriptor_handoff(c); > >> +} > >> + > >> /** > >> * tap_ns_tun() - Get tuntap fd in namespace > >> * @c: Execution context > >> @@ -1524,16 +1612,15 @@ void tap_listen_handler(struct ctx *c, uint32_= t events) > >> */ > >> static int tap_ns_tun(void *arg) > >> { > >> - struct ifreq ifr =3D { .ifr_flags =3D IFF_TAP | IFF_NO_PI }; > >> - int flags =3D O_RDWR | O_NONBLOCK | O_CLOEXEC; > >> struct ctx *c =3D (struct ctx *)arg; > >> + struct ifreq ifr =3D { .ifr_flags =3D IFF_TAP | IFF_NO_PI }; > >> int fd, rc; > >> =20 > >> c->fd_tap =3D -1; > >> memcpy(ifr.ifr_name, c->pasta_ifn, IFNAMSIZ); > >> ns_enter(c); > >> =20 > >> - fd =3D open("/dev/net/tun", flags); > >> + fd =3D open("/dev/net/tun", O_RDWR | O_NONBLOCK | O_CLOEXEC); > > > > This change seems unrelated to the actual aim of this patch. >=20 > Woops, removing >=20 > > > >> if (fd < 0) > >> die_perror("Failed to open() /dev/net/tun"); > >> =20 > >> @@ -1561,6 +1648,20 @@ static void tap_sock_tun_init(struct ctx *c) > >> die("Failed to set up tap device in namespace"); > >> } > >> =20 > >> + /* initialize the vhost-net dev file descriptor */ > >> + if (c->vhost) { > >> + setup_vhost_net(c); > >> + > >> + for (int i =3D 0; i < ARRAY_SIZE(c->vq); i++) > >> + setup_eventfds(c, i); > >> + > >> + if (setup_memory_table(c) < 0) > >> + die_perror("VHOST_SET_MEM_TABLE ioctl on /dev/vhost-net failed"); > >> + > >> + for (int i =3D 0; i < ARRAY_SIZE(c->vq); i++) > >> + set_vring_for_queue(c, i, c->fd_tap); > >> + } > > > > It's not clear to me if this code block needs to execute within the > > guest namespace. If not, it should probably be in tap_sock_tun_init() > > rather than tap_ns_tun(). If it _does_ have to be in tap_ns_tun(), > > then tap_ns_tun() probably needs a rename, since it's now doing rather > > more in the vhost case that just getting a device fd. >=20 > Not sure i follow. its already in tap_sock_tun_init not tap_ns_tun. > But generally speaking it needs to be after tap_ns_tun. because it > relies on the device fd that tap_ns_tun has created in the namespace Oh, sorry, I misread the diff. > >> + > >> pasta_ns_conf(c); > >> =20 > >> if (!c->splice_only) > >> diff --git a/tap.h b/tap.h > >> index 1625975..eb02da8 100644 > >> --- a/tap.h > >> +++ b/tap.h > >> @@ -66,6 +66,7 @@ static inline void tap_hdr_update(struct tap_hdr *th= dr, size_t l2len) > >> thdr->vnet_len =3D htonl(l2len); > >> } > >> =20 > >> +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct= timespec *now); > >> unsigned long tap_l2_max_len(const struct ctx *c); > >> void *tap_push_l2h(const struct ctx *c, void *buf, > >> const void *src_mac, uint16_t proto); > >> --=20 > >> 2.34.1 > >>=20 >=20 --=20 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 --i5LCkKY6W5c3jTE1 Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmp9H0UACgkQzQJF27ox 2GewERAAoC75GuADmDFciiSp1vdJpqZjn9ZpWfYMB+PJet3+NKd3neiLbpujpEGj uu+lHj6glPrwc0KP6kfM5nNzbcEo+T1BZdEmHQS3HSOq8Kiw2tnSA7QY+wAWXn5a 0hQ6YZ9AbipCnBsUMBYwxTNUGlz7ZiXQnzGqO9sQqtb57eRBsmjE5KGfsL1HGa/K sZWNVtikxxAs9Sq6Sq1tHmbYt6o9NvRUKWnEbaTlOWFXrr21fh1rrpIvLv0JXBGs 4I+jYGKZXDNGLQY7flQTdSI2sK4Jz8tbQ+3E2BIlBk6IRcAFdumDO5jQglcjc6ny NXYWDyiiG2BA5exsce90r4PnlrLFYliBH39/5BBMxny3umnWtLXL9HrnKnhh+Svd 8jkikIzsm+YzlzB0RMuJuCg+GJme+CZtmRj9HP/i6JDLGBftcMfBwG7XgyZZrxvn vysKf4Z+I4Lmka7hEGmo6akLXakeYbTSHupY13/83eHfMZk0DAWjmt+6PLpEDq+N mSiVDBJ+gCx7+eSYCdWHHbMhhyFvFNJ0Ay8Af4Afa6yfcP3ChDlzal2EYC3URtrd NEkLNXqfTO4e6Kl0fLeBy4GceBRowd29/Fkz7aLO1JGW5eRQNcczYiT7i76Dgi2Z qgFoPYP/gpSEipHqgiJHeKZ6HT8wAP87FL2Y+gnTcatsWFGsBVE= =KIin -----END PGP SIGNATURE----- --i5LCkKY6W5c3jTE1--