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=202508 header.b=gRTRVPhR; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id DE9845A027A for ; Wed, 06 Aug 2025 03:58:25 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202508; t=1754445502; bh=Kj+02KtftXh1hRvm3/D9F71p4e/wWNPPkTm+kwsIwOs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gRTRVPhRViD2Mpq/DZDkLunkmxlxNVFKp3DDje1IpHq4OosKBH2X7j87mmfLHC3k4 2dxODFq6lFW03328VbwhVS3TI+BAa+CcAEMzfepwKdkoE15gFiZZ29femUGBwLO8Ml mT5WNXX7i6myjRfAp4KcqJfZ/krpSvmARBZ0bxrzQGyzzkrEZPH75iUPbWVgTPL/mm WBwZjzY98tqcSB7YgUM7F4tAFWN/QYfEJ1VB728f6LqSoa0LP/W8kHSMWhgEdr/ISh cugvD6gMt6eDXZVw5BZg4Rg4kQ78lv2RYtKDR1lC9Y9FnLOuPVqvHHXMTh4E2v53fR TKp7BQu7P2eDg== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4bxYNZ6flQz4xQ1; Wed, 6 Aug 2025 11:58:22 +1000 (AEST) Date: Wed, 6 Aug 2025 11:56:56 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH v8 04/30] tap: Use iov_tail with tap_add_packet() Message-ID: References: <20250805154628.301343-1-lvivier@redhat.com> <20250805154628.301343-5-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="NpYCIIRlTmKlyxfa" Content-Disposition: inline In-Reply-To: <20250805154628.301343-5-lvivier@redhat.com> Message-ID-Hash: W2SMJCHRAUCKD74AMFW6QIHAR5ULIV7X X-Message-ID-Hash: W2SMJCHRAUCKD74AMFW6QIHAR5ULIV7X 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 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: --NpYCIIRlTmKlyxfa Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Aug 05, 2025 at 05:46:02PM +0200, Laurent Vivier wrote: > Use IOV_PEEK_HEADER() to get the ethernet header from the iovec. >=20 > Move the workaround about multiple iovec array from vu_handle_tx() to > tap_add_packet(). Removing the offset out of the iovec array should > reduce the iovec count to 1. >=20 > Signed-off-by: Laurent Vivier > Reviewed-by: David Gibson > --- > iov.c | 1 - > pcap.c | 1 + > tap.c | 30 +++++++++++++++++++++--------- > tap.h | 3 +-- > vu_common.c | 26 +++++--------------------- > 5 files changed, 28 insertions(+), 33 deletions(-) >=20 > diff --git a/iov.c b/iov.c > index d39bb099fa69..97e4ea733540 100644 > --- a/iov.c > +++ b/iov.c > @@ -200,7 +200,6 @@ size_t iov_tail_size(struct iov_tail *tail) > * > * Return: true if the item still contains any bytes, otherwise false > */ > -/* cppcheck-suppress unusedFunction */ > bool iov_tail_drop(struct iov_tail *tail, size_t len) > { > tail->off =3D tail->off + len; > diff --git a/pcap.c b/pcap.c > index 46d11a2a6daa..03adc4c55f4b 100644 > --- a/pcap.c > +++ b/pcap.c > @@ -74,6 +74,7 @@ static void pcap_frame(const struct iovec *iov, size_t = iovcnt, > * @pkt: Pointer to data buffer, including L2 headers > * @l2len: L2 frame length > */ > +/* cppcheck-suppress unusedFunction */ Are we going to get any new users of this? Maybe we can just remove it now. > void pcap(const char *pkt, size_t l2len) > { > struct iovec iov =3D { (char *)pkt, l2len }; > diff --git a/tap.c b/tap.c > index 6db5d88b1760..c5520bf3bc76 100644 > --- a/tap.c > +++ b/tap.c > @@ -1070,24 +1070,29 @@ void tap_handler(struct ctx *c, const struct time= spec *now) > /** > * tap_add_packet() - Queue/capture packet, update notion of guest MAC a= ddress > * @c: Execution context > - * @l2len: Total L2 packet length > - * @p: Packet buffer > + * @data: Packet to add to the pool > * @now: Current timestamp > */ > -void tap_add_packet(struct ctx *c, ssize_t l2len, char *p, > +void tap_add_packet(struct ctx *c, struct iov_tail *data, > const struct timespec *now) > { > + struct ethhdr eh_storage; > const struct ethhdr *eh; > =20 > - pcap(p, l2len); > + pcap_iov(data->iov, data->cnt, data->off); > =20 > - eh =3D (struct ethhdr *)p; > + eh =3D IOV_PEEK_HEADER(data, eh_storage); > + if (!eh) > + return; > =20 > if (memcmp(c->guest_mac, eh->h_source, ETH_ALEN)) { > memcpy(c->guest_mac, eh->h_source, ETH_ALEN); > proto_update_l2_buf(c->guest_mac, NULL); > } > =20 > + iov_tail_prune(data); > + ASSERT(data->cnt =3D=3D 1); /* packet_add() doesn't support iovec */ So.. the IOV_PEEK_HEADER will have already pruned, but I think it would be a layering violation to assume that here. I'm wondering if we should change the invariants for the iov_tail structure and say they must *always* be pruned, outside of the innards of the iov_tail handling. That would mean pruning in all "constructors" of an iov_tail. It would also mean moving the prune to the end of iov_tail_drop() instead of the beginning. It would have the nice bonus that peek_header could take the iov_tail as a const *. > switch (ntohs(eh->h_proto)) { > case ETH_P_ARP: > case ETH_P_IP: > @@ -1095,14 +1100,16 @@ void tap_add_packet(struct ctx *c, ssize_t l2len,= char *p, > tap4_handler(c, pool_tap4, now); > pool_flush(pool_tap4); > } > - packet_add(pool_tap4, l2len, p); > + packet_add(pool_tap4, data->iov[0].iov_len - data->off, > + (char *)data->iov[0].iov_base + data->off); > break; > case ETH_P_IPV6: > if (pool_full(pool_tap6)) { > tap6_handler(c, pool_tap6, now); > pool_flush(pool_tap6); > } > - packet_add(pool_tap6, l2len, p); > + packet_add(pool_tap6, data->iov[0].iov_len - data->off, > + (char *)data->iov[0].iov_base + data->off); > break; > default: > break; > @@ -1168,6 +1175,7 @@ static void tap_passt_input(struct ctx *c, const st= ruct timespec *now) > =20 > while (n >=3D (ssize_t)sizeof(uint32_t)) { > uint32_t l2len =3D ntohl_unaligned(p); > + struct iov_tail data; > =20 > if (l2len < sizeof(struct ethhdr) || l2len > L2_MAX_LEN_PASST) { > err("Bad frame size from guest, resetting connection"); > @@ -1182,7 +1190,8 @@ static void tap_passt_input(struct ctx *c, const st= ruct timespec *now) > p +=3D sizeof(uint32_t); > n -=3D sizeof(uint32_t); > =20 > - tap_add_packet(c, l2len, p, now); > + data =3D IOV_TAIL_FROM_BUF(p, l2len, 0); > + tap_add_packet(c, &data, now); > =20 > p +=3D l2len; > n -=3D l2len; > @@ -1226,6 +1235,8 @@ static void tap_pasta_input(struct ctx *c, const st= ruct timespec *now) > for (n =3D 0; > n <=3D (ssize_t)(sizeof(pkt_buf) - L2_MAX_LEN_PASTA); > n +=3D len) { > + struct iov_tail data; > + > len =3D read(c->fd_tap, pkt_buf + n, L2_MAX_LEN_PASTA); > =20 > if (len =3D=3D 0) { > @@ -1247,7 +1258,8 @@ static void tap_pasta_input(struct ctx *c, const st= ruct timespec *now) > len > (ssize_t)L2_MAX_LEN_PASTA) > continue; > =20 > - tap_add_packet(c, len, pkt_buf + n, now); > + data =3D IOV_TAIL_FROM_BUF(pkt_buf + n, len, 0); > + tap_add_packet(c, &data, now); > } > =20 > tap_handler(c, now); > diff --git a/tap.h b/tap.h > index 936ae9371fd6..ce5510882d5d 100644 > --- a/tap.h > +++ b/tap.h > @@ -119,7 +119,6 @@ void tap_sock_update_pool(void *base, size_t size); > void tap_backend_init(struct ctx *c); > void tap_flush_pools(void); > void tap_handler(struct ctx *c, const struct timespec *now); > -void tap_add_packet(struct ctx *c, ssize_t l2len, char *p, > +void tap_add_packet(struct ctx *c, struct iov_tail *data, > const struct timespec *now); > - > #endif /* TAP_H */ > diff --git a/vu_common.c b/vu_common.c > index 5e6fd4a8261f..b77b21420c57 100644 > --- a/vu_common.c > +++ b/vu_common.c > @@ -163,7 +163,6 @@ static void vu_handle_tx(struct vu_dev *vdev, int ind= ex, > struct vu_virtq_element elem[VIRTQUEUE_MAX_SIZE]; > struct iovec out_sg[VIRTQUEUE_MAX_SIZE]; > struct vu_virtq *vq =3D &vdev->vq[index]; > - int hdrlen =3D sizeof(struct virtio_net_hdr_mrg_rxbuf); > int out_sg_count; > int count; > =20 > @@ -176,6 +175,7 @@ static void vu_handle_tx(struct vu_dev *vdev, int ind= ex, > while (count < VIRTQUEUE_MAX_SIZE && > out_sg_count + VU_MAX_TX_BUFFER_NB <=3D VIRTQUEUE_MAX_SIZE) { > int ret; > + struct iov_tail data; > =20 > elem[count].out_num =3D VU_MAX_TX_BUFFER_NB; > elem[count].out_sg =3D &out_sg[out_sg_count]; > @@ -191,26 +191,10 @@ static void vu_handle_tx(struct vu_dev *vdev, int i= ndex, > warn("virtio-net transmit queue contains no out buffers"); > break; > } > - if (elem[count].out_num =3D=3D 1) { > - tap_add_packet(vdev->context, > - elem[count].out_sg[0].iov_len - hdrlen, > - (char *)elem[count].out_sg[0].iov_base + > - hdrlen, now); > - } else { > - /* vnet header can be in a separate iovec */ > - if (elem[count].out_num !=3D 2) { > - debug("virtio-net transmit queue contains more than one buffer ([%d]= : %u)", > - count, elem[count].out_num); > - } else if (elem[count].out_sg[0].iov_len !=3D (size_t)hdrlen) { > - debug("virtio-net transmit queue entry not aligned on hdrlen ([%d]: = %d !=3D %zu)", > - count, hdrlen, elem[count].out_sg[0].iov_len); > - } else { > - tap_add_packet(vdev->context, > - elem[count].out_sg[1].iov_len, > - (char *)elem[count].out_sg[1].iov_base, > - now); > - } > - } > + > + data =3D IOV_TAIL(elem[count].out_sg, elem[count].out_num, 0); > + if (IOV_DROP_HEADER(&data, struct virtio_net_hdr_mrg_rxbuf)) > + tap_add_packet(vdev->context, &data, now); Nice simplification here. > =20 > count++; > } --=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 --NpYCIIRlTmKlyxfa Content-Type: application/pgp-signature; name=signature.asc -----BEGIN PGP SIGNATURE----- iQIzBAEBCgAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmiStlsACgkQzQJF27ox 2Gc5JRAAnBF3ZIXDckN8tX0FLmUY2gK1O5BWD17DcKzu7HmKB4YiHBecysEJtMog gRnm5BObA5L6AZCr0t/bIS35B2lvF1sfl6xDAwp/eZolKxJlwzCjXeMxsHTdrLSp qx3F/Cqj/pABWcSb7uNMmCBuJcO5GPDA+hEsrSW+v8KuAsWTVCLVSgf9opTIFFMl KJ3/oQ+vFC5520IQqhqjUNwM0PEXQd+yR0TYITW4LQMEb6tUvcsQd5bfjTHdqmzu TVhRqRn+TczIRXqO0rVpvQPvwKTQ9DnJgRj38/6k1uxpR/nOop40mxmdvYE7fbeR +sIeJlbl+hOU4gDfAR1FBvWDIcbPHmCsxmrSvXCYKYwxwrTcPVQ8HxV6dLsVwLOB mmAIces3vlUmEm577fmDSzHx/VV1l6JsFXPsxLXDEUWMhNfRgkixoJZ1BW5X3u5m jxWPeiUVQudaS0dFLb44EB6FgtbpKDyaEp9jyoAjTVMBT7wiKBhlqUUeCKDqMO/D 3+dnhd43Bah8M1cxTOtnp24KgFuMLlrt24pruZsw+zajsTGxkEJ+zueCmnSqiH+7 cFe1vBh4dFoLvdEj9wuoF47EAwf1W7zjw6ulzMCYvNeZWkA6ygZpw8PtSqnyNmOp GL6WchBrUtfVlEpiu3QR4uroreod3rICv/i2LIcAc4Yp1B1aSb4= =Z2Uy -----END PGP SIGNATURE----- --NpYCIIRlTmKlyxfa--