Re: [PATCH 5/7] udp: Share payload buffers between IPv4 and IPv6

[Stefano Brivio <sbrivio@redhat.com>]

On Tue, 30 Apr 2024 20:05:39 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Currently the IPv4 and IPv6 paths unnecessarily use different buffers for
> the UDP payload.  Now that we're handling the various pieces of the UDP
> packets with an iov, we can split the payload part of the buffers off into
> its own array shared between IPv4 and IPv6.  As well as saving a little
> memory, this allows the payload buffers to be neatly page aligned.
> 
> With the buffers merged, udp[46]_l2_iov_sock contain exactly the same thing
> as each other and can also be merged.  Likewise udp[46]_iov_splice can be
> merged together.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 127 ++++++++++++++++++++++++++++++----------------------------
>  1 file changed, 65 insertions(+), 62 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index 86d0419f..8c726056 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -171,14 +171,22 @@ static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][DIV_ROUND_UP(NUM_PORTS, 8)
>  
>  /* Static buffers */
>  
> +static struct udp_payload {

As we're defining a new struct, the usual comment format would be nice,
and I would also keep it symmetric with tcp_payload_t (udp_payload_t),
say:

/**
 * struct udp_payload_t - UDP header and data for inbound messages
 * @uh:		UDP header
 * @data:	UDP data
 */

> +	struct udphdr uh;
> +	char data[USHRT_MAX - sizeof(struct udphdr)];
> +#ifdef __AVX2__
> +} __attribute__ ((packed, aligned(32)))
> +#else
> +} __attribute__ ((packed, aligned(__alignof__(unsigned int))))
> +#endif
> +udp_payload_buf[UDP_MAX_FRAMES];

...and this could be 'udp_payload'.

