Re: [PATCH v5 05/12] arp: Pass queue pair explicitly through ARP send path

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

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

On Tue, Jun 16, 2026 at 02:51:23PM +0200, Laurent Vivier wrote:
> Add a qpair parameter to arp(), arp_send_init_req(), and
> arp_announce(), forwarding it to tap_send_single() instead of
> hardcoding QPAIR_DEFAULT.
> 
> tap_start_connection() now consumes the qpair parameter it received
> in the previous commit.
> 
> fwd_neigh_table_update() has no queue pair context and keeps using
> QPAIR_DEFAULT.
> 
> No functional change.
> 
> Signed-off-by: Laurent Vivier <lvivier@redhat.com>
> ---
>  arp.c | 15 +++++++++------
>  arp.h |  6 +++---
>  fwd.c |  2 +-
>  tap.c |  6 ++----
>  4 files changed, 15 insertions(+), 14 deletions(-)
> 
> diff --git a/arp.c b/arp.c
> index e97c4de86a99..1dc8b87cd993 100644
> --- a/arp.c
> +++ b/arp.c
> @@ -63,11 +63,12 @@ static bool ignore_arp(const struct ctx *c,
>  /**
>   * arp() - Check if this is a supported ARP message, reply as needed
>   * @c:		Execution context
> + * @qpair:	Queue pair on which to send the reply
>   * @data:	Single packet with Ethernet buffer
>   *
>   * Return: 1 if handled, -1 on failure
>   */
> -int arp(const struct ctx *c, struct iov_tail *data)
> +int arp(const struct ctx *c, unsigned int qpair, struct iov_tail *data)

The qpair parameter makes sense to me here: you want to send the reply
on the same qpair the request came in on.

>  {
>  	union inany_addr tgt;
>  	struct {
> @@ -112,7 +113,7 @@ int arp(const struct ctx *c, struct iov_tail *data)
>  	memcpy(resp.am.tha,		am->sha,	sizeof(resp.am.tha));
>  	memcpy(resp.am.tip,		am->sip,	sizeof(resp.am.tip));
>  
> -	tap_send_single(c, QPAIR_DEFAULT, &resp, sizeof(resp));
> +	tap_send_single(c, qpair, &resp, sizeof(resp));
>  
>  	return 1;
>  }
> @@ -120,8 +121,9 @@ int arp(const struct ctx *c, struct iov_tail *data)
>  /**
>   * arp_send_init_req() - Send initial ARP request to retrieve guest MAC address
>   * @c:		Execution context
> + * @qpair:	Queue pair on which to send the request
>   */
> -void arp_send_init_req(const struct ctx *c)
> +void arp_send_init_req(const struct ctx *c, unsigned int qpair)

But I'm dubious about it here.  These are unsolicited messages to
which we don't expect a reply.  They're also low volume, so I don't
see that we particularly need to distribute across queues.  What's the
advantage in putting QPAIR_DEFAULT in the caller, rather than right
here?

>  {
>  	struct {
>  		struct ethhdr eh;
> @@ -148,16 +150,17 @@ void arp_send_init_req(const struct ctx *c)
>  	memcpy(req.am.tip,	&c->ip4.addr,		sizeof(req.am.tip));
>  
>  	debug("Sending initial ARP request for guest MAC address");
> -	tap_send_single(c, QPAIR_DEFAULT, &req, sizeof(req));
> +	tap_send_single(c, qpair, &req, sizeof(req));
>  }
>  
>  /**
>   * arp_announce() - Send an ARP announcement for an IPv4 host
>   * @c:		Execution context
> + * @qpair:	Queue pair on which to send the announcement
>   * @ip:	IPv4 address we announce as owned by @mac
>   * @mac:	MAC address to advertise for @ip
>   */
> -void arp_announce(const struct ctx *c, struct in_addr *ip,
> +void arp_announce(const struct ctx *c, unsigned int qpair, struct in_addr *ip,
>  		  const unsigned char *mac)

Similar thoughts here.

>  {
>  	char ip_str[INET_ADDRSTRLEN];
> @@ -202,5 +205,5 @@ void arp_announce(const struct ctx *c, struct in_addr *ip,
>  	eth_ntop(mac, mac_str, sizeof(mac_str));
>  	debug("ARP announcement for %s / %s", ip_str, mac_str);
>  
> -	tap_send_single(c, QPAIR_DEFAULT, &msg, sizeof(msg));
> +	tap_send_single(c, qpair, &msg, sizeof(msg));
>  }
> diff --git a/arp.h b/arp.h
> index 4b1f38bcec9b..501760393a74 100644
> --- a/arp.h
> +++ b/arp.h
> @@ -22,9 +22,9 @@ struct arpmsg {
>  	unsigned char tip[4];
>  } __attribute__((__packed__));
>  
> -int arp(const struct ctx *c, struct iov_tail *data);
> -void arp_send_init_req(const struct ctx *c);
> -void arp_announce(const struct ctx *c, struct in_addr *ip,
> +int arp(const struct ctx *c, unsigned int qpair, struct iov_tail *data);
> +void arp_send_init_req(const struct ctx *c, unsigned int qpair);
> +void arp_announce(const struct ctx *c, unsigned int qpair, struct in_addr *ip,
>  		  const unsigned char *mac);
>  
>  #endif /* ARP_H */
> diff --git a/fwd.c b/fwd.c
> index c0a6adacd294..0d0e265b7dc0 100644
> --- a/fwd.c
> +++ b/fwd.c
> @@ -145,7 +145,7 @@ void fwd_neigh_table_update(const struct ctx *c, const union inany_addr *addr,
>  		return;
>  
>  	if (inany_v4(addr))
> -		arp_announce(c, inany_v4(addr), e->mac);
> +		arp_announce(c, QPAIR_DEFAULT, inany_v4(addr), e->mac);
>  	else
>  		ndp_unsolicited_na(c, &addr->a6);
>  }
> diff --git a/tap.c b/tap.c
> index 521ccd6d47e7..66dcb83665a7 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -741,7 +741,7 @@ resume:
>  		if (!eh)
>  			continue;
>  		if (ntohs(eh->h_proto) == ETH_P_ARP) {
> -			arp(c, &data);
> +			arp(c, qpair, &data);
>  			continue;
>  		}
>  
> @@ -1453,8 +1453,6 @@ static void tap_start_connection(const struct ctx *c, unsigned int qpair)
>  {
>  	union epoll_ref ref = { 0 };
>  
> -	(void)qpair;
> -
>  	ref.fd = c->fd_tap;
>  	switch (c->mode) {
>  	case MODE_PASST:
> @@ -1474,7 +1472,7 @@ static void tap_start_connection(const struct ctx *c, unsigned int qpair)
>  		return;
>  
>  	if (c->ifi4)
> -		arp_send_init_req(c);
> +		arp_send_init_req(c, qpair);
>  	if (c->ifi6 && !c->no_ndp)
>  		ndp_send_init_req(c);
>  }
> -- 
> 2.54.0
> 

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