From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Jesper Dangaard Brouer <jbrouer@redhat.com>,
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 09:19:58 -0400 [thread overview]
Message-ID: <64c3c07ef0ced_90c172943d@willemb.c.googlers.com.notmuch> (raw)
In-Reply-To: <92cd917b-6757-d834-5e5a-7899c5338117@redhat.com>
Jesper Dangaard Brouer wrote:
>
>
> 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
>
Agreed. That is pretty slick :-) I had entirely missed that.
next prev parent reply other threads:[~2023-07-28 13:20 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
2023-07-28 13:19 ` Willem de Bruijn [this message]
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=64c3c07ef0ced_90c172943d@willemb.c.googlers.com.notmuch \
--to=willemdebruijn.kernel@gmail.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=jbrouer@redhat.com \
--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=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