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=gE4nujrn; dkim-atps=neutral Received: from mail.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id B9C085A061A for ; Tue, 29 Oct 2024 10:34:08 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202410; t=1730194434; bh=NgUtmyEP1fxQh7YhvZltB2wZCSrn1vSRkyFxvFpTOss=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=gE4nujrnmXNDHLSo44XW2K9l7k1XqT2SbYt9DdIRC+rKsN2A95tgres4eSA+3Fi3x gxfqskjQyZHq0FUqs97vwMAFZbATiCI2704gjpy7s0qSE2/xl3pEt6lMpuUm8EF//P 5hiz0pqjX9DWW8NFyuBg/t0Nqx6seSdJsazZH/nR8xjRNFeKYrUOJ3yPDhXv/af66/ 23+/rXwLazD4ywOl91TxS2mHJJ3BZeKwVDysTOfxe8M+usGNP0yPLpUWQ3taptQV1x 372vuT6TA3mqdaPszUfLmZqn+5pc5Au14s6wDLItUupHFOM5rwtc2JoJRevv6APhcG yoZS3gqtJWtlA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4Xd4nt4k0rz4x8f; Tue, 29 Oct 2024 20:33:54 +1100 (AEDT) Date: Tue, 29 Oct 2024 20:26:25 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [PATCH 1/7] tcp: Pass TCP header and payload separately to tcp_update_check_tcp[46]() Message-ID: References: <20241028094050.1609090-1-david@gibson.dropbear.id.au> <20241028094050.1609090-2-david@gibson.dropbear.id.au> <20241028194254.71df8d2f@elisabeth> <20241029100954.7eb6b8a9@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="oyavu+lLG1AH4uF8" Content-Disposition: inline In-Reply-To: <20241029100954.7eb6b8a9@elisabeth> Message-ID-Hash: 3KAZYVOV43KTLXBMYNHJR2BVUHQHJL7I X-Message-ID-Hash: 3KAZYVOV43KTLXBMYNHJR2BVUHQHJL7I 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: Laurent Vivier , 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: --oyavu+lLG1AH4uF8 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Oct 29, 2024 at 10:09:54AM +0100, Stefano Brivio wrote: > On Tue, 29 Oct 2024 15:07:56 +1100 > David Gibson wrote: >=20 > > On Tue, Oct 29, 2024 at 02:02:25PM +1100, David Gibson wrote: > > > On Mon, Oct 28, 2024 at 07:42:54PM +0100, Stefano Brivio wrote: =20 > > > > On Mon, 28 Oct 2024 20:40:44 +1100 > > > > David Gibson wrote: > > > > =20 > > > > > Currently these expects both the TCP header and payload in a sing= le IOV, > > > > > and goes to some trouble to locate the checksum field within it. = In the > > > > > current caller we've already know where the TCP header is, so we = might as > > > > > well just pass it in. This will need to work a bit differently f= or > > > > > vhost-user, but that code already needs to locate the TCP header = for other > > > > > reasons, so again we can just pass it in. =20 > > > >=20 > > > > We couldn't do this, and also what you're now doing in 5/7, because > > > > with vhost-user the TCP header is not aligned, so we can't pass it > > > > around as a pointer, see: > > > >=20 > > > > > > > > https://archives.passt.top/passt-dev/ZeUpxEY-sn64NLE5@zatzit/ > > > >=20 > > > > and following. That one is about IP headers, but the same applies to > > > > TCP and UDP headers. =20 > > >=20 > > > Hrm. I'm aware it theoretically need not be aligned, but I thought it > > > was in practice.. and that we were already relying on that. > > >=20 > > > In fact, I'm pretty sure the second part is true, although more subtly > > > than here. v8 of the vhost-user patches calls tcp_fill_headers[46]() > > > with the bp parameter set to the offset of the TCP header. If > > > creating a tcphdr * there is a problem, then creating a tcp_payload_t > > > * can't be any better. >=20 > Ah, okay, I missed that. Still, I think we should ask gcc for an > opinion (with the vhost-user series on top of this series), because > those build-time pointer alignment checks are pretty reliable. I'm not exactly sure what you're suggesting with this. I don't think the compiler will catch it in this case, because we're constructing the (possibly) misaligned pointer as a (void *), then implicitly casting it by passing it to a (tcp_payload_t *) argument. (void *) is explicitly allowed to be cast to any pointer type, so I think the compiler will take this as asserting we know what we're doing. More fool it. > > > > Of course the current solution is not elegant and it would be nice = to > > > > find another way to deal with it, but we couldn't come up with anyt= hing > > > > better back then. > > > >=20 > > > > The rest of the series looks good to me, but I'm afraid that without > > > > this one and 5/7 the other changes will be a bit more complicated to > > > > implement (if at all possible). =20 > > >=20 > > > Definitely. I have so ideas for approaches more robust to > > > misalignment, but they're substantially more complicated. I was > > > hoping we could avoid it at least for now. =20 > >=20 > > I had a closer look at that earlier message now. I believe at the > > time I was aiming for fully robust handling of misaligned user > > buffers. AIUI, we've given up on that for the time being: instead > > we'll just *test* for suitable alignment and we can do the hard work > > of handling it if it ever arises in practice. >=20 > Right, and we can use the compiler to test for suitable alignment. I do see allowing the compiler to check this in more cases as an advantage of using explicitly typed pointers where we can. Btw, I didn't find a use for it just yet, but I also have a draft patch which adds a function+macro that extracts a typed pointer from a given offset into a IO vector, verifying that it's contiguous and properly aligned. --=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 --oyavu+lLG1AH4uF8 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmcgqg8ACgkQzQJF27ox 2Gf6chAArMFHMu7LxxS8ENRzxd0R6iogxHpufYTQ4xvdm718qFfh1nfWUyIRLfrp MTCVZWmFVgH8sYdP1CEKQzZkXcVjxLX5G9cSqInQexk1mMbMmnBYVnLmVUedjEQ6 gj6LwPgnCUZGupA2Fvc84TJzZcaHdB3dgrcP6CFOUmt+2UASJelbadm0FqXH2JVn 3bJ2QjPfpe9e1530mhy02bzHWLGILFrQhliTSiLvqmsLa52u9tuQqi/bHptlLlYW 5QtonzhikCP09IIicl9/0fjDs0/pZ0lS48EUAGZln2qP0kUr8AiY639dRL/HgWQd dKYKyJ+fB5ESFRSt6K69KRg55VgP2ZUVrPhl8gziP4/x7fg+GQwbGjzEjEaaqb10 GKEYBHy2vtyfgtSV5fufG8sqhcUARrj0NYxk9aVzEM4pKMcW6ewrMWKGShTIh2VM JPnMZHNtJCGljqXRxADpdBxwWZzdij2+cl4XAc9RBjPcyS3o/2KFI/fHkSIBqRZB 3x1xJXzB7YPNSC9LWoyAgJbLlrNVXORLYJB7vCWQB8SRluBKy5L2Bx8Ezy4Cgv0S 8d67D3YOFMXASWENtR3zEX/8p4BT/R4mNdPdkUtYRqhH/WJdyGe4Vce/bHh+N9CD x3ujrkyz6LT+pHL2KHTvXxqv6w3GUMUzFo14ww+Fsd4+F1P25FI= =PCFF -----END PGP SIGNATURE----- --oyavu+lLG1AH4uF8--