> +
>  /**
>   * udp4_l2_buf_t - Pre-cooked IPv4 packet buffers for tap connections
>   * @s_in:	Source socket address, filled in by recvmmsg()
>   * @taph:	Tap backend specific header
>   * @eh:		Prefilled ethernet header
>   * @iph:	Pre-filled IP header (except for tot_len and saddr)
> - * @uh:		Headroom for UDP header
> - * @data:	Storage for UDP payload
>   */
>  static struct udp4_l2_buf_t {
>  	struct sockaddr_in s_in;
> @@ -186,9 +194,6 @@ static struct udp4_l2_buf_t {
>  	struct tap_hdr taph;
>  	struct ethhdr eh;
>  	struct iphdr iph;
> -	struct udphdr uh;
> -	uint8_t data[USHRT_MAX -
> -		     (sizeof(struct iphdr) + sizeof(struct udphdr))];
>  } __attribute__ ((packed, aligned(__alignof__(unsigned int))))
>  udp4_l2_buf[UDP_MAX_FRAMES];
>  
> @@ -198,8 +203,6 @@ udp4_l2_buf[UDP_MAX_FRAMES];
>   * @taph:	Tap backend specific header
>   * @eh:		Pre-filled ethernet header
>   * @ip6h:	Pre-filled IP header (except for payload_len and addresses)
> - * @uh:		Headroom for UDP header
> - * @data:	Storage for UDP payload
>   */
>  struct udp6_l2_buf_t {
>  	struct sockaddr_in6 s_in6;
> @@ -212,9 +215,6 @@ struct udp6_l2_buf_t {
>  	struct tap_hdr taph;
>  	struct ethhdr eh;
>  	struct ipv6hdr ip6h;
> -	struct udphdr uh;
> -	uint8_t data[USHRT_MAX -
> -		     (sizeof(struct ipv6hdr) + sizeof(struct udphdr))];
>  #ifdef __AVX2__
>  } __attribute__ ((packed, aligned(32)))
>  #else
> @@ -239,8 +239,7 @@ enum udp_iov_idx {
>  };
>  
>  /* recvmmsg()/sendmmsg() data for tap */
> -static struct iovec	udp4_l2_iov_sock	[UDP_MAX_FRAMES];
> -static struct iovec	udp6_l2_iov_sock	[UDP_MAX_FRAMES];
> +static struct iovec	udp_l2_iov_sock		[UDP_MAX_FRAMES];
>  
>  static struct iovec	udp4_l2_iov_tap		[UDP_MAX_FRAMES][UDP_NUM_IOVS];
>  static struct iovec	udp6_l2_iov_tap		[UDP_MAX_FRAMES][UDP_NUM_IOVS];
> @@ -249,8 +248,7 @@ static struct mmsghdr	udp4_l2_mh_sock		[UDP_MAX_FRAMES];
>  static struct mmsghdr	udp6_l2_mh_sock		[UDP_MAX_FRAMES];
>  
>  /* recvmmsg()/sendmmsg() data for "spliced" connections */
> -static struct iovec	udp4_iov_splice		[UDP_MAX_FRAMES];
> -static struct iovec	udp6_iov_splice		[UDP_MAX_FRAMES];
> +static struct iovec	udp_iov_splice		[UDP_MAX_FRAMES];
>  
>  static struct sockaddr_in udp4_localname = {
>  	.sin_family = AF_INET,
> @@ -261,8 +259,8 @@ static struct sockaddr_in6 udp6_localname = {
>  	.sin6_addr = IN6ADDR_LOOPBACK_INIT,
>  };
>  
> -static struct mmsghdr	udp4_mh_splice		[UDP_MAX_FRAMES];
> -static struct mmsghdr	udp6_mh_splice		[UDP_MAX_FRAMES];
> +static struct mmsghdr	udp4_mh_splice	[UDP_MAX_FRAMES];
> +static struct mmsghdr	udp6_mh_splice	[UDP_MAX_FRAMES];
>  
>  /**
>   * udp_portmap_clear() - Clear UDP port map before configuration
> @@ -322,10 +320,14 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
>   */
>  static void udp_iov_init_one(const struct ctx *c, size_t i)
>  {
> +	struct udp_payload *payload = &udp_payload_buf[i];
> +	struct iovec *siov = &udp_l2_iov_sock[i];
> +
> +	*siov = IOV_OF_LVALUE(payload->data);
> +
>  	if (c->ifi4) {
>  		struct msghdr *mh = &udp4_l2_mh_sock[i].msg_hdr;
>  		struct udp4_l2_buf_t *buf = &udp4_l2_buf[i];
> -		struct iovec *siov = &udp4_l2_iov_sock[i];
>  		struct iovec *tiov = udp4_l2_iov_tap[i];
>  
>  		*buf = (struct udp4_l2_buf_t) {
> @@ -333,8 +335,6 @@ static void udp_iov_init_one(const struct ctx *c, size_t i)
>  			.iph = L2_BUF_IP4_INIT(IPPROTO_UDP)
>  		};
>  
> -		*siov		= IOV_OF_LVALUE(buf->data);
> -
>  		mh->msg_name	= &buf->s_in;
>  		mh->msg_namelen	= sizeof(buf->s_in);
>  		mh->msg_iov	= siov;
> @@ -343,13 +343,12 @@ static void udp_iov_init_one(const struct ctx *c, size_t i)
>  		tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &buf->taph);
>  		tiov[UDP_IOV_ETH] = IOV_OF_LVALUE(buf->eh);
>  		tiov[UDP_IOV_IP] = IOV_OF_LVALUE(buf->iph);
> -		tiov[UDP_IOV_PAYLOAD].iov_base = &buf->uh;
> +		tiov[UDP_IOV_PAYLOAD].iov_base = payload;
>  	}
>  
>  	if (c->ifi6) {
>  		struct msghdr *mh = &udp6_l2_mh_sock[i].msg_hdr;
>  		struct udp6_l2_buf_t *buf = &udp6_l2_buf[i];
> -		struct iovec *siov = &udp6_l2_iov_sock[i];
>  		struct iovec *tiov = udp6_l2_iov_tap[i];
>  
>  		*buf = (struct udp6_l2_buf_t) {
> @@ -357,8 +356,6 @@ static void udp_iov_init_one(const struct ctx *c, size_t i)
>  			.ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP)
>  		};
>  
> -		*siov		 = IOV_OF_LVALUE(buf->data);
> -
>  		mh->msg_name	= &buf->s_in6;
>  		mh->msg_namelen	= sizeof(buf->s_in6);
>  		mh->msg_iov	= siov;
> @@ -367,7 +364,7 @@ static void udp_iov_init_one(const struct ctx *c, size_t i)
>  		tiov[UDP_IOV_TAP] = tap_hdr_iov(c, &buf->taph);
>  		tiov[UDP_IOV_ETH] = IOV_OF_LVALUE(buf->eh);
>  		tiov[UDP_IOV_IP] = IOV_OF_LVALUE(buf->ip6h);
> -		tiov[UDP_IOV_PAYLOAD].iov_base = &buf->uh;
> +		tiov[UDP_IOV_PAYLOAD].iov_base = payload;
>  	}
>  }
>  
> @@ -581,22 +578,24 @@ static void udp_splice_sendfrom(const struct ctx *c, unsigned start, unsigned n,
>  /**
>   * udp_update_hdr4() - Update headers for one IPv4 datagram
>   * @c:		Execution context
> - * @b:		Pointer to udp4_l2_buf to update
> + * @bh:		Pointer to udp4_l2_buf to update
> + * @bp:		Pointer to udp_payload to update
>   * @dstport:	Destination port number
>   * @plen:	Length of UDP payload
>   * @now:	Current timestamp
>   *
>   * Return: size of IPv4 payload (UDP header + data)
>   */
> -static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
> +static size_t udp_update_hdr4(const struct ctx *c,
> +			      struct udp4_l2_buf_t *bh, struct udp_payload *bp,
>  			      in_port_t dstport, size_t plen,
>  			      const struct timespec *now)
>  {
> +	in_port_t srcport = ntohs(bh->s_in.sin_port);
>  	const struct in_addr dst = c->ip4.addr_seen;
> -	size_t l4len = plen + sizeof(b->uh);
> -	size_t l3len = l4len + sizeof(b->iph);
> -	in_port_t srcport = ntohs(b->s_in.sin_port);
> -	struct in_addr src = b->s_in.sin_addr;
> +	struct in_addr src = bh->s_in.sin_addr;
> +	size_t l4len = plen + sizeof(bp->uh);
> +	size_t l3len = l4len + sizeof(bh->iph);
>  
>  	if (!IN4_IS_ADDR_UNSPECIFIED(&c->ip4.dns_match) &&
>  	    IN4_ARE_ADDR_EQUAL(&src, &c->ip4.dns_host) && srcport == 53 &&
> @@ -617,39 +616,41 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
>  		src = c->ip4.gw;
>  	}
>  
> -	b->iph.tot_len = htons(l3len);
> -	b->iph.daddr = dst.s_addr;
> -	b->iph.saddr = src.s_addr;
> -	b->iph.check = csum_ip4_header(b->iph.tot_len, IPPROTO_UDP,
> -				       src, c->ip4.addr_seen);
> +	bh->iph.tot_len = htons(l3len);
> +	bh->iph.daddr = c->ip4.addr_seen.s_addr;

This is still the same as dst.s_addr (existing assignment), right?

-- 
Stefano