[xdp-hints] Re: [PATCH bpf-next v4 02/11] xsk: Add TX timestamp and TX checksum offload support

[Stanislav Fomichev <sdf@google.com>]

On 10/20, Alexei Starovoitov wrote:
> On Thu, Oct 19, 2023 at 10:49:35AM -0700, Stanislav Fomichev wrote:
> > diff --git a/include/uapi/linux/if_xdp.h b/include/uapi/linux/if_xdp.h
> > index 2ecf79282c26..ecfd67988283 100644
> > --- a/include/uapi/linux/if_xdp.h
> > +++ b/include/uapi/linux/if_xdp.h
> > @@ -106,6 +106,43 @@ struct xdp_options {
> >  #define XSK_UNALIGNED_BUF_ADDR_MASK \
> >  	((1ULL << XSK_UNALIGNED_BUF_OFFSET_SHIFT) - 1)
> >  
> > +/* Request transmit timestamp. Upon completion, put it into tx_timestamp
> > + * field of struct xsk_tx_metadata.
> > + */
> > +#define XDP_TX_METADATA_TIMESTAMP		(1 << 0)
> > +
> > +/* Request transmit checksum offload. Checksum start position and offset
> > + * are communicated via csum_start and csum_offset fields of struct
> > + * xsk_tx_metadata.
> > + */
> > +#define XDP_TX_METADATA_CHECKSUM		(1 << 1)
> > +
> > +/* Force checksum calculation in software. Can be used for testing or
> > + * working around potential HW issues. This option causes performance
> > + * degradation and only works in XDP_COPY mode.
> > + */
> > +#define XDP_TX_METADATA_CHECKSUM_SW		(1 << 2)
> > +
> > +struct xsk_tx_metadata {
> > +	union {
> > +		struct {
> > +			__u32 flags;
> > +
> > +			/* XDP_TX_METADATA_CHECKSUM */
> > +
> > +			/* Offset from desc->addr where checksumming should start. */
> > +			__u16 csum_start;
> > +			/* Offset from csum_start where checksum should be stored. */
> > +			__u16 csum_offset;
> > +		};
> > +
> > +		struct {
> > +			/* XDP_TX_METADATA_TIMESTAMP */
> > +			__u64 tx_timestamp;
> > +		} completion;
> > +	};
> > +};
> 
> Could you add a comment to above union that csum fields are consumed by the driver
> before it xmits the packet while timestamp is filled during xmit, so union
> doesn't prevent using both features simultaneously.
> It's clear from the example, but not obvious from uapi and the doc in patch 11
> doesn't clarify it either.
> 
> Also please add a name to csum part of the union like you did for completion.
> We've learned it the hard way with bpf_attr. All anon structs better have field name
> within a union. Helps extensibility (avoid conflicts) in the long term.

Sure, will do, thanks!

> Other than this the patch set looks great to me.
> With Saeed and Magnus acks we can take it in.

Magnus is on CC, so I hope see sees the request.

Added Saeed here as well. Saeed, can you please take a look at the mlx part?
You've been on CC for a particular patch, but just in case:
https://lore.kernel.org/bpf/20231019174944.3376335-5-sdf@google.com/T/#u