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

[Jakub Kicinski <kuba@kernel.org>]

On Thu, 19 Oct 2023 10:49:35 -0700 Stanislav Fomichev wrote:
> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> index 14511b13f305..22d2649a34ee 100644
> --- a/Documentation/netlink/specs/netdev.yaml
> +++ b/Documentation/netlink/specs/netdev.yaml
> @@ -55,6 +55,19 @@ name: netdev
>          name: hash
>          doc:
>            Device is capable of exposing receive packet hash via bpf_xdp_metadata_rx_hash().
> +  -
> +    type: flags
> +    name: xsk-flags
> +    render-max: true

I don't think you're using the MAX, maybe don't render it.
IDK what purpose it'd serve for feature flag enums.

> +/*
> + * This structure defines the AF_XDP TX metadata hooks for network devices.

s/This structure defines the //

> + * The following hooks can be defined; unless noted otherwise, they are
> + * optional and can be filled with a null pointer.
> + *
> + * void (*tmo_request_timestamp)(void *priv)
> + *     This function is called when AF_XDP frame requested egress timestamp.

s/This function is // in many places

> + * u64 (*tmo_fill_timestamp)(void *priv)
> + *     This function is called when AF_XDP frame, that had requested
> + *     egress timestamp, received a completion. The hook needs to return
> + *     the actual HW timestamp.
> + *
> + * void (*tmo_request_checksum)(u16 csum_start, u16 csum_offset, void *priv)
> + *     This function is called when AF_XDP frame requested HW checksum
> + *     offload. csum_start indicates position where checksumming should start.
> + *     csum_offset indicates position where checksum should be stored.
> + *
> + */
> +struct xsk_tx_metadata_ops {
> +	void	(*tmo_request_timestamp)(void *priv);
> +	u64	(*tmo_fill_timestamp)(void *priv);
> +	void	(*tmo_request_checksum)(u16 csum_start, u16 csum_offset, void *priv);
> +};

Could you move the definition of the struct to include/net/xdp_sock.h ?
netdevice.h doesn't need it.

> +/* 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)

Reuse of enum netdev_xsk_flags is not an option?

> +/* 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)

Is there a need for this to be on packet-by-packet basis?
HW issues should generally be fixed by the driver, is there 
any type of problem in particular you have in mind here?

> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index fe61f85bcf33..5d889c2425fd 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -14,6 +14,7 @@ netdev_nl_dev_fill(struct net_device *netdev, struct sk_buff *rsp,
>  		   const struct genl_info *info)
>  {
>  	u64 xdp_rx_meta = 0;
> +	u64 xsk_features = 0;

rev xmas tree? :)

> +			meta = buffer - xs->pool->tx_metadata_len;
> +
> +			if (meta->flags & XDP_TX_METADATA_CHECKSUM) {

Do we need to worry about reserved / unsupported meta->flags ?

> +				if (unlikely(meta->csum_start + meta->csum_offset +
> +					     sizeof(__sum16) > len)) {
> +					err = -EINVAL;
> +					goto free_err;
> +				}