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=202410 header.b=mlLe18bs; dkim-atps=neutral Received: from mail.ozlabs.org (gandalf.ozlabs.org [150.107.74.76]) by passt.top (Postfix) with ESMTPS id 46CFB5A004E for ; Mon, 04 Nov 2024 03:30:05 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1730687395; bh=NKvduJUofVeVUCqha1IncZEMN7il61ZjQQgujRgwbjk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=mlLe18bsP0WUJdamTVwLCL2/AU1iVk3X0/J2yuG9KtLGR0QyF1OZe9oLcExRYzK6H fq02kYgDaDM1SX3uqdAg9cMfHmt/qUERbzBPLGvJIKPaccurpbxPaqNt65M5QA16oE 4fETFtx5DB4UgFDvA/ultbV+UzlVTVZUthV5tJ/LuIa1voU6t6GAipucLeABcvwY+q 99E98/xQmF5aS0S0DXJb/xF5Cgv1frBPYkPfIZlpVLyDq1qQBHCu4i7pPSroMiS508 FqJpzyaRIS7LxJij6ALk20xNjzKcLF/i9iCMscvZAgUOdIEyzNUGZOJhZkWQox+/IX bC9bUlKkF2uJA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Xhb5v3wGrz4x7G; Mon, 4 Nov 2024 13:29:55 +1100 (AEDT) Date: Mon, 4 Nov 2024 13:29:53 +1100 From: David Gibson To: Jon Maloy Subject: Re: [PATCH v2] tcp: unify payload and flags l2 frames array Message-ID: References: <20241102005132.1408340-1-jmaloy@redhat.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="mws5S9Ez6wyZfv2A" Content-Disposition: inline In-Reply-To: Message-ID-Hash: 2DFKHUNQPX6UQKGBQPAM2JQOPUAGWU7P X-Message-ID-Hash: 2DFKHUNQPX6UQKGBQPAM2JQOPUAGWU7P 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: --mws5S9Ez6wyZfv2A Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 04, 2024 at 12:16:58PM +1100, David Gibson wrote: > On Fri, Nov 01, 2024 at 08:51:32PM -0400, Jon Maloy wrote: > > In order to reduce static 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 > > Performance measurements with iperf3, where we force all > > traffic via the tap queue, show no significant difference: Spotted a couple of extra things I missed in my previous mail. [snip] > > @@ -107,13 +96,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]; > > Because you're now setting all the flags fields for every frame, you should remove the initialisation of th.ack from iov_init. [snip] > > @@ -274,6 +237,9 @@ static void tcp_data_to_tap(const struct ctx *c, st= ruct 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; > > + *(flags) =3D PAYLOAD_FLAGS; >=20 > I think this is likely to run afoul of TBAA rules, which could cause > miscompiles because cc assumes *flags is not aliased with payload->th. > Although it's more bulky, I think it's better to explicitly assign > each of the fields in struct tcphdr. The compiler should be able to > boil it down to the same instructions in any case. >=20 > Note that now we're using the netinet version of struct tcphdr, it has > an anonymous union so you can use th_flags, rather than the one bit > fields (doesn't include doffset, though). Actually, looking at this again, you shouldn't need to update doff in any case - it's the same for flag frames and data frames. Of course, it's plausible there's no point pre-filling the doff (or any other) header fields, but I don't think changing that should be in scope for this patch. I hope to post this afternoon a bunch of different buffer cleanups, along with vhost rebased on those and other changes. Unfortunately those will conflict with this patch, sorry. --=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 --mws5S9Ez6wyZfv2A Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmcoMaAACgkQzQJF27ox 2Gc7Zw/9HfcWhzcMf21HqvrYEcBlxlnpP2gPxJnZJdJIrmtjx9T2qJwzMMWgoSnn y8781ZonMCuV3oqFFJ9ysZoXmHt17lOjp78sTxLyqiATwrzuxVnsWf6AoNpjrU1H xgOAz2KV8h/IQH7dmVWf51ollJfdi2R9HrjBRRLWxwUZ4ERqM6diw1EqBPoWwnJH MilMMP2KvzbWWFqF7p+0pMLsArYkROd5+E3AwFnwb4OHhC0HsGJw52tZ9fjgIzGe dStr0DYQ1pHcdViOHKqjy+xl0gjE5xOK2sHCnpwrAp+s4L/qJ5sNyQXlK/bTAhJV T+AHMICEEt/eUK9HOhkOQtHKefltuFM/XYUBANLtDdcO7ePOOe0EywAb+j5TjXZx PhfeDeK8XijS55ZIs7Dm/0P6X3b7tf5o3uFLh+5PMJoeZ3AGs0fTzCyoQS8xDYpx V6y0mBGnf8y0cRVfL/8O4/NwncU7SEenkmOF5and6GHHkuD56iNxwCOeY7P+nobj 2MpUSPiUNGo680sR8/NxznpcfTWxmPl+ZrJS0n/RXgBz5hvCK+NRAwYoc4Nj1asr O+sOoZ1lnFmz3dOVZDRF4vaGGbCZkZa0f+BX6JXLcG+2q3YY4BX0kslW/gTvqon3 xxiwOUEMKRHIYxEwy1oZA5OEbAwvEaqdP9sJB+y7c0o0seoR4Sk= =8Qdd -----END PGP SIGNATURE----- --mws5S9Ez6wyZfv2A--