Re: [PATCH 2/7] tap: Split tap specific and L2 (ethernet) headers

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

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

On Tue, Apr 30, 2024 at 08:46:29PM +0200, Stefano Brivio wrote:
> On Mon, 29 Apr 2024 17:09:28 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > In some places (well, actually only UDP now) we use struct tap_hdr to
> > represent both tap backend specific and L2 ethernet headers.  Handling
> > these together seemed like a good idea at the time, but Laurent's changes
> > in the TCP code working towards vhost-user support suggest that treating
> > them separately is more useful, more often.
> > 
> > Alter struct tap_hdr to represent only the TAP backend specific headers.
> > Updated related helpers and the UDP code to match.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tap.h | 21 +++++++++------------
> >  udp.c | 23 ++++++++++++++---------
> >  2 files changed, 23 insertions(+), 21 deletions(-)
> > 
> > diff --git a/tap.h b/tap.h
> > index 2adc4e2b..dbc23b31 100644
> > --- a/tap.h
> > +++ b/tap.h
> > @@ -6,30 +6,28 @@
> >  #ifndef TAP_H
> >  #define TAP_H
> >  
> > +#define ETH_HDR_INIT(proto) { .h_proto = htons_constant(proto) }
> > +
> >  /**
> > - * struct tap_hdr - L2 and tap specific headers
> > + * struct tap_hdr - tap backend specific headers
> >   * @vnet_len:	Frame length (for qemu socket transport)
> > - * @eh:		Ethernet header
> >   */
> >  struct tap_hdr {
> >  	uint32_t vnet_len;
> > -	struct ethhdr eh;
> >  } __attribute__((packed));
> 
> No need to have it packed anymore.

I'd arguably that conceptually it should still be packed.  This is
still expected to be a contiguous block of bytes, laid out in a
specific way.  Of course, since it's just a single u32, ((packed))
doesn't have any effect, but that could change if we need any
different tap-specific headers in future.

