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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

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.

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