On Mon, Mar 18, 2024 at 09:53:29AM +0100, Stefano Brivio wrote:[Not a full review, just some follow-up comments] On Mon, 18 Mar 2024 16:47:34 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote: > On Fri, Mar 15, 2024 at 06:51:19PM +0100, Laurent Vivier wrote:[snip]I think so, yes.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. But... is there any value in ignoring the length of the Ethernet header here?-#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 tcphdr) - sizeof(struct iphdr), sizeof(uint32_t)) +#define MSS6 ROUND_DOWN(ETH_MAX_MTU - ETH_HLEN - sizeof(struct tcphdr) - sizeof(struct ipv6hdr), sizeof(uint32_t))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).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.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.The L2 MTU restriction, ought to be applied at runtime, to properly handle the --mtu option.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).Oh, good point, ignore my comment. -- 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/~dgibsonThe 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.#define WINDOW_DEFAULT 14600 /* RFC 6928 */ #ifdef HAS_SND_WND @@ -445,133 +414,111 @@ struct tcp_buf_seq_update { }; /* 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 dataPre-existing problem, but without a typedef, I don't think the _t suffix is useful.