public inbox for passt-dev@passt.top
 help / color / mirror / code / Atom feed
From: Stefano Brivio <sbrivio@redhat.com>
To: David Gibson <david@gibson.dropbear.id.au>
Cc: passt-dev@passt.top, Laurent Vivier <lvivier@redhat.com>
Subject: Re: [PATCH 2/7] tap: Split tap specific and L2 (ethernet) headers
Date: Tue, 30 Apr 2024 20:46:29 +0200	[thread overview]
Message-ID: <20240430204629.1f0cc9c6@elisabeth> (raw)
In-Reply-To: <20240429070933.1366881-3-david@gibson.dropbear.id.au>

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.

> -#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));

-- 
Stefano


  reply	other threads:[~2024-04-30 18:47 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-29  7:09 [PATCH 0/7] Small improvements to IOV handling David Gibson
2024-04-29  7:09 ` [PATCH 1/7] checksum: Use proto_ipv6_header_psum() for ICMPv6 as well David Gibson
2024-04-29  7:09 ` [PATCH 2/7] tap: Split tap specific and L2 (ethernet) headers David Gibson
2024-04-30 18:46   ` Stefano Brivio [this message]
2024-04-30 23:53     ` David Gibson
2024-04-29  7:09 ` [PATCH 3/7] treewide: Standardise variable names for various packet lengths David Gibson
2024-04-30 18:46   ` Stefano Brivio
2024-05-01  0:05     ` David Gibson
2024-04-29  7:09 ` [PATCH 4/7] tcp: Simplify packet length calculation when preparing headers David Gibson
2024-04-29  7:09 ` [PATCH 5/7] tap, tcp: (Re-)abstract TAP specific header handling David Gibson
2024-04-30 18:47   ` Stefano Brivio
2024-05-01  0:06     ` David Gibson
2024-04-29  7:09 ` [PATCH 6/7] iov: Helper macro to construct iovs covering existing variables or fields David Gibson
2024-04-30 18:47   ` Stefano Brivio
2024-05-01  0:09     ` David Gibson
2024-04-29  7:09 ` [PATCH 7/7] tcp: Update tap specific header too in tcp_fill_headers[46]() David Gibson
2024-04-30 18:48   ` Stefano Brivio
2024-05-01  0:10     ` David Gibson

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20240430204629.1f0cc9c6@elisabeth \
    --to=sbrivio@redhat.com \
    --cc=david@gibson.dropbear.id.au \
    --cc=lvivier@redhat.com \
    --cc=passt-dev@passt.top \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://passt.top/passt

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for IMAP folder(s).