On Tue, Apr 30, 2024 at 10:16:34PM +0200, Stefano Brivio wrote:
> 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?

No, I don't think so.  There's still a separate IPv4 and IPv6 header,
and socket address for each frame.  It's just that those are stored in
a parallel array, rather than alongside the paylaod buffers.

If we are handling multiple guests, we can probably additionally merge
the IPv4 and IPv6 header buffers, since I don't think there will be
anything we can pre-fill any more.

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

Adjusted.  I also changed to udp_meta_t and udp_meta[] for consistency
with the payload buffers.

> > + * @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.

Fixed.

-- 
David Gibson			| 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