From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: passt.top; dmarc=none (p=none dis=none) header.from=gibson.dropbear.id.au Authentication-Results: passt.top; dkim=pass (2048-bit key; secure) header.d=gibson.dropbear.id.au header.i=@gibson.dropbear.id.au header.a=rsa-sha256 header.s=202408 header.b=X06Ug8Rt; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 796F95A004E for ; Mon, 23 Sep 2024 05:44:31 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1727063054; bh=BN2s+89IuYSb8X++hfrntFqIb3K0bO8KVe5iroiY6cs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=X06Ug8RtmGpcBqwkFo3D41BQHCldBN4oPAGX90JdWN69vyGj7ffxroN7BoY9/efgI xOt57OCITyrlglgVKynKdzbamGL/r99aJeE6t+FfM7LGAJEOZIs6Os5lVX0wrIXaGH nxEdWYEI7huUIRhprgHrTn6YgHHQTm06TlMLQxPwDv7MLdpLcdcehY7IBdyGXtAEje ISHAT+2JDnVbzZR5QARoCWS7jkv7sSm1nw9H5/8DOegxYX9PfIQ2ZfKCmNlAafB4ug t6hZZ6/4MovIC1W1S0o09Pq1gX1TagNldvvcSbpQC09Qac7TKny7zNPlnBsKoAJ+X7 cl363yHl8x6JA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XBpl2671pz4wj1; Mon, 23 Sep 2024 13:44:14 +1000 (AEST) Date: Mon, 23 Sep 2024 13:43:55 +1000 From: David Gibson To: Laurent Vivier Subject: Re: [PATCH] tcp: Use tcp_payload_t rather than tcphdr Message-ID: References: <20240920095631.3911625-1-lvivier@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="3/JRSZHH8a5exO6t" Content-Disposition: inline In-Reply-To: <20240920095631.3911625-1-lvivier@redhat.com> Message-ID-Hash: CCXAKO6KDSXD77UEHJHXVGML5P64EPQI X-Message-ID-Hash: CCXAKO6KDSXD77UEHJHXVGML5P64EPQI X-MailFrom: dgibson@gandalf.ozlabs.org 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: --3/JRSZHH8a5exO6t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Sep 20, 2024 at 11:56:31AM +0200, Laurent Vivier wrote: > As tcp_update_check_tcp4() and tcp_update_check_tcp6() compute the > checksum using the TCP header and the TCP payload, it is clearer > to use a pointer to tcp_payload_t that includes tcphdr and payload > rather than a pointer to tcphdr (and guessing TCP header is > followed by the payload). >=20 > Move tcp_payload_t and tcp_flags_t to tcp_internal.h. > (They will be used also by vhost-user). >=20 > Signed-off-by: Laurent Vivier Reviewed-by: David Gibson Of course, it will conflict with Jon's pending changes merging the payload and flags arrays. > --- > tcp.c | 42 ++++++++++++++++++++++-------------------- > tcp_buf.c | 29 ----------------------------- > tcp_internal.h | 29 +++++++++++++++++++++++++++++ > 3 files changed, 51 insertions(+), 49 deletions(-) >=20 > diff --git a/tcp.c b/tcp.c > index 1962fcc469ed..c9472d905520 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -757,32 +757,34 @@ static void tcp_sock_set_bufsize(const struct ctx *= c, int s) > /** > * tcp_update_check_tcp4() - Update TCP checksum from stored one > * @iph: IPv4 header > - * @th: TCP header followed by TCP payload > + * @bp: TCP header followed by TCP payload > */ > -static void tcp_update_check_tcp4(const struct iphdr *iph, struct tcphdr= *th) > +static void tcp_update_check_tcp4(const struct iphdr *iph, > + struct tcp_payload_t *bp) > { > uint16_t l4len =3D ntohs(iph->tot_len) - sizeof(struct iphdr); > struct in_addr saddr =3D { .s_addr =3D iph->saddr }; > struct in_addr daddr =3D { .s_addr =3D iph->daddr }; > uint32_t sum =3D proto_ipv4_header_psum(l4len, IPPROTO_TCP, saddr, dadd= r); > =20 > - th->check =3D 0; > - th->check =3D csum(th, l4len, sum); > + bp->th.check =3D 0; > + bp->th.check =3D csum(bp, l4len, sum); > } > =20 > /** > * tcp_update_check_tcp6() - Calculate TCP checksum for IPv6 > * @ip6h: IPv6 header > - * @th: TCP header followed by TCP payload > + * @bp: TCP header followed by TCP payload > */ > -static void tcp_update_check_tcp6(struct ipv6hdr *ip6h, struct tcphdr *t= h) > +static void tcp_update_check_tcp6(const struct ipv6hdr *ip6h, > + struct tcp_payload_t *bp) > { > uint16_t l4len =3D ntohs(ip6h->payload_len); > uint32_t sum =3D proto_ipv6_header_psum(l4len, IPPROTO_TCP, > &ip6h->saddr, &ip6h->daddr); > =20 > - th->check =3D 0; > - th->check =3D csum(th, l4len, sum); > + bp->th.check =3D 0; > + bp->th.check =3D csum(bp, l4len, sum); > } > =20 > /** > @@ -902,7 +904,7 @@ static void tcp_fill_header(struct tcphdr *th, > * @conn: Connection pointer > * @taph: tap backend specific header > * @iph: Pointer to IPv4 header > - * @th: Pointer to TCP header > + * @bp: Pointer to TCP header followed by TCP payload > * @dlen: TCP payload length > * @check: Checksum, if already known > * @seq: Sequence number for this segment > @@ -912,14 +914,14 @@ static void tcp_fill_header(struct tcphdr *th, > */ > static size_t tcp_fill_headers4(const struct tcp_tap_conn *conn, > struct tap_hdr *taph, > - struct iphdr *iph, struct tcphdr *th, > + struct iphdr *iph, struct tcp_payload_t *bp, > size_t dlen, const uint16_t *check, > uint32_t seq, bool no_tcp_csum) > { > const struct flowside *tapside =3D TAPFLOW(conn); > const struct in_addr *src4 =3D inany_v4(&tapside->oaddr); > const struct in_addr *dst4 =3D inany_v4(&tapside->eaddr); > - size_t l4len =3D dlen + sizeof(*th); > + size_t l4len =3D dlen + sizeof(bp->th); > size_t l3len =3D l4len + sizeof(*iph); > =20 > ASSERT(src4 && dst4); > @@ -931,12 +933,12 @@ static size_t tcp_fill_headers4(const struct tcp_ta= p_conn *conn, > iph->check =3D check ? *check : > csum_ip4_header(l3len, IPPROTO_TCP, *src4, *dst4); > =20 > - tcp_fill_header(th, conn, seq); > + tcp_fill_header(&bp->th, conn, seq); > =20 > if (no_tcp_csum) > - th->check =3D 0; > + bp->th.check =3D 0; > else > - tcp_update_check_tcp4(iph, th); > + tcp_update_check_tcp4(iph, bp); > =20 > tap_hdr_update(taph, l3len + sizeof(struct ethhdr)); > =20 > @@ -948,7 +950,7 @@ static size_t tcp_fill_headers4(const struct tcp_tap_= conn *conn, > * @conn: Connection pointer > * @taph: tap backend specific header > * @ip6h: Pointer to IPv6 header > - * @th: Pointer to TCP header > + * @bp: Pointer to TCP header followed by TCP payload > * @dlen: TCP payload length > * @check: Checksum, if already known > * @seq: Sequence number for this segment > @@ -958,11 +960,11 @@ static size_t tcp_fill_headers4(const struct tcp_ta= p_conn *conn, > */ > static size_t tcp_fill_headers6(const struct tcp_tap_conn *conn, > struct tap_hdr *taph, > - struct ipv6hdr *ip6h, struct tcphdr *th, > + struct ipv6hdr *ip6h, struct tcp_payload_t *bp, > size_t dlen, uint32_t seq, bool no_tcp_csum) > { > const struct flowside *tapside =3D TAPFLOW(conn); > - size_t l4len =3D dlen + sizeof(*th); > + size_t l4len =3D dlen + sizeof(bp->th); > =20 > ip6h->payload_len =3D htons(l4len); > ip6h->saddr =3D tapside->oaddr.a6; > @@ -976,12 +978,12 @@ static size_t tcp_fill_headers6(const struct tcp_ta= p_conn *conn, > ip6h->flow_lbl[1] =3D (conn->sock >> 8) & 0xff; > ip6h->flow_lbl[2] =3D (conn->sock >> 0) & 0xff; > =20 > - tcp_fill_header(th, conn, seq); > + tcp_fill_header(&bp->th, conn, seq); > =20 > if (no_tcp_csum) > - th->check =3D 0; > + bp->th.check =3D 0; > else > - tcp_update_check_tcp6(ip6h, th); > + tcp_update_check_tcp6(ip6h, bp); > =20 > tap_hdr_update(taph, l4len + sizeof(*ip6h) + sizeof(struct ethhdr)); > =20 > diff --git a/tcp_buf.c b/tcp_buf.c > index ffbff5e4b485..238827b01d90 100644 > --- a/tcp_buf.c > +++ b/tcp_buf.c > @@ -38,35 +38,6 @@ > (c->mode =3D=3D MODE_PASTA ? 1 : TCP_FRAMES_MEM) > =20 > /* Static buffers */ > -/** > - * struct tcp_payload_t - TCP header and data to send segments with payl= oad > - * @th: TCP header > - * @data: TCP data > - */ > -struct tcp_payload_t { > - struct tcphdr th; > - uint8_t data[IP_MAX_MTU - sizeof(struct tcphdr)]; > -#ifdef __AVX2__ > -} __attribute__ ((packed, aligned(32))); /* For AVX2 checksum routine= s */ > -#else > -} __attribute__ ((packed, aligned(__alignof__(unsigned int)))); > -#endif > - > -/** > - * struct tcp_flags_t - TCP header and data to send zero-length > - * segments (flags) > - * @th: TCP header > - * @opts TCP options > - */ > -struct tcp_flags_t { > - struct tcphdr th; > - char opts[OPT_MSS_LEN + OPT_WS_LEN + 1]; > -#ifdef __AVX2__ > -} __attribute__ ((packed, aligned(32))); > -#else > -} __attribute__ ((packed, aligned(__alignof__(unsigned int)))); > -#endif > - > /* Ethernet header for IPv4 frames */ > static struct ethhdr tcp4_eth_src; > =20 > diff --git a/tcp_internal.h b/tcp_internal.h > index de06db1438d6..2f74ffeff8f3 100644 > --- a/tcp_internal.h > +++ b/tcp_internal.h > @@ -63,6 +63,35 @@ enum tcp_iov_parts { > TCP_NUM_IOVS > }; > =20 > +/** > + * struct tcp_payload_t - TCP header and data to send segments with payl= oad > + * @th: TCP header > + * @data: TCP data > + */ > +struct tcp_payload_t { > + struct tcphdr th; > + uint8_t data[IP_MAX_MTU - sizeof(struct tcphdr)]; > +#ifdef __AVX2__ > +} __attribute__ ((packed, aligned(32))); /* For AVX2 checksum routine= s */ > +#else > +} __attribute__ ((packed, aligned(__alignof__(unsigned int)))); > +#endif > + > +/** > + * struct tcp_flags_t - TCP header and data to send zero-length > + * segments (flags) > + * @th: TCP header > + * @opts TCP options > + */ > +struct tcp_flags_t { > + struct tcphdr th; > + char opts[OPT_MSS_LEN + OPT_WS_LEN + 1]; > +#ifdef __AVX2__ > +} __attribute__ ((packed, aligned(32))); > +#else > +} __attribute__ ((packed, aligned(__alignof__(unsigned int)))); > +#endif > + > extern char tcp_buf_discard [MAX_WINDOW]; > =20 > void conn_flag_do(const struct ctx *c, struct tcp_tap_conn *conn, --=20 David Gibson (he or they) | 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 --3/JRSZHH8a5exO6t Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmbw4/oACgkQzQJF27ox 2GeN8xAAnV4rxKQ3KlCRdId0R16BXs1Mp5ZbYDv236VGExPOrHTZTFUgrx7bImHC Id8w1ASwZUZt6hz5IZoSCICG9G2BAaMUGEb5aYgvhgOTorTM2WAmPXXjgYZ6nRiC WJgmnijZwa8BQOK8jBelYIzFvcXGtcqToTz4+mPkA7e8nsMrUN5YzwZuOWG3mANz t0GnPj++ULCwmqMt8rYN1EJk3EiyQw5oCQBRvBJZun4V+Vi3qsAkbQTXJ/btnP2m EpBCrKJsOxDUN9FOiQKjI7SZkxl6waK1dOY/4SWboNW11IJeJRvV9l0vpxnDYcko WOxMoIfg5bmxVml1S6yonla9lzeJsgY4CGs+KBVIGeOiJWT+GcnJ7O49DoMj12Zk AdhCYkWNwlQGXexQvmBVJ2szNM8XuT1DOJXVIJPHnhGDsh+8s/SWeNnpu4ABPoyG DmDtmI8nHxRjW3rNTiCGyLRIG+92j8Cw2QvXCZ7qFlTFOXum2nm/KtOcpYu1dNP8 DYf0/UBWaJIW6w10UfOmkkUTj7KlYW6y+959iLUGqyggPFNX9nkFOZvobiboC+5L g2X32aMc7Rx4W71MliVIH3+d/Nwzk5pqUaNz3128CQE4dq9u/9ZcOccWgTFRp6Od zcrWzaJ2/BIo4CMQEq/LRtPGp98DganLeW5Oz6uxIEcWMq+EcCI= =WVuf -----END PGP SIGNATURE----- --3/JRSZHH8a5exO6t--