Re: [PATCH 7/7] udp: Single buffer for IPv4, IPv6 headers and metadata

[Stefano Brivio <sbrivio@redhat.com>]

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

> Currently we have separate arrays for IPv4 and IPv6 which contain the
> headers for guest-bound packets, and also the originating socket address.
> We can combine these into a single array of "metadata" structures with
> space for both pre-cooked IPv4 and IPv6 headers, as well as shared space
> for the tap specific header and socket address (using sockaddr_inany).

This is neat, definitely, I just wonder: will we need to essentially
revert this if we implement a flow-based mechanism where frames can be
sent to different guests/containers?

On the other hand, chances are it would help instead because we would
have tables of metadata instead of those being buried into frames.

> Because we're using IOVs to separately address the pieces of each frame,
> these structures don't need to be packed to keep the headers contiguous
> so we can more naturally arrange for the alignment we want.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  udp.c | 131 ++++++++++++++++++++++++----------------------------------
>  1 file changed, 55 insertions(+), 76 deletions(-)
> 
> diff --git a/udp.c b/udp.c
> index 64e7dcef..a9cc6ed2 100644
> --- a/udp.c
> +++ b/udp.c
> @@ -184,44 +184,27 @@ udp_payload_buf[UDP_MAX_FRAMES];
>  /* Ethernet header for IPv4 frames */
>  static struct ethhdr udp4_eth_hdr;
>  
> -/**
> - * 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
> - * @iph:	Pre-filled IP header (except for tot_len and saddr)
> - */
> -static struct udp4_l2_buf_t {
> -	struct sockaddr_in s_in;
> -
> -	struct tap_hdr taph;
> -	struct iphdr iph;
> -} __attribute__ ((packed, aligned(__alignof__(unsigned int))))
> -udp4_l2_buf[UDP_MAX_FRAMES];
> -
>  /* Ethernet header for IPv6 frames */
>  static struct ethhdr udp6_eth_hdr;
>  
>  /**
> - * udp6_l2_buf_t - Pre-cooked IPv6 packet buffers for tap connections
> - * @s_in6:	Source socket address, filled in by recvmmsg()
> + * udp_meta - Pre-cooked headers and metadata for UDP packets

Pre-existing, but... kernel-doc format wants *struct* x - Description.

> + * @ip6h:	Pre-filled IPv6 header (except for payload_len and addresses)
> + * @ip4h:	Pre-filled IPv4 header (except for tot_len and saddr)
>   * @taph:	Tap backend specific header
> - * @ip6h:	Pre-filled IP header (except for payload_len and addresses)
> + * @s_in:	Source socket address, filled in by recvmmsg()
>   */
> -struct udp6_l2_buf_t {
> -	struct sockaddr_in6 s_in6;
> -#ifdef __AVX2__
> -	/* Align ip6h to 32-byte boundary. */
> -	uint8_t pad[64 - (sizeof(struct sockaddr_in6) + sizeof(struct tap_hdr))];
> -#endif
> -
> -	struct tap_hdr taph;
> +static struct udp_meta {
>  	struct ipv6hdr ip6h;
> +	struct iphdr ip4h;
> +	struct tap_hdr taph;
> +
> +	union sockaddr_inany s_in;
> +}
>  #ifdef __AVX2__
> -} __attribute__ ((packed, aligned(32)))
> -#else
> -} __attribute__ ((packed, aligned(__alignof__(unsigned int))))
> +__attribute__ ((aligned(32)))
>  #endif
> -udp6_l2_buf[UDP_MAX_FRAMES];
> +udp_meta_buf[UDP_MAX_FRAMES];
>  
>  /**
>   * enum udp_iov_idx - Indices for the buffers making up a single UDP frame
> @@ -315,49 +298,46 @@ 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 udp_meta *meta = &udp_meta_buf[i];
>  	struct iovec *siov = &udp_l2_iov_sock[i];
>  
> +	*meta = (struct udp_meta) {
> +		.ip4h = L2_BUF_IP4_INIT(IPPROTO_UDP),
> +		.ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP),
> +	};
> +
> +

One extra newline should be enough.

-- 
Stefano