Re: [PATCH net-next v3 02/12] net: homa: define Homa packet formats

[Edward Cree <ecree.xilinx@gmail.com>]

On 09/12/2024 17:51, John Ousterhout wrote:
> Signed-off-by: John Ousterhout <ouster@cs.stanford.edu>
...> +/**
> + * define ETHERNET_MAX_PAYLOAD - Maximum length of an Ethernet packet,
> + * excluding preamble, frame delimeter, VLAN header, CRC, and interpacket gap;
> + * i.e. all of this space is available for Homa.
> + */
> +#define ETHERNET_MAX_PAYLOAD 1500

This would seem to bake in an assumption about Ethernet MTU, which is
 bad as this can vary on different Ethernet networks (e.g. jumbo frames
 on the one hand, or VxLAN on the other which reduces MTU to 1460 iirc).
In any case this define doesn't seem to be used anywhere in this patch
 series (nor in the version on Github), so I'd say just dike it out.

> +
> +/**
> + * struct common_header - Wire format for the first bytes in every Homa
> + * packet. This must (mostly) match the format of a TCP header to enable
> + * Homa packets to actually be transmitted as TCP packets (and thereby
> + * take advantage of TSO and other features).
> + */
> +struct common_header {

Arguably names like this should be namespaced (i.e. call it "struct
 homa_common_header"), as otherwise some other kernel .h file declaring
 a different "struct common_header" type for unrelated uses would cause
 compilation errors if both are included in the same unit.
Some of your typenames are namespaced and some aren't, and it's not clear
 how that division has been made.

> +	__u8 dummy1;

This is the flags byte in TCP, right?  Seems like you'd need to specify
 something about what goes here (or at least make it reserved) so as not
 to confuse TSO implementations.  I see HomaModule defines it as filled
 in with SYN|RST, at least the commit message here should say something
 about why you've not done the same in the kernel.

> +	__be16 dummy2;

Again, specify something here, even if it's only "reserved, must be 0"
 to allow for future extension.

> + *    No hijacking:

Assuming you don't intend to try to upstream TCP hijacking, this line
 probably doesn't want to be here as without context it'll just confuse
 the reader.