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=eejCQMaG; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 517DD5A004E for ; Mon, 23 Sep 2024 04:04:28 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202408; t=1727057063; bh=3dzMEAnShKNRlaJGGqyTxx+JL9UIFg1yK2u9ZBQk5sU=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=eejCQMaGCW4sXoRyEyTBqFb52teIsQiu4soczFnP8rZmvNiVY6tZIn/b6tSlvpt0e pBsqJCHsmfHtzZ6/IaCvPcIn6oxqlNk9ZsZ0p86fx7uVNW+83Qh1/Oxm6hrobGp34f fZtjt5pLAFh/DtxQyi7C5fphDDPUXLamwKk/3Zn6YNe8Dpz6iggjVghsXteJd+C8rW 7eiNICCoK9zZqH/GNfoYkQnsm+FxqhP+aKx5+du5iUNnN9VYfYNcbjLjSM+kdZ9vTJ sO/o5HLKv6hOZA5gVz4GvJAQwiylTF+8u9usC6HCGMlzevelBQuHq1Sqw73S2AxEj3 92NFQXLKZxj+g== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4XBmWq6gHqz4wj1; Mon, 23 Sep 2024 12:04:23 +1000 (AEST) Date: Mon, 23 Sep 2024 12:03:18 +1000 From: David Gibson To: Jon Maloy Subject: Re: [PATCH] tcp: unify payload and flags l2 frames array Message-ID: References: <20240921194333.3938826-1-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TAZJYn2pBQyhsYtM" Content-Disposition: inline In-Reply-To: <20240921194333.3938826-1-jmaloy@redhat.com> Message-ID-Hash: LD7JPLDOTJ3FEAPJE4CECXG2GZGY4ZCK X-Message-ID-Hash: LD7JPLDOTJ3FEAPJE4CECXG2GZGY4ZCK 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, sbrivio@redhat.com, lvivier@redhat.com, dgibson@redhat.com 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: --TAZJYn2pBQyhsYtM Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Sep 21, 2024 at 03:43:33PM -0400, Jon Maloy wrote: > In order to reduce memory and code footprint, we merge the array > for l2 flag frames into the one for payload frames. >=20 > This change also ensures that no flag message will be sent out > over the l2 media bypassing already queued payload messages. >=20 > Signed-off-by: Jon Maloy > --- > tcp.c | 3 +-- > tcp_buf.c | 70 ++++++++++++------------------------------------------- > 2 files changed, 16 insertions(+), 57 deletions(-) >=20 > diff --git a/tcp.c b/tcp.c > index 12e864f..dd060d0 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -868,7 +868,6 @@ bool tcp_flow_defer(const struct tcp_tap_conn *conn) > /* cppcheck-suppress [constParameterPointer, unmatchedSuppression] */ > void tcp_defer_handler(struct ctx *c) > { > - tcp_flags_flush(c); > tcp_payload_flush(c); As Stefano also noted, I think it would make more sense to just combine these into a single "tcp_flush()". > } > =20 > @@ -1148,7 +1147,7 @@ static void tcp_update_seqack_from_tap(const struct= ctx *c, > * 1 otherwise > */ > int tcp_prepare_flags(const struct ctx *c, struct tcp_tap_conn *conn, > - int flags, struct tcphdr *th, char *data, > + int flags, struct tcphdr *th, uint8_t *data, This seems like an unrelated change to the rest. > size_t *optlen) > { > struct tcp_info tinfo =3D { 0 }; > diff --git a/tcp_buf.c b/tcp_buf.c > index 625c29c..58636d1 100644 > --- a/tcp_buf.c > +++ b/tcp_buf.c > @@ -33,6 +33,7 @@ > #include "tcp_internal.h" > #include "tcp_buf.h" > =20 > +#define PAYLOAD_FLAGS htons(0x5010) /* doff =3D 5, ack =3D 1 */ > #define TCP_FRAMES_MEM 128 > #define TCP_FRAMES \ > (c->mode =3D=3D MODE_PASTA ? 1 : TCP_FRAMES_MEM) > @@ -52,21 +53,6 @@ struct tcp_payload_t { > } __attribute__ ((packed, aligned(__alignof__(unsigned int)))); > #endif > =20 > -/** > - * 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 and IPv6 frames */ > static struct ethhdr tcp4_eth_src; > static struct ethhdr tcp6_eth_src; > @@ -87,22 +73,10 @@ static_assert(MSS6 <=3D sizeof(tcp_payload[0].data), = "MSS6 is greater than 65516") > static struct tcp_tap_conn *tcp_frame_conns[TCP_FRAMES_MEM]; > static unsigned int tcp_payload_used; Similarly changing this just to "tcp_buf_used" or the like would make sense. > -static struct tap_hdr tcp_flags_tap_hdr[TCP_FRAMES_MEM]; > -/* IPv4 headers for TCP segment without payload */ > -static struct iphdr tcp4_flags_ip[TCP_FRAMES_MEM]; > -/* TCP segments without payload for IPv4 frames */ > -static struct tcp_flags_t tcp_flags[TCP_FRAMES_MEM]; > - > -static unsigned int tcp_flags_used; > - > -/* IPv6 headers for TCP segment without payload */ > -static struct ipv6hdr tcp6_flags_ip[TCP_FRAMES_MEM]; > - > /* recvmsg()/sendmsg() data for tap */ > static struct iovec iov_sock [TCP_FRAMES_MEM + 1]; > =20 > static struct iovec tcp_l2_iov[TCP_FRAMES_MEM][TCP_NUM_IOVS]; > -static struct iovec tcp_l2_flags_iov[TCP_FRAMES_MEM][TCP_NUM_IOVS]; > =20 > /** > * tcp_update_l2_buf() - Update Ethernet header buffers with addresses > @@ -135,13 +109,6 @@ void tcp_sock_iov_init(const struct ctx *c) > tcp_payload[i].th.ack =3D 1; > } > =20 > - for (i =3D 0; i < ARRAY_SIZE(tcp_flags); i++) { > - tcp6_flags_ip[i] =3D ip6; > - tcp4_flags_ip[i] =3D iph; > - tcp_flags[i].th.doff =3D sizeof(struct tcphdr) / 4; > - tcp_flags[i].th.ack =3D 1; > - } > - > for (i =3D 0; i < TCP_FRAMES_MEM; i++) { > struct iovec *iov =3D tcp_l2_iov[i]; > =20 > @@ -149,14 +116,6 @@ void tcp_sock_iov_init(const struct ctx *c) > iov[TCP_IOV_ETH].iov_len =3D sizeof(struct ethhdr); > iov[TCP_IOV_PAYLOAD].iov_base =3D &tcp_payload[i]; > } > - > - for (i =3D 0; i < TCP_FRAMES_MEM; i++) { > - struct iovec *iov =3D tcp_l2_flags_iov[i]; > - > - iov[TCP_IOV_TAP] =3D tap_hdr_iov(c, &tcp_flags_tap_hdr[i]); > - iov[TCP_IOV_ETH].iov_len =3D sizeof(struct ethhdr); > - iov[TCP_IOV_PAYLOAD].iov_base =3D &tcp_flags[i]; > - } > } > =20 > /** > @@ -165,9 +124,7 @@ void tcp_sock_iov_init(const struct ctx *c) > */ > void tcp_flags_flush(const struct ctx *c) > { > - tap_send_frames(c, &tcp_l2_flags_iov[0][0], TCP_NUM_IOVS, > - tcp_flags_used); > - tcp_flags_used =3D 0; > + tcp_payload_flush(c); I think tcp_flags_flush() can just be removed entirely. > } > =20 > /** > @@ -225,37 +182,35 @@ void tcp_payload_flush(const struct ctx *c) > */ > int tcp_buf_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, in= t flags) > { > - struct tcp_flags_t *payload; > + struct tcp_payload_t *payload; > struct iovec *iov; > size_t optlen; > size_t l4len; > uint32_t seq; > int ret; > =20 > - iov =3D tcp_l2_flags_iov[tcp_flags_used]; > + iov =3D tcp_l2_iov[tcp_payload_used]; > if (CONN_V4(conn)) { > - iov[TCP_IOV_IP] =3D IOV_OF_LVALUE(tcp4_flags_ip[tcp_flags_used]); > + iov[TCP_IOV_IP] =3D IOV_OF_LVALUE(tcp4_payload_ip[tcp_payload_used]); > iov[TCP_IOV_ETH].iov_base =3D &tcp4_eth_src; > } else { > - iov[TCP_IOV_IP] =3D IOV_OF_LVALUE(tcp6_flags_ip[tcp_flags_used]); > + iov[TCP_IOV_IP] =3D IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_used]); > iov[TCP_IOV_ETH].iov_base =3D &tcp6_eth_src; > } > =20 > payload =3D iov[TCP_IOV_PAYLOAD].iov_base; > seq =3D conn->seq_to_tap; > ret =3D tcp_prepare_flags(c, conn, flags, &payload->th, > - payload->opts, &optlen); > + payload->data, &optlen); > if (ret <=3D 0) > return ret; > =20 > - tcp_flags_used++; > + tcp_payload_used++; > l4len =3D tcp_l2_buf_fill_headers(conn, iov, optlen, NULL, seq, false); > iov[TCP_IOV_PAYLOAD].iov_len =3D l4len; > - > if (flags & DUP_ACK) { > - struct iovec *dup_iov; > + struct iovec *dup_iov =3D tcp_l2_iov[tcp_payload_used++]; > =20 > - dup_iov =3D tcp_l2_flags_iov[tcp_flags_used++]; > memcpy(dup_iov[TCP_IOV_TAP].iov_base, iov[TCP_IOV_TAP].iov_base, > iov[TCP_IOV_TAP].iov_len); > dup_iov[TCP_IOV_ETH].iov_base =3D iov[TCP_IOV_ETH].iov_base; > @@ -265,7 +220,7 @@ int tcp_buf_send_flag(const struct ctx *c, struct tcp= _tap_conn *conn, int flags) > dup_iov[TCP_IOV_PAYLOAD].iov_len =3D l4len; > } > =20 > - if (tcp_flags_used > TCP_FRAMES_MEM - 2) > + if (tcp_payload_used > TCP_FRAMES_MEM - 2) > tcp_flags_flush(c); > =20 > return 0; > @@ -282,8 +237,10 @@ int tcp_buf_send_flag(const struct ctx *c, struct tc= p_tap_conn *conn, int flags) > static void tcp_data_to_tap(const struct ctx *c, struct tcp_tap_conn *co= nn, > ssize_t dlen, int no_csum, uint32_t seq) > { > + struct tcp_payload_t *payload; > const uint16_t *check =3D NULL; > struct iovec *iov; > + uint16_t *flags; > size_t l4len; > =20 > conn->seq_to_tap =3D seq + dlen; > @@ -302,6 +259,9 @@ static void tcp_data_to_tap(const struct ctx *c, stru= ct tcp_tap_conn *conn, > iov[TCP_IOV_IP] =3D IOV_OF_LVALUE(tcp6_payload_ip[tcp_payload_used]); > iov[TCP_IOV_ETH].iov_base =3D &tcp6_eth_src; > } > + payload =3D iov[TCP_IOV_PAYLOAD].iov_base; > + flags =3D &payload->th.window - 1; I think this will run afoul of the C strict aliasing rules (they're disabled in the kernel, but not for us). > + *(flags) =3D PAYLOAD_FLAGS; > l4len =3D tcp_l2_buf_fill_headers(conn, iov, dlen, check, seq, false); > iov[TCP_IOV_PAYLOAD].iov_len =3D l4len; > if (++tcp_payload_used > TCP_FRAMES_MEM - 1) --=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 --TAZJYn2pBQyhsYtM Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmbwzGEACgkQzQJF27ox 2Ge7Aw/+JJf+7i+B6blWVKUs5S1qF6QUJ8I1ci9ltK5GytlJ41COumfIVsZrCKtn XlKANwNZfffmsuzjn310Oc4V6vSjJcFkALs69ju/9C2t82iq6OgrsTbrqwcfsT+X 4+uXaO9v5/lrkqMU4v4Cxyju5abt/J6xw6FvMWVMbfa60Rp9RK/q9VMKN4K/gYVG QAbCAzyGX6aVXc7cyOFPpqYCvtvXbUvrmrW58U9Co9b8mNr5BGqG674Q06CehD2d oBKZY3l+vqZ2z18S8odI/Kcspln8eOq+4GTI+8tG6iT+LJBFUtu45U7B0wK4hE56 cd17PUmzScdogscWM0jUc5ab1QPxV+nwe4g6vwzoY63Rm8stJKrpkaG/xqXyLLxb Yayj1i332ZeRaKiSmoPENe+oqLgk1CsktwdT5jRTP7XUQr9Z09CRTlUWCQeSp2gL K27YMG0hwenr9ohACDqdFolaFFZcm4TIQPcJYUTNzN1KPfJSx36oqkiILvunRHmm sT+XjWdbg9g0LAtNdyFn2mekdya20cYqdB63IQ6Mda44uwQh1EEJspNqeYvZ5lw+ A78+AVNk+KJPUy3vYLi7UtcroS5B8WvoGP90ZribRwDPsPcINpNiaZN4xrMUqf/n 7olk2NfvWe2d8YOb5/n+J9xk5sLefbtuegLKKOe9mPJVVXBXFkM= =MZGB -----END PGP SIGNATURE----- --TAZJYn2pBQyhsYtM--