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=CcVzwDlu; dkim-atps=neutral Received: from mail-wm1-x32e.google.com (mail-wm1-x32e.google.com [IPv6:2a00:1450:4864:20::32e]) by passt.top (Postfix) with ESMTPS id 38E355A0262 for ; Wed, 12 Aug 2026 20:17:34 +0200 (CEST) Received: by mail-wm1-x32e.google.com with SMTP id 5b1f17b1804b1-495590dde14so16162725e9.0 for ; Wed, 12 Aug 2026 11:17:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1786558654; x=1787163454; darn=passt.top; h=in-reply-to:references:from:subject:cc:to:message-id:date :content-type:content-transfer-encoding:mime-version:from:to:cc :subject:date:message-id:reply-to:content-type; bh=+d1gUevKOEMKVEnFPWS/GJTEr6DAp4mrlT3Kte9N2Xk=; b=CcVzwDlu0HcyRVpFIRS9T17FeYrecgHtPQOrndmdLh1D4U4sDQZrJm1gxUB+ooEwZg rKv/oQiouX9IxXSZYEKr6efA5g3tJEp57/PkksWmiGSe8iB3y9j1Uj81DELnrdzYd1A/ Fo9AVZQbVENvC23C4yku1Kj4OXR70z0fM7DVfJMWZdfJPlff6KSK1mpEDYpwCvlU95uH 8Fjq223KEXkKzxjnuVLnM5729uXzm66h+hSHVI702cijGXrhPmVgH8/lqM9yNZ1EIVWN T6RBcUNMGu55LCjZZBbiXq72+c8gyxLEotNgPSFzhKymbjjnUNPbI9Yjz94YFpOj3JaD MEMA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1786558654; x=1787163454; h=in-reply-to:references:from:subject:cc:to: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=+d1gUevKOEMKVEnFPWS/GJTEr6DAp4mrlT3Kte9N2Xk=; b=ogLzMk94fphDTdS6t5v0sJZY1BfKEDBgxwOTYA16fjxoZMzeIaKzspR4oGG5m/SIYk CPdyI4ECunNQ6bqNlddiDQv6CmyK2bfwuf4LmkcZOERhpBGxJhVmZHin+s7QNk5qfrIK pYYNNT19CCg9Of8+I6zEubwAHZE885xOQIeSz3iV1Yeg82pAbcQx/esD8Qig60/ZpwTa vLskpOzLR44AKQdUAbZFAyfCwLPBs7Lsilj2ExApIK5eDNk+qn8WIxB/5uIQQYeMTznK 4popcM2bqFDI/F9BtVnruoWWauL1UE8zom2TIh/gjfoCg1LTOothqncU1LrCbkrrpRZS 5oaQ== X-Gm-Message-State: AOJu0YyC54MyMmi2mqC0wy966AJGQ8rI0jKIcobZl/zLjWqor6/AFg52 d9AqDdX0Sgh/1TAojAqJXaz99AGap1AgZP66IUR3624dNKZOm+7ZrDAD X-Gm-Gg: AR+sD11hrSDWgtKeOcFn0IA6aXjtqcs9VWm3cEMi4rGGZDAVHQwnpbCeAUofQ9TGdMP LGEbwMehnEdbPrAwHRnrpZFFBQvzzfLlLcYn9KU1mvkAuXyhY4I1m630r//Yuv7b1VEPSpOU1dG uICna8o8/7e53FAMpJhL8lTYHZCa8MfiO+o04ws5b6HtPv3jolc1f7DHsPWWmy9JOr39RfHnutc jWTtbIG26MXar/J2G7yz24my2wxE3iieUGwfxySGCnUKYwthOmzljv5zLNQZKjmv24Y1f0AXYht YNG45XBaBBaTT4OsL4gAHm5/R7tnhxHufyQIOPy+38cycWSBFgXA0QoV/ZntaLqGIxcQMIWNS0S vQuhioQu4jD2XTSR/VcGVmUDiowCGoBDWCzMNQTkBdgNG+jHD0nIEKATDHbudKqzEB01m8VcHeb XhYlEQWlAmEJFBpJSZlit7Hv/xjrvecbIAtU0sloIUQnoWcEaAgN9FlKVLIvWcvMjQqpObHP3VQ q4x2NELCRayuvhnh8VOimbbbgUROdMF1iAW X-Received: by 2002:a05:600c:19d3:b0:499:7e2b:8e5 with SMTP id 5b1f17b1804b1-4997e2b092bmr37352205e9.5.1786558653307; Wed, 12 Aug 2026 11:17:33 -0700 (PDT) Received: from localhost ([196.157.64.51]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-49981e3d3d9sm1474405e9.3.2026.08.12.11.17.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 12 Aug 2026 11:17:32 -0700 (PDT) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 12 Aug 2026 21:17:30 +0300 Message-Id: To: "David Gibson" , "Ammar Yasser" Subject: Re: [RFC v3 6/8] virtio: Implement pasta vhost acceleration guest->pasta path From: "Ammar Yasser" X-Mailer: aerc 0.21.0 References: <20260802132155.870796-1-aerosound161@gmail.com> <20260802132155.870796-7-aerosound161@gmail.com> In-Reply-To: Message-ID-Hash: U5ZRS26DN6JBY7VYVBQJMO6VDAIYH3FG X-Message-ID-Hash: U5ZRS26DN6JBY7VYVBQJMO6VDAIYH3FG 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 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 the >> kernel is informing us to handle this data. >>=20 >> We will however still recieve epoll events on the fd normally. We should > > "the fd" is not very clear here, there are a bunch of fds. I'm > guessing you mean the character device fd? 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. 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. =20 >> @@ -1359,7 +1362,8 @@ void tap_handler_pasta(struct ctx *c, uint32_t eve= nts, >> 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 accelera= tion 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. Yeah, agreed as per my comment above. > >> tap_pasta_input(c, now); >> } >> =20 >> @@ -1514,6 +1518,90 @@ void tap_listen_handler(struct ctx *c, uint32_t e= vents) >> tap_start_connection(c); >> } >> =20 >> +/** >> + * consume_one_rx_descriptor() - Consume one used RX descriptor from th= e 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". Noted > >> + * @len: Set to the length of data written by the kernel >> + * >> + * Pops a single entry from the used ring. Advances vqs[0].last_used_id= x >> + * (the number of entries we have consumed) and vqs[0].num_free (the co= unt >> + * of descriptors awaiting refill announcement). >> + *=20 >> + * NOTE: This function assumes the kernel is going to post single descr= iptors >> + * always, No chains. If that changes, we would need to increment num_f= ree >> + * as we advertise back to the kernel the free descriptors by the lengt= h of the chain. >> + * >> + * Return: pointer to the packet buffer, or NULL if no data is availabl= e >> + */ >> +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. Noted >> + /* 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. Ok > >> + >> + /* 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. Will do > >> + die("vhost: id %d len %u > %zu", i, *len, PKT_BUF_BYTES/VHOST_NDESCS)= ; >> + } >> + >> + 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 hyperv= isor vq >> + * @c: Execution context >> + * @ref: epoll reference >> + * @now: Current timestamp >> + */ >> +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct t= imespec *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. 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 ? > >> + 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. Woops, removing > >> 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. 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 > > >> + >> 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 *thdr= , size_t l2len) >> thdr->vnet_len =3D htonl(l2len); >> } >> =20 >> +void tap_vhost_input(struct ctx *c, union epoll_ref ref, const struct t= imespec *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