netdev - Re: [PATCH net-next v8 02/15] net: homa: create homa

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXJAmzuf51CVYZRPDC9OOpr+UVo5SjgiEEGfSQVdy=TC0OhtQ@mail.gmail.com>
Date: Mon, 5 May 2025 16:54:17 -0700
From: John Ousterhout <ouster@...stanford.edu>
To: Paolo Abeni <pabeni@...hat.com>
Cc: netdev@...r.kernel.org, edumazet@...gle.com, horms@...nel.org, 
	kuba@...nel.org
Subject: Re: [PATCH net-next v8 02/15] net: homa: create homa_wire.h

On Mon, May 5, 2025 at 1:28 AM Paolo Abeni <pabeni@...hat.com> wrote:
>
> On 5/3/25 1:37 AM, John Ousterhout wrote:
> > diff --git a/net/homa/homa_wire.h b/net/homa/homa_wire.h
> > new file mode 100644
> > index 000000000000..47693244c3ec
> > --- /dev/null
> > +++ b/net/homa/homa_wire.h
>
> I'm wondering why you keep the wire-struct definition outside the uAPI -
> the opposite of what other protocols do.

By 'uAPI' I assume you mean homa.h? I didn't consider putting it
there, and would prefer not to, because Homa users should not need to
know anything about the wire format. But, I will combine the files if
you think it is better to do that for consistency with other
protocols. Please confirm?

> > +/* Defines the possible types of Homa packets.
> > + *
> > + * See the xxx_header structs below for more information about each type.
> > + */
> > +enum homa_packet_type {
> > +     DATA               = 0x10,
> > +     RESEND             = 0x12,
> > +     RPC_UNKNOWN        = 0x13,
> > +     BUSY               = 0x14,
> > +     NEED_ACK           = 0x17,
> > +     ACK                = 0x18,
> > +     BOGUS              = 0x19,      /* Used only in unit tests. */
> > +     /* If you add a new type here, you must also do the following:
> > +      * 1. Change BOGUS so it is the highest opcode
>
> If you instead define 'MAX' value, the required update policy would be
> self-explained and you will not need to expose tests details.

Done.

>
> > +      * 2. Add support for the new opcode in homa_print_packet,
> > +      *    homa_print_packet_short, homa_symbol_for_type, and mock_skb_new.
> > +      * 3. Add the header length to header_lengths in homa_plumbing.c.
> > +      */
> > +};
> > +
> > +/** define HOMA_IPV6_HEADER_LENGTH - Size of IP header (V6). */
> > +#define HOMA_IPV6_HEADER_LENGTH 40
> > +
> > +/** define HOMA_IPV4_HEADER_LENGTH - Size of IP header (V4). */
> > +#define HOMA_IPV4_HEADER_LENGTH 20
>
> I suspect you will be better off using sizeof(<relevant struct>). Making
> protocol-specific definition for common/global constants is somewhat
> confusing and unexpected

Done.

> > +/**
> > + * define HOMA_SKB_EXTRA - How many bytes of additional space to allow at the
> > + * beginning of each sk_buff, before the IP header. This includes room for a
> > + * VLAN header and also includes some extra space, "just to be safe" (not
> > + * really sure if this is needed).
> > + */
> > +#define HOMA_SKB_EXTRA 40
>
> You could use:
>
> #define MAX_HOME_HEADER MAX_TCP_HEADER
>
> to leverage a consolidated value covering most use-cases and kernel configs.

Done (I wasn't aware of MAX_TCP_HEADER).

> > +/**
> > + * define HOMA_ETH_OVERHEAD - Number of bytes per Ethernet packet for Ethernet
> > + * header, CRC, preamble, and inter-packet gap.
> > + */
> > +#define HOMA_ETH_OVERHEAD 42
>
> It's not clear why the protocol should be interested in MAC-specific
> details. What if the the MAC is not ethernet?

This is used by the pacer in order to get the most accurate possible
estimate of exactly how much time it will take to transmit a packet
(so that Homa can pass packets to the NIC at a rate that runs the
uplink at utilization just under 100% without generating queue buildup
in the NIC). Is there a way to get this information from the dev in a
way that reflects the specific hardware more accurately than guessing
based on Ethernet?

> > +     /**
> > +      * @type: Homa packet type (one of the values of the homa_packet_type
> > +      * enum). Corresponds to the low-order byte of the ack in TCP.
> > +      */
> > +     __u8 type;
>
> If you keep this outside uAPI you should use 'u8'

Will do.

> > +_Static_assert(sizeof(struct homa_data_hdr) <= HOMA_MAX_HEADER,
> > +            "homa_data_hdr too large for HOMA_MAX_HEADER; must adjust HOMA_MAX_HEADER");
> > +_Static_assert(sizeof(struct homa_data_hdr) >= HOMA_MIN_PKT_LENGTH,
> > +            "homa_data_hdr too small: Homa doesn't currently have code to pad data packets");
> > +_Static_assert(((sizeof(struct homa_data_hdr) - sizeof(struct homa_seg_hdr)) &
> > +             0x3) == 0,
> > +            " homa_data_hdr length not a multiple of 4 bytes (required for TCP/TSO compatibility");
>
> Please use BUILD_BUG_ON() in a .c file instead. Many other cases below.

Will do, if this info doesn't move to uAPI (BTW, BUILD_BUG_ON feels
more awkward because it doesn't allow an error message).


-John-