XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: Jesper Dangaard Brouer <jbrouer@redhat.com>
To: Willem de Bruijn <willemdebruijn.kernel@gmail.com>,
	Stanislav Fomichev <sdf@google.com>,
	bpf@vger.kernel.org
Cc: brouer@redhat.com, ast@kernel.org, daniel@iogearbox.net,
	andrii@kernel.org, martin.lau@linux.dev, song@kernel.org,
	yhs@fb.com, john.fastabend@gmail.com, kpsingh@kernel.org,
	haoluo@google.com, jolsa@kernel.org, kuba@kernel.org,
	toke@kernel.org, willemb@google.com, dsahern@kernel.org,
	magnus.karlsson@intel.com, bjorn@kernel.org,
	maciej.fijalkowski@intel.com, hawk@kernel.org,
	netdev@vger.kernel.org, xdp-hints@xdp-project.net
Subject: [xdp-hints] Re: [RFC net-next v4 2/8] xsk: add TX timestamp and TX checksum offload support
Date: Fri, 28 Jul 2023 13:56:03 +0200	[thread overview]
Message-ID: <92cd917b-6757-d834-5e5a-7899c5338117@redhat.com> (raw)
In-Reply-To: <64c0376cd946a_3fe1bc2947a@willemb.c.googlers.com.notmuch>



On 25/07/2023 22.58, Willem de Bruijn wrote:
> Stanislav Fomichev wrote:
>> 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 that
>>    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@google.com>
>> ---
>>   Documentation/netlink/specs/netdev.yaml | 19 ++++++++
>>   include/linux/netdevice.h               | 27 +++++++++++
>>   include/linux/skbuff.h                  |  5 ++-
>>   include/net/xdp_sock.h                  | 60 +++++++++++++++++++++++++
>>   include/net/xdp_sock_drv.h              | 13 ++++++
>>   include/uapi/linux/if_xdp.h             | 35 +++++++++++++++
>>   include/uapi/linux/netdev.h             | 15 +++++++
>>   net/core/netdev-genl.c                  | 12 ++++-
>>   net/xdp/xsk.c                           | 38 ++++++++++++++++
>>   net/xdp/xsk_queue.h                     |  2 +-
>>   tools/include/uapi/linux/if_xdp.h       | 50 ++++++++++++++++++---
>>   11 files changed, 268 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
>> index e41015310a6e..bf9c1cc32614 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:
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 11652e464f5d..8b40c80557aa 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -1660,6 +1660,31 @@ struct xdp_metadata_ops {
>>   			       enum xdp_rss_hash_type *rss_type);
>>   };
>>   
>> +/*
>> + * 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_timestamp)(u16 csum_start, u16 csum_offset, void *priv)
> 
> typo: tmo_request_checksum
> 
>> + *     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
>>    *
>> @@ -1844,6 +1869,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
>> @@ -2100,6 +2126,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 faaba050f843..5febc1a5131e 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -581,7 +581,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 *xsk_meta;
>> +	};
>>   	unsigned int	gso_type;
>>   	u32		tskey;
>>   
>> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
>> index 467b9fb56827..288fa58c4665 100644
>> --- a/include/net/xdp_sock.h
>> +++ b/include/net/xdp_sock.h
>> @@ -90,6 +90,54 @@ 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_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);
>> +}
> 
> Might be cheaper to test the flag in the hot cacheline before
> dereferencing ops?
> 

I was thinking the same thing, but I was wrong, see below, as these ops
deref's are optimized out by the compiler.

> Also, just add these functions to net_device_ops directly,
> rather than dereferencing another pointer to xsk_tx_metadata_ops?
> 

After the ASM/objdump discussion[1] in this thread, I think Stanislav's
code approach here is actually optimal. Because when the 'const' ops are
defined locally in the same file that use/does the inlining of
xsk_tx_metadata_request() then the compilers (both llvm and gcc) are
smart enough to inline and do dead-code elimination.  E.g. I noticed
that generated ASM code eliminated `if (ops->tmo_request_timestamp)`
code path in mlx5 as its not implemented.

  [1] 
https://lore.kernel.org/all/cce9db50-8c9d-ea97-cb88-171fa46cc064@redhat.com/

--Jesper


  reply	other threads:[~2023-07-28 11:56 UTC|newest]

Thread overview: 30+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-07-24 23:59 [xdp-hints] [RFC net-next v4 0/8] xsk: TX metadata Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 1/8] xsk: Support XDP_TX_METADATA_LEN Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 2/8] xsk: add TX timestamp and TX checksum offload support Stanislav Fomichev
2023-07-25 19:28   ` [xdp-hints] " Simon Horman
2023-07-25 20:30     ` Stanislav Fomichev
2023-07-25 21:28       ` Jakub Kicinski
2023-07-25 22:40         ` Stanislav Fomichev
2023-07-26  7:47           ` Simon Horman
2023-07-25 20:54   ` Willem de Bruijn
2023-07-25 22:39     ` Stanislav Fomichev
2023-07-25 23:10       ` Willem de Bruijn
2023-07-25 23:48         ` Stanislav Fomichev
2023-07-27 14:11         ` Jesper Dangaard Brouer
2023-07-27 16:37           ` Stanislav Fomichev
2023-07-25 20:58   ` Willem de Bruijn
2023-07-28 11:56     ` Jesper Dangaard Brouer [this message]
2023-07-28 13:19       ` Willem de Bruijn
2023-07-26 19:25   ` Jesper Dangaard Brouer
2023-07-26 21:25     ` Stanislav Fomichev
2023-07-27 13:50       ` Jesper Dangaard Brouer
2023-07-27 16:34         ` Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 3/8] net/mlx5e: Implement AF_XDP TX timestamp and checksum offload Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 4/8] tools: ynl: update netdev sample to dump xsk-features Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 5/8] selftests/xsk: Support XDP_TX_METADATA_LEN Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 6/8] selftests/bpf: Add csum helpers Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 7/8] selftests/bpf: Add TX side to xdp_metadata Stanislav Fomichev
2023-07-24 23:59 ` [xdp-hints] [RFC net-next v4 8/8] selftests/bpf: Add TX side to xdp_hw_metadata Stanislav Fomichev
2023-07-25 20:59   ` [xdp-hints] " Willem de Bruijn
2023-07-25 22:36     ` Stanislav Fomichev
2023-07-25 22:55       ` Willem de Bruijn

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: https://lists.xdp-project.net/postorius/lists/xdp-hints.xdp-project.net/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=92cd917b-6757-d834-5e5a-7899c5338117@redhat.com \
    --to=jbrouer@redhat.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=dsahern@kernel.org \
    --cc=haoluo@google.com \
    --cc=hawk@kernel.org \
    --cc=john.fastabend@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=maciej.fijalkowski@intel.com \
    --cc=magnus.karlsson@intel.com \
    --cc=martin.lau@linux.dev \
    --cc=netdev@vger.kernel.org \
    --cc=sdf@google.com \
    --cc=song@kernel.org \
    --cc=toke@kernel.org \
    --cc=willemb@google.com \
    --cc=willemdebruijn.kernel@gmail.com \
    --cc=xdp-hints@xdp-project.net \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox