Date: Thu, 14 Sep 2023 18:30:25 -0700
From: Vinicius Costa Gomes <vinicius.gomes@...el.com>
To: Stanislav Fomichev <sdf@...gle.com>, bpf@...r.kernel.org
Cc: ast@...nel.org, daniel@...earbox.net, andrii@...nel.org,
 martin.lau@...ux.dev, song@...nel.org, yhs@...com,
 john.fastabend@...il.com, kpsingh@...nel.org, sdf@...gle.com,
 haoluo@...gle.com, jolsa@...nel.org, kuba@...nel.org, toke@...nel.org,
 willemb@...gle.com, dsahern@...nel.org, magnus.karlsson@...el.com,
 bjorn@...nel.org, maciej.fijalkowski@...el.com, hawk@...nel.org,
 yoong.siang.song@...el.com, netdev@...r.kernel.org,
 xdp-hints@...-project.net
Subject: Re: [PATCH bpf-next v2 2/9] xsk: add TX timestamp and TX checksum
 offload support

Hi,

Stanislav Fomichev <sdf@...gle.com> writes:

> This change actually defines the (initial) metadata layout
> that should be used by AF_XDP userspace (xsk_tx_metadata).
> The first field is flags which requests appropriate offloads,
> followed by the offload-specific fields. The supported per-device
> offloads are exported via netlink (new xsk-flags).
>
> The offloads themselves are still implemented in a bit of a
> framework-y fashion that's left from my initial kfunc attempt.
> I'm introducing new xsk_tx_metadata_ops which drivers are
> supposed to implement. The drivers are also supposed
> to call xsk_tx_metadata_request/xsk_tx_metadata_complete in
> the right places. Since xsk_tx_metadata_{request,_complete}
> are static inline, we don't incur any extra overhead doing
> indirect calls.
>
> The benefit of this scheme is as follows:
> - keeps all metadata layout parsing away from driver code
> - makes it easy to grep and see which drivers implement what
> - don't need any extra flags to maintain to keep track of what
>   offloads are implemented; if the callback is implemented - the offload
>   is supported (used by netlink reporting code)
>
> Two offloads are defined right now:
> 1. XDP_TX_METADATA_CHECKSUM: skb-style csum_start+csum_offset
> 2. XDP_TX_METADATA_TIMESTAMP: writes TX timestamp back into metadata
>    area upon completion (tx_timestamp field)
>
> The offloads are also implemented for copy mode:
> 1. Extra XDP_TX_METADATA_CHECKSUM_SW to trigger skb_checksum_help; this
>    might be useful as a reference implementation and for testing
> 2. XDP_TX_METADATA_TIMESTAMP writes SW timestamp from the skb
>    destructor (note I'm reusing hwtstamps to pass metadata pointer)
>
> The struct is forward-compatible and can be extended in the future
> by appending more fields.
>
> Signed-off-by: Stanislav Fomichev <sdf@...gle.com>
> ---
>  Documentation/netlink/specs/netdev.yaml | 20 +++++++
>  include/linux/netdevice.h               | 27 +++++++++
>  include/linux/skbuff.h                  | 14 ++++-
>  include/net/xdp_sock.h                  | 80 +++++++++++++++++++++++++
>  include/net/xdp_sock_drv.h              | 13 ++++
>  include/net/xsk_buff_pool.h             |  6 ++
>  include/uapi/linux/if_xdp.h             | 40 +++++++++++++
>  include/uapi/linux/netdev.h             | 16 +++++
>  net/core/netdev-genl.c                  | 12 +++-
>  net/xdp/xsk.c                           | 39 ++++++++++++
>  net/xdp/xsk_queue.h                     |  2 +-
>  tools/include/uapi/linux/if_xdp.h       | 54 +++++++++++++++--
>  tools/include/uapi/linux/netdev.h       | 15 +++++
>  13 files changed, 330 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> index 1c7284fd535b..9002b37b7676 100644
> --- a/Documentation/netlink/specs/netdev.yaml
> +++ b/Documentation/netlink/specs/netdev.yaml
> @@ -42,6 +42,19 @@ name: netdev
>          doc:
>            This feature informs if netdev implements non-linear XDP buffer
>            support in ndo_xdp_xmit callback.
> +  -
> +    type: flags
> +    name: xsk-flags
> +    render-max: true
> +    entries:
> +      -
> +        name: tx-timestamp
> +        doc:
> +          HW timestamping egress packets is supported by the driver.
> +      -
> +        name: tx-checksum
> +        doc:
> +          L3 checksum HW offload is supported by the driver.
>  
>  attribute-sets:
>    -
> @@ -68,6 +81,12 @@ name: netdev
>          type: u32
>          checks:
>            min: 1
> +      -
> +        name: xsk-features
> +        doc: Bitmask of enabled AF_XDP features.
> +        type: u64
> +        enum: xsk-flags
> +        enum-as-flags: true
>  
>  operations:
>    list:
> @@ -84,6 +103,7 @@ name: netdev
>              - ifindex
>              - xdp-features
>              - xdp-zc-max-segs
> +            - xsk-features
>        dump:
>          reply: *dev-all
>      -
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 0896aaa91dd7..3f02aaa30590 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1647,6 +1647,31 @@ struct net_device_ops {
>  						    struct netlink_ext_ack *extack);
>  };
>  
> +/*
> + * This structure defines the AF_XDP TX metadata hooks for network devices.
> + * The following hooks can be defined; unless noted otherwise, they are
> + * optional and can be filled with a null pointer.
> + *
> + * int (*tmo_request_timestamp)(void *priv)
> + *     This function is called when AF_XDP frame requested egress timestamp.
> + *
> + * int (*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.
> + *
> + * int (*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);
> +};
> +
>  /**
>   * enum netdev_priv_flags - &struct net_device priv_flags
>   *
> @@ -1835,6 +1860,7 @@ enum netdev_ml_priv_type {
>   *	@netdev_ops:	Includes several pointers to callbacks,
>   *			if one wants to override the ndo_*() functions
>   *	@xdp_metadata_ops:	Includes pointers to XDP metadata callbacks.
> + *	@xsk_tx_metadata_ops:	Includes pointers to AF_XDP TX metadata callbacks.
>   *	@ethtool_ops:	Management operations
>   *	@l3mdev_ops:	Layer 3 master device operations
>   *	@ndisc_ops:	Includes callbacks for different IPv6 neighbour
> @@ -2091,6 +2117,7 @@ struct net_device {
>  	unsigned long long	priv_flags;
>  	const struct net_device_ops *netdev_ops;
>  	const struct xdp_metadata_ops *xdp_metadata_ops;
> +	const struct xsk_tx_metadata_ops *xsk_tx_metadata_ops;
>  	int			ifindex;
>  	unsigned short		gflags;
>  	unsigned short		hard_header_len;
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 4174c4b82d13..444d35dcd690 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -566,6 +566,15 @@ struct ubuf_info_msgzc {
>  int mm_account_pinned_pages(struct mmpin *mmp, size_t size);
>  void mm_unaccount_pinned_pages(struct mmpin *mmp);
>  
> +/* Preserve some data across TX submission and completion.
> + *
> + * Note, this state is stored in the driver. Extending the layout
> + * might need some special care.
> + */
> +struct xsk_tx_metadata_compl {
> +	__u64 *tx_timestamp;
> +};
> +
>  /* This data is invariant across clones and lives at
>   * the end of the header data, ie. at skb->end.
>   */
> @@ -578,7 +587,10 @@ struct skb_shared_info {
>  	/* Warning: this field is not always filled in (UFO)! */
>  	unsigned short	gso_segs;
>  	struct sk_buff	*frag_list;
> -	struct skb_shared_hwtstamps hwtstamps;
> +	union {
> +		struct skb_shared_hwtstamps hwtstamps;
> +		struct xsk_tx_metadata_compl xsk_meta;
> +	};
>  	unsigned int	gso_type;
>  	u32		tskey;
>  
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index 10993a05d220..c438c614a8d0 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -90,6 +90,74 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp);
>  int __xsk_map_redirect(struct xdp_sock *xs, struct xdp_buff *xdp);
>  void __xsk_map_flush(void);
>  
> +/**
> + *  xsk_tx_metadata_to_compl - Save enough relevant metadata information
> + *  to perform tx completion in the future.
> + *  @meta: pointer to AF_XDP metadata area
> + *  @compl: pointer to output struct xsk_tx_metadata_to_compl
> + *
> + *  This function should be called by the networking device when
> + *  it prepares AF_XDP egress packet. The value of @compl should be stored
> + *  and passed to xsk_tx_metadata_complete upon TX completion.
> + */
> +static inline void xsk_tx_metadata_to_compl(struct xsk_tx_metadata *meta,
> +					    struct xsk_tx_metadata_compl *compl)

One of my systems didn't have the config XDP_SOCKETS enabled, and found
some issues:

This function is not defined when XDP_SOCKETS is disabled, got a
compilation error.

> +{
> +	if (!meta)
> +		return;
> +
> +	if (meta->flags & XDP_TX_METADATA_TIMESTAMP)
> +		compl->tx_timestamp = &meta->completion.tx_timestamp;
> +	else
> +		compl->tx_timestamp = NULL;
> +}
> +
> +/**
> + *  xsk_tx_metadata_request - Evaluate AF_XDP TX metadata at submission
> + *  and call appropriate xsk_tx_metadata_ops operation.
> + *  @meta: pointer to AF_XDP metadata area
> + *  @ops: pointer to struct xsk_tx_metadata_ops
> + *  @priv: pointer to driver-private aread
> + *
> + *  This function should be called by the networking device when
> + *  it prepares AF_XDP egress packet.
> + */
> +static inline void xsk_tx_metadata_request(const struct xsk_tx_metadata *meta,
> +					   const struct xsk_tx_metadata_ops *ops,
> +					   void *priv)
> +{
> +	if (!meta)
> +		return;
> +
> +	if (ops->tmo_request_timestamp)
> +		if (meta->flags & XDP_TX_METADATA_TIMESTAMP)
> +			ops->tmo_request_timestamp(priv);
> +
> +	if (ops->tmo_request_checksum)
> +		if (meta->flags & XDP_TX_METADATA_CHECKSUM)
> +			ops->tmo_request_checksum(meta->csum_start, meta->csum_offset, priv);
> +}
> +
> +/**
> + *  xsk_tx_metadata_complete - Evaluate AF_XDP TX metadata at completion
> + *  and call appropriate xsk_tx_metadata_ops operation.
> + *  @compl: pointer to completion metadata produced from xsk_tx_metadata_to_compl
> + *  @ops: pointer to struct xsk_tx_metadata_ops
> + *  @priv: pointer to driver-private aread
> + *
> + *  This function should be called by the networking device upon
> + *  AF_XDP egress completion.
> + */
> +static inline void xsk_tx_metadata_complete(struct xsk_tx_metadata_compl *compl,
> +					    const struct xsk_tx_metadata_ops *ops,
> +					    void *priv)
> +{
> +	if (!compl)
> +		return;
> +
> +	*compl->tx_timestamp = ops->tmo_fill_timestamp(priv);
> +}
> +
>  #else
>  
>  static inline int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
> @@ -106,6 +174,18 @@ static inline void __xsk_map_flush(void)
>  {
>  }
>  
> +static inline void xsk_tx_metadata_request(struct xsk_tx_metadata *meta,
> +					   const struct xsk_tx_metadata_ops *ops,
> +					   void *priv)
> +{
> +}
> +
> +static inline void xsk_tx_metadata_complete(struct xsk_tx_metadata_comp *compl,

typo here: should be 'struct xsk_tx_metadata_compl'.

Just so you are aware of these (very) minor issues for the next version.

I enabled XDP_SOCKETS and will take a better look.


Cheers,
-- 
Vinicius

Powered by blists - more mailing lists