Re: [PATCH v3 1/6] tap: Remove pool parameter from tap4_handler() and tap6_handler()

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

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

On Wed, Dec 03, 2025 at 07:54:29PM +0100, Laurent Vivier wrote:
> These handlers only ever operate on their respective global pools
> (pool_tap4 and pool_tap6). The pool parameter was always passed the
> same value, making it unnecessary indirection.
> 
> Access the global pools directly instead, simplifying the function
> signatures.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> ---
>  tap.c | 46 +++++++++++++++++++++-------------------------
>  1 file changed, 21 insertions(+), 25 deletions(-)
> 
> diff --git a/tap.c b/tap.c
> index 44b06448757a..2cda8c9772b8 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -696,23 +696,21 @@ static bool tap4_is_fragment(const struct iphdr *iph,
>  /**
>   * tap4_handler() - IPv4 and ARP packet handler for tap file descriptor
>   * @c:		Execution context
> - * @in:		Ingress packet pool, packets with Ethernet headers
>   * @now:	Current timestamp
>   *
>   * Return: count of packets consumed by handlers
>   */
> -static int tap4_handler(struct ctx *c, const struct pool *in,
> -			const struct timespec *now)
> +static int tap4_handler(struct ctx *c, const struct timespec *now)
>  {
>  	unsigned int i, j, seq_count;
>  	struct tap4_l4_t *seq;
>  
> -	if (!c->ifi4 || !in->count)
> -		return in->count;
> +	if (!c->ifi4 || !pool_tap4->count)
> +		return pool_tap4->count;
>  
>  	i = 0;
>  resume:
> -	for (seq_count = 0, seq = NULL; i < in->count; i++) {
> +	for (seq_count = 0, seq = NULL; i < pool_tap4->count; i++) {
>  		size_t l3len, hlen, l4len;
>  		struct ethhdr eh_storage;
>  		struct iphdr iph_storage;
> @@ -722,7 +720,7 @@ resume:
>  		struct iov_tail data;
>  		struct iphdr *iph;
>  
> -		if (!packet_get(in, i, &data))
> +		if (!packet_get(pool_tap4, i, &data))
>  			continue;
>  
>  		eh = IOV_PEEK_HEADER(&data, eh_storage);
> @@ -789,7 +787,7 @@ resume:
>  		if (iph->protocol == IPPROTO_UDP) {
>  			struct iov_tail eh_data;
>  
> -			packet_get(in, i, &eh_data);
> +			packet_get(pool_tap4, i, &eh_data);
>  			if (dhcp(c, &eh_data))
>  				continue;
>  		}
> @@ -820,7 +818,7 @@ resume:
>  			goto append;
>  
>  		if (seq_count == TAP_SEQS)
> -			break;	/* Resume after flushing if i < in->count */
> +			break;	/* Resume after flushing if i < pool_tap4->count */
>  
>  		for (seq = tap4_l4 + seq_count - 1; seq >= tap4_l4; seq--) {
>  			if (L4_MATCH(iph, uh, seq)) {
> @@ -866,32 +864,30 @@ append:
>  		}
>  	}
>  
> -	if (i < in->count)
> +	if (i < pool_tap4->count)
>  		goto resume;
>  
> -	return in->count;
> +	return pool_tap4->count;
>  }
>  
>  /**
>   * tap6_handler() - IPv6 packet handler for tap file descriptor
>   * @c:		Execution context
> - * @in:		Ingress packet pool, packets with Ethernet headers
>   * @now:	Current timestamp
>   *
>   * Return: count of packets consumed by handlers
>   */
> -static int tap6_handler(struct ctx *c, const struct pool *in,
> -			const struct timespec *now)
> +static int tap6_handler(struct ctx *c, const struct timespec *now)
>  {
>  	unsigned int i, j, seq_count = 0;
>  	struct tap6_l4_t *seq;
>  
> -	if (!c->ifi6 || !in->count)
> -		return in->count;
> +	if (!c->ifi6 || !pool_tap6->count)
> +		return pool_tap6->count;
>  
>  	i = 0;
>  resume:
> -	for (seq_count = 0, seq = NULL; i < in->count; i++) {
> +	for (seq_count = 0, seq = NULL; i < pool_tap6->count; i++) {
>  		size_t l4len, plen, check;
>  		struct in6_addr *saddr, *daddr;
>  		struct ipv6hdr ip6h_storage;
> @@ -903,7 +899,7 @@ resume:
>  		struct ipv6hdr *ip6h;
>  		uint8_t proto;
>  
> -		if (!packet_get(in, i, &data))
> +		if (!packet_get(pool_tap6, i, &data))
>  			return -1;
>  
>  		eh = IOV_REMOVE_HEADER(&data, eh_storage);
> @@ -1011,7 +1007,7 @@ resume:
>  			goto append;
>  
>  		if (seq_count == TAP_SEQS)
> -			break;	/* Resume after flushing if i < in->count */
> +			break;	/* Resume after flushing if i < pool_tap6->count */
>  
>  		for (seq = tap6_l4 + seq_count - 1; seq >= tap6_l4; seq--) {
>  			if (L4_MATCH(ip6h, proto, uh, seq)) {
> @@ -1058,10 +1054,10 @@ append:
>  		}
>  	}
>  
> -	if (i < in->count)
> +	if (i < pool_tap6->count)
>  		goto resume;
>  
> -	return in->count;
> +	return pool_tap6->count;
>  }
>  
>  /**
> @@ -1080,8 +1076,8 @@ void tap_flush_pools(void)
>   */
>  void tap_handler(struct ctx *c, const struct timespec *now)
>  {
> -	tap4_handler(c, pool_tap4, now);
> -	tap6_handler(c, pool_tap6, now);
> +	tap4_handler(c, now);
> +	tap6_handler(c, now);
>  }
>  
>  /**
> @@ -1115,14 +1111,14 @@ void tap_add_packet(struct ctx *c, struct iov_tail *data,
>  	case ETH_P_ARP:
>  	case ETH_P_IP:
>  		if (!pool_can_fit(pool_tap4, data)) {
> -			tap4_handler(c, pool_tap4, now);
> +			tap4_handler(c, now);
>  			pool_flush(pool_tap4);
>  		}
>  		packet_add(pool_tap4, data);
>  		break;
>  	case ETH_P_IPV6:
>  		if (!pool_can_fit(pool_tap6, data)) {
> -			tap6_handler(c, pool_tap6, now);
> +			tap6_handler(c, now);
>  			pool_flush(pool_tap6);
>  		}
>  		packet_add(pool_tap6, data);
> -- 
> 2.51.1
> 

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