On Tue, Apr 30, 2024 at 10:16:04PM +0200, Stefano Brivio wrote:On Tue, 30 Apr 2024 20:05:39 +1000 David Gibson <david(a)gibson.dropbear.id.au> wrote:Amended as suggested.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(a)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'.Ah, yes, I think that's a leftover from some interim version. Spurious change removed. -- 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+ /** * 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?