From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id A05425A02D1 for ; Tue, 30 Apr 2024 22:16:45 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1714508204; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=NnwU2f/9kKzO2gWKLkTreYtEyNZSbvrpus3Nd1ITsCs=; b=XoIEw3dPO8HEz1nQX6yBCeindH5uyXVnYJblzSRphWaaeGkD2t0lAgPUzQcK/U9b1/KA5w 34S9f6hvczMWXh1wBEdq8dHwLQtUOvG5EAUQTUlE/Ram8H7bKCtgu8tNhlpZv6/Xt0QA+R Tp+iDGCj+7OXndvX2YdFnp9ztRxBG6c= Received: from mail-lj1-f198.google.com (mail-lj1-f198.google.com [209.85.208.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-227-ATIn-kWJNvOXwmrThOhmww-1; Tue, 30 Apr 2024 16:16:42 -0400 X-MC-Unique: ATIn-kWJNvOXwmrThOhmww-1 Received: by mail-lj1-f198.google.com with SMTP id 38308e7fff4ca-2d871086a09so54394811fa.1 for ; Tue, 30 Apr 2024 13:16:42 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1714508199; x=1715112999; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=NnwU2f/9kKzO2gWKLkTreYtEyNZSbvrpus3Nd1ITsCs=; b=KnvyDRFBK5IuPqg+Lsbox/WCr+b+VSGLFAsivGFfPrs2f1pD/jYuCwHgt+KkR3+/Rv HEksRcFO1rrUCypbNiqgQm2pdlPbdr239hPXfxgkPapZglTCcsd9sau+emK3bJgO5xvj 6M4096gWGEtLEzHmQ402jvPO7HpWFYk4wiCCNxEdXPnrktp51Pb48ZlHe9I4Gm1mUz7b xwTi0NvNomrog1hq/s2FIithPp+5fJxW3dJXsFrHVPslz9MUbJsL/iOfFKHd//s3kxkN QtQbTIQQ7d+W90gjbMdu5IC/P6ciI9mwJNbJ+72XodMJsCo6no+FCIZZ9yAaJDAfV/0s Ysvw== X-Gm-Message-State: AOJu0YxVrvAclJqNnP5u1YQL7Asb/OW8n7p37gUgFDiJDkG5C5aTVUUc 57OMY8P2fUTO3oiDSNx483zo8IE3Dik1b6ACZ/kBG0QUV1GhF7tGlV/WPFB+7VcW0EEwkPma1w8 NMPt7gLfVbpuWhRt19v/JRR+mWRGT13/7hhPqQukpRS249Vnl44yKNOwv0wdO X-Received: by 2002:a05:6512:3193:b0:51e:7e65:1060 with SMTP id i19-20020a056512319300b0051e7e651060mr354701lfe.67.1714508198653; Tue, 30 Apr 2024 13:16:38 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHoE4bDnhbT6/6SzzGHPelekx7VkWMoBfwNcZBT4GG2Boaq6Y2tyPPUPXG2mv38Ng36BdjKDA== X-Received: by 2002:a05:6512:3193:b0:51e:7e65:1060 with SMTP id i19-20020a056512319300b0051e7e651060mr354687lfe.67.1714508197984; Tue, 30 Apr 2024 13:16:37 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id s17-20020a1709067b9100b00a51bf5932aesm15433424ejo.28.2024.04.30.13.16.37 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 30 Apr 2024 13:16:37 -0700 (PDT) Date: Tue, 30 Apr 2024 22:16:04 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 5/7] udp: Share payload buffers between IPv4 and IPv6 Message-ID: <20240430221604.247fb543@elisabeth> In-Reply-To: <20240430100541.381350-6-david@gibson.dropbear.id.au> References: <20240430100541.381350-1-david@gibson.dropbear.id.au> <20240430100541.381350-6-david@gibson.dropbear.id.au> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.36; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: ZEFUTND7BIE6UQQPNZJFR2UVQD3THEQ2 X-Message-ID-Hash: ZEFUTND7BIE6UQQPNZJFR2UVQD3THEQ2 X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Tue, 30 Apr 2024 20:05:39 +1000 David Gibson wrote: > 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 > --- > 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'. > + > /** > * 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? -- Stefano