Re: [PATCH v8 04/30] tap: Use iov_tail with tap_add_packet()

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 8105 bytes --]

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.
> 
> 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.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> Reviewed-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  iov.c       |  1 -
>  pcap.c      |  1 +
>  tap.c       | 30 +++++++++++++++++++++---------
>  tap.h       |  3 +--
>  vu_common.c | 26 +++++---------------------
>  5 files changed, 28 insertions(+), 33 deletions(-)
> 
> 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 = 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 = { (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 timespec *now)
>  /**
>   * tap_add_packet() - Queue/capture packet, update notion of guest MAC address
>   * @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;
>  
> -	pcap(p, l2len);
> +	pcap_iov(data->iov, data->cnt, data->off);
>  
> -	eh = (struct ethhdr *)p;
> +	eh = IOV_PEEK_HEADER(data, eh_storage);
> +	if (!eh)
> +		return;
>  
>  	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);
>  	}
>  
> +	iov_tail_prune(data);
> +	ASSERT(data->cnt == 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 struct timespec *now)
>  
>  	while (n >= (ssize_t)sizeof(uint32_t)) {
>  		uint32_t l2len = ntohl_unaligned(p);
> +		struct iov_tail data;
>  
>  		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 struct timespec *now)
>  		p += sizeof(uint32_t);
>  		n -= sizeof(uint32_t);
>  
> -		tap_add_packet(c, l2len, p, now);
> +		data = IOV_TAIL_FROM_BUF(p, l2len, 0);
> +		tap_add_packet(c, &data, now);
>  
>  		p += l2len;
>  		n -= l2len;
> @@ -1226,6 +1235,8 @@ static void tap_pasta_input(struct ctx *c, const struct timespec *now)
>  	for (n = 0;
>  	     n <= (ssize_t)(sizeof(pkt_buf) - L2_MAX_LEN_PASTA);
>  	     n += len) {
> +		struct iov_tail data;
> +
>  		len = read(c->fd_tap, pkt_buf + n, L2_MAX_LEN_PASTA);
>  
>  		if (len == 0) {
> @@ -1247,7 +1258,8 @@ static void tap_pasta_input(struct ctx *c, const struct timespec *now)
>  		    len > (ssize_t)L2_MAX_LEN_PASTA)
>  			continue;
>  
> -		tap_add_packet(c, len, pkt_buf + n, now);
> +		data = IOV_TAIL_FROM_BUF(pkt_buf + n, len, 0);
> +		tap_add_packet(c, &data, now);
>  	}
>  
>  	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 index,
>  	struct vu_virtq_element elem[VIRTQUEUE_MAX_SIZE];
>  	struct iovec out_sg[VIRTQUEUE_MAX_SIZE];
>  	struct vu_virtq *vq = &vdev->vq[index];
> -	int hdrlen = sizeof(struct virtio_net_hdr_mrg_rxbuf);
>  	int out_sg_count;
>  	int count;
>  
> @@ -176,6 +175,7 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
>  	while (count < VIRTQUEUE_MAX_SIZE &&
>  	       out_sg_count + VU_MAX_TX_BUFFER_NB <= VIRTQUEUE_MAX_SIZE) {
>  		int ret;
> +		struct iov_tail data;
>  
>  		elem[count].out_num = VU_MAX_TX_BUFFER_NB;
>  		elem[count].out_sg = &out_sg[out_sg_count];
> @@ -191,26 +191,10 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
>  			warn("virtio-net transmit queue contains no out buffers");
>  			break;
>  		}
> -		if (elem[count].out_num == 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 != 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 != (size_t)hdrlen) {
> -				debug("virtio-net transmit queue entry not aligned on hdrlen ([%d]: %d != %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 = 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.

>  
>  		count++;
>  	}

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]