> > -#define TAP_HDR_INIT(proto) { .eh.h_proto = htons_constant(proto) }
> > -
> >  static inline size_t tap_hdr_len_(const struct ctx *c)
> >  {
> >  	if (c->mode == MODE_PASST)
> >  		return sizeof(struct tap_hdr);
> >  	else
> > -		return sizeof(struct ethhdr);
> > +		return 0;
> >  }
> >  
> >  /**
> >   * tap_frame_base() - Find start of tap frame
> >   * @c:		Execution context
> > - * @taph:	Pointer to L2 and tap specific header buffer
> > + * @taph:	Pointer to tap specific header buffer
> >   *
> >   * Returns: pointer to the start of tap frame - suitable for an
> >   *          iov_base to be passed to tap_send_frames())
> > @@ -43,17 +41,16 @@ static inline void *tap_frame_base(const struct ctx *c, struct tap_hdr *taph)
> >   * tap_frame_len() - Finalize tap frame and return total length
> >   * @c:		Execution context
> >   * @taph:	Tap header to finalize
> > - * @plen:	L3 packet length (excludes L2 and tap specific headers)
> > + * @plen:	L2 packet length (includes L2, excludes tap specific headers)
> >   *
> > - * Returns: length of the tap frame including L2 and tap specific
> > - *          headers - suitable for an iov_len to be passed to
> > - *          tap_send_frames()
> > + * Returns: length of the tap frame including tap specific headers - suitable
> > + *          for an iov_len to be passed to tap_send_frames()
> >   */
> >  static inline size_t tap_frame_len(const struct ctx *c, struct tap_hdr *taph,
> >  				   size_t plen)
> >  {
> >  	if (c->mode == MODE_PASST)
> > -		taph->vnet_len = htonl(plen + sizeof(taph->eh));
> > +		taph->vnet_len = htonl(plen);
> >  	return plen + tap_hdr_len_(c);
> >  }
> >  
> > diff --git a/udp.c b/udp.c
> > index 594ea191..c3e4f6b6 100644
> > --- a/udp.c
> > +++ b/udp.c
> > @@ -173,7 +173,8 @@ static uint8_t udp_act[IP_VERSIONS][UDP_ACT_TYPE_MAX][DIV_ROUND_UP(NUM_PORTS, 8)
> >  /**
> >   * udp4_l2_buf_t - Pre-cooked IPv4 packet buffers for tap connections
> >   * @s_in:	Source socket address, filled in by recvmmsg()
> > - * @taph:	Tap-level headers (partially pre-filled)
> > + * @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
> > @@ -182,6 +183,7 @@ static struct udp4_l2_buf_t {
> >  	struct sockaddr_in s_in;
> >  
> >  	struct tap_hdr taph;
> > +	struct ethhdr eh;
> >  	struct iphdr iph;
> >  	struct udphdr uh;
> >  	uint8_t data[USHRT_MAX -
> > @@ -192,7 +194,8 @@ udp4_l2_buf[UDP_MAX_FRAMES];
> >  /**
> >   * udp6_l2_buf_t - Pre-cooked IPv6 packet buffers for tap connections
> >   * @s_in6:	Source socket address, filled in by recvmmsg()
> > - * @taph:	Tap-level headers (partially pre-filled)
> > + * @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
> > @@ -202,10 +205,11 @@ struct udp6_l2_buf_t {
> >  #ifdef __AVX2__
> >  	/* Align ip6h to 32-byte boundary. */
> >  	uint8_t pad[64 - (sizeof(struct sockaddr_in6) + sizeof(struct ethhdr) +
> > -			  sizeof(uint32_t))];
> > +			  sizeof(struct tap_hdr))];
> >  #endif
> >  
> >  	struct tap_hdr taph;
> > +	struct ethhdr eh;
> >  	struct ipv6hdr ip6h;
> >  	struct udphdr uh;
> >  	uint8_t data[USHRT_MAX -
> > @@ -289,8 +293,8 @@ void udp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s)
> >  		struct udp4_l2_buf_t *b4 = &udp4_l2_buf[i];
> >  		struct udp6_l2_buf_t *b6 = &udp6_l2_buf[i];
> >  
> > -		eth_update_mac(&b4->taph.eh, eth_d, eth_s);
> > -		eth_update_mac(&b6->taph.eh, eth_d, eth_s);
> > +		eth_update_mac(&b4->eh, eth_d, eth_s);
> > +		eth_update_mac(&b6->eh, eth_d, eth_s);
> >  	}
> >  }
> >  
> > @@ -307,7 +311,7 @@ static void udp_sock4_iov_init_one(const struct ctx *c, size_t i)
> >  	struct iovec *tiov = &udp4_l2_iov_tap[i];
> >  
> >  	*buf = (struct udp4_l2_buf_t) {
> > -		.taph = TAP_HDR_INIT(ETH_P_IP),
> > +		.eh  = ETH_HDR_INIT(ETH_P_IP),
> >  		.iph = L2_BUF_IP4_INIT(IPPROTO_UDP)
> >  	};
> >  
> > @@ -335,7 +339,7 @@ static void udp_sock6_iov_init_one(const struct ctx *c, size_t i)
> >  	struct iovec *tiov = &udp6_l2_iov_tap[i];
> >  
> >  	*buf = (struct udp6_l2_buf_t) {
> > -		.taph = TAP_HDR_INIT(ETH_P_IPV6),
> > +		.eh   = ETH_HDR_INIT(ETH_P_IPV6),
> >  		.ip6h = L2_BUF_IP6_INIT(IPPROTO_UDP)
> >  	};
> >  
> > @@ -608,7 +612,7 @@ static size_t udp_update_hdr4(const struct ctx *c, struct udp4_l2_buf_t *b,
> >  	b->uh.dest = htons(dstport);
> >  	b->uh.len = htons(datalen + sizeof(b->uh));
> >  
> > -	return tap_frame_len(c, &b->taph, ip_len);
> > +	return tap_frame_len(c, &b->taph, ip_len + sizeof(b->eh));
> >  }
> >  
> >  /**
> > @@ -676,7 +680,8 @@ static size_t udp_update_hdr6(const struct ctx *c, struct udp6_l2_buf_t *b,
> >  	b->uh.len = b->ip6h.payload_len;
> >  	csum_udp6(&b->uh, src, dst, b->data, datalen);
> >  
> > -	return tap_frame_len(c, &b->taph, payload_len + sizeof(b->ip6h));
> > +	return tap_frame_len(c, &b->taph, payload_len + sizeof(b->ip6h)
> > +			     + sizeof(b->eh));
> 
> Nit: the + operator should be on the first line for consistency, and
> for readability I'd rather do:
> 
> 	return tap_frame_len(c, &b->taph, payload_len + sizeof(b->ip6h) +
> 							sizeof(b->eh));

Tweaked, thanks.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]