From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from gandalf.ozlabs.org (mail.ozlabs.org [IPv6:2404:9400:2221:ea00::3]) by passt.top (Postfix) with ESMTPS id F02C05A0271 for ; Tue, 19 Mar 2024 05:53:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gibson.dropbear.id.au; s=202312; t=1710824022; bh=XbDLNzjMy9vmCKPaovR7VzOXcCKLkIuivZNyuc3qJBE=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=I3HRIR+qiA/a4cOKXRGHgUMfRv1HqlJ0EpysI/bVwCaksaiYHLVBmDchjvUoEDMPu dWFRLAmj74i+U3aQa3yaOH0+p5yFJS02uvg8Rt2YXoONNXr4IPRBkSilVp4sT/6y86 qJk2PgI2yXa7a6jABTsIDPdUl3E5E2Old+vjJvINOLi+iCg4tZhRwOdFWqXDYD5Qmj zF7F/zd+wObdchP6EoeN3wRgWO1hYSHUoZHSAEpEg3YQVkK3KkZgjU40mg1zgi0hHc HoxG06zz6YZYHbaoh9MTJ4LiZTdbtJlMle6pgdxMv9DswvOWsKOof6c6eujld5RtVc JTak1eRHI8AmA== Received: by gandalf.ozlabs.org (Postfix, from userid 1007) id 4TzK9y6TJvz4wcC; Tue, 19 Mar 2024 15:53:42 +1100 (AEDT) Date: Tue, 19 Mar 2024 15:51:10 +1100 From: David Gibson To: Stefano Brivio Subject: Re: [RFC v2] tcp: Replace TCP buffer structure by an iovec array Message-ID: References: <20240315175119.1618550-1-lvivier@redhat.com> <20240318095329.34152c6f@elisabeth> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="lyyFMqpTyxKMb1K9" Content-Disposition: inline In-Reply-To: <20240318095329.34152c6f@elisabeth> Message-ID-Hash: SPDKP24L7GENPQ6SPD6P5J2XNF6ZXEFS X-Message-ID-Hash: SPDKP24L7GENPQ6SPD6P5J2XNF6ZXEFS 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: --lyyFMqpTyxKMb1K9 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Mar 18, 2024 at 09:53:29AM +0100, Stefano Brivio wrote: > [Not a full review, just some follow-up comments] >=20 > On Mon, 18 Mar 2024 16:47:34 +1100 > David Gibson wrote: >=20 > > On Fri, Mar 15, 2024 at 06:51:19PM +0100, Laurent Vivier wrote: [snip] > > > -#define MSS4 ROUND_DOWN(USHRT_MAX - sizeof(struct tcp4_l2_head), 4) > > > -#define MSS6 ROUND_DOWN(USHRT_MAX - sizeof(struct tcp6_l2_head), 4) > > > +#define MSS4 ROUND_DOWN(ETH_MAX_MTU - ETH_HLEN - sizeof(struct tc= phdr) - sizeof(struct iphdr), sizeof(uint32_t)) > > > +#define MSS6 ROUND_DOWN(ETH_MAX_MTU - ETH_HLEN - sizeof(struct tc= phdr) - sizeof(struct ipv6hdr), sizeof(uint32_t)) =20 > >=20 > > I'd be inclined to base these static constants on only the IP level > > restriction (16-bit L3 packet length), i.e. replace (ETH_MAX_MTU - > > ETH_HLEN) with USHRT_MAX (or a new L3_MAX_LENGTH constant with the > > same value). >=20 > The kernel has IP_MAX_MTU for internal usage -- with its name spilling > over to userspace via comment to ETH_MAX_MTU ("same as IP_MAX_MTU"). I > would call it that way, in case. >=20 > But... is there any value in ignoring the length of the Ethernet header > here? I think so, yes. > Thinking further about it, if one day we enable usage of, say, a tun > interface (instead of tap), we can probably send 14 bytes more. Is this > the reason why you suggest this? Largely, yes. > Because to me, if we assume instead that we'll always have 802.3-style > frames, it makes sense to account for ETH_HLEN here. >=20 > > The L2 MTU restriction, ought to be applied at runtime, > > to properly handle the --mtu option. >=20 > Okay, but MSS4 and MSS6 are here to define buffer sizes, not to be used > at runtime (except for clamping incoming values, and there I'm not so > sure we want to ignore ETH_HLEN). Exactly - all we want here is an upper bound. 14 bytes isn't enough savings to make it worthwhile to compact this further, especially since it makes us less well aligned. Therefore we might as well make the simpler calculation based only on IP-internal limits, since this will also make this robust if we ever want to implement a different L2 encapsulation. > > > #define WINDOW_DEFAULT 14600 /* RFC 6928 */ > > > #ifdef HAS_SND_WND > > > @@ -445,133 +414,111 @@ struct tcp_buf_seq_update { > > > }; > > > =20 > > > /* Static buffers */ > > > - > > > /** > > > - * tcp4_l2_buf_t - Pre-cooked IPv4 packet buffers for tap connections > > > - * @pad: Align TCP header to 32 bytes, for AVX2 checksum calculation= only > > > - * @taph: Tap-level headers (partially pre-filled) > > > - * @iph: Pre-filled IP header (except for tot_len and saddr) > > > - * @uh: Headroom for TCP header > > > - * @data: Storage for TCP payload > > > + * struct tcp_l2_payload_t - TCP header and data to send data =20 > >=20 > > Pre-existing problem, but without a typedef, I don't think the _t > > suffix is useful. >=20 > The idea behind that suffix is that the variable is tcp4_l2_buf[], > without _t, and the type name has a _t: you can't call them the same > way. And yet, we needed to refer to the type name. Oh, good point, ignore my comment. --=20 David Gibson | 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 --lyyFMqpTyxKMb1K9 Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEO+dNsU4E3yXUXRK2zQJF27ox2GcFAmX5GbkACgkQzQJF27ox 2Gf1qw/+M6atikTTYGAR+dScxUWNZMRFW3wmxMNUKmFt6nrj0IwZPLNMYpJeANYc g5UTIhg4LVlgEi41vF66Jqx21++iLDxJD4BUvdWqv4DS0eNJFBvJ0VwTxDA+lNzL VkkW2K/k3NEC4tRJrvN3qTuqJ0iLP0yAH2e5EVck1duyCBVAra9jDZXKoOpgdw+5 ngOCpPmxaZ+8QuddbZBZIP5dWOUjCV+eiifwNHB3XU2HhvCuKV99q7G9bNAo+r9o t9Oid2ZqZftUCNGQ6rTZ0T58mQkZofDeBg/PeEYEkaSugtqNr8QoccblACjwM/U6 Iveyq69hMVWL3v8X1euTdia+6VxoxtzB9dKgvO1RHaySr39P/bCDzhU0qlMs+I+H pJQKDg7+VyQZSgG8wXS+G62ujTHwFsZ2F2Vkp1JYan1iVuNSrzOufdFql/FuGuEJ x3qwguVIIV8hFGdiPJabHmp9dH4VheIM7dWBPEzETjrX/D0AiVnu8yu1L0S154j3 T4oW8C0Grzbns+sZBFPcLK8GvKNnpd1SUTA84PpdO2Eoe0Up2FC7ohuTFSTNXixP gVPsZiN9QA5MO2IRAISxwRZnDEUPPbqskycsUrfm5J+3qGyhCYF4djyXg3ZbPU/h r9t9toRXIlM1ejF5h1qxJI+uJjXu3088MqfXel73ghmvptp1l+8= =ObOr -----END PGP SIGNATURE----- --lyyFMqpTyxKMb1K9--