Re: [PATCH 03/18] tap: Use iov_tail with tap_add_packet()

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

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

On Wed, Apr 02, 2025 at 07:23:28PM +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       |  2 +-
>  vu_common.c | 25 +++++--------------------
>  5 files changed, 28 insertions(+), 31 deletions(-)
> 
> diff --git a/iov.c b/iov.c
> index d96fc2ab594b..508fb6da91fb 100644
> --- a/iov.c
> +++ b/iov.c
> @@ -238,7 +238,6 @@ size_t iov_tail_size(struct iov_tail *tail)
>   *
>   * Returns:	true if the tail still contains any bytes, otherwise false
>   */
> -/* cppcheck-suppress unusedFunction */
>  bool iov_drop(struct iov_tail *tail, size_t len)
>  {
>  	tail->off = tail->off + len;
> diff --git a/pcap.c b/pcap.c
> index e95aa6fe29a6..404043a27e22 100644
> --- a/pcap.c
> +++ b/pcap.c
> @@ -76,6 +76,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 */
>  void pcap(const char *pkt, size_t l2len)
>  {
>  	struct iovec iov = { (char *)pkt, l2len };
> diff --git a/tap.c b/tap.c
> index 182a1151f139..ab3effe80f89 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -1040,29 +1040,36 @@ 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
>   */
> -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 ethhdr *eh;
> +	struct ethhdr ehc;
>  
> -	pcap(p, l2len);
> +	pcap_iov(data->iov, data->cnt, data->off);
>  
> -	eh = (struct ethhdr *)p;
> +	eh = IOV_PEEK_HEADER(data, ehc);
> +	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 */
> +
>  	switch (ntohs(eh->h_proto)) {
>  	case ETH_P_ARP:
>  	case ETH_P_IP:
> -		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:
> -		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;
> @@ -1128,6 +1135,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");
> @@ -1142,7 +1150,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);
> +		data = IOV_TAIL_FROM_BUF(p, l2len, 0);
> +		tap_add_packet(c, &data);
>  
>  		p += l2len;
>  		n -= l2len;
> @@ -1186,6 +1195,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) {
> @@ -1207,7 +1218,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);
> +		data = IOV_TAIL_FROM_BUF(pkt_buf + n, len, 0);
> +		tap_add_packet(c, &data);
>  	}
>  
>  	tap_handler(c, now);
> diff --git a/tap.h b/tap.h
> index dd39fd896f4a..5034acd8ac46 100644
> --- a/tap.h
> +++ b/tap.h
> @@ -119,6 +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);
>  
>  #endif /* TAP_H */
> diff --git a/vu_common.c b/vu_common.c
> index 686a09b28c8e..e446fc4f2054 100644
> --- a/vu_common.c
> +++ b/vu_common.c
> @@ -159,7 +159,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;
>  
> @@ -172,6 +171,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];
> @@ -187,25 +187,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);
> -		} 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);
> -			}
> -		}
> +
> +		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);
>  
>  		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 --]