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@gibson.dropbear.id.au> wrote:
> 
> > 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 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).
> 
> 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?

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.
> 
> > 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).

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 {
> > >  };
> > >  
> > >  /* 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  
> > 
> > Pre-existing problem, but without a typedef, I don't think the _t
> > suffix is useful.
> 
> 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.

-- 
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