XDP hardware hints discussion mail archive
 help / color / mirror / Atom feed
From: Stanislav Fomichev <sdf@google.com>
To: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Cc: bpf@vger.kernel.org, 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,
	yoong.siang.song@intel.com, netdev@vger.kernel.org,
	xdp-hints@xdp-project.net
Subject: [xdp-hints] Re: [PATCH bpf-next v2 2/9] xsk: add TX timestamp and TX checksum offload support
Date: Fri, 15 Sep 2023 09:25:11 -0700	[thread overview]
Message-ID: <CAKH8qBtQf9g8Ye7QZ5+s2j+srJEkBwy3GAXPwPaxVeoc056C_A@mail.gmail.com> (raw)
In-Reply-To: <87edj0dxzi.fsf@intel.com>

On Thu, Sep 14, 2023 at 6:30 PM Vinicius Costa Gomes
<vinicius.gomes@intel.com> wrote:
>
> Hi,
>
> Stanislav Fomichev <sdf@google.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@google.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.

Thank you, will take a look. There are a bunch of bot build errors
that look similar as well.

  reply	other threads:[~2023-09-15 16:25 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-14 21:04 [xdp-hints] [PATCH bpf-next v2 0/9] xsk: TX metadata Stanislav Fomichev
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 1/9] xsk: Support tx_metadata_len Stanislav Fomichev
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 2/9] xsk: add TX timestamp and TX checksum offload support Stanislav Fomichev
2023-09-15  1:29   ` [xdp-hints] " kernel test robot
2023-09-15  1:30   ` Vinicius Costa Gomes
2023-09-15 16:25     ` Stanislav Fomichev [this message]
2023-09-15  9:44   ` kernel test robot
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 3/9] tools: ynl: print xsk-features from the sample Stanislav Fomichev
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 4/9] net/mlx5e: Implement AF_XDP TX timestamp and checksum offload Stanislav Fomichev
2023-09-15  2:44   ` [xdp-hints] " kernel test robot
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 5/9] selftests/xsk: Support tx_metadata_len Stanislav Fomichev
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 6/9] selftests/bpf: Add csum helpers Stanislav Fomichev
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 7/9] selftests/bpf: Add TX side to xdp_metadata Stanislav Fomichev
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 8/9] selftests/bpf: Add TX side to xdp_hw_metadata Stanislav Fomichev
2023-09-14 21:04 ` [xdp-hints] [PATCH bpf-next v2 9/9] xsk: document tx_metadata_len layout Stanislav Fomichev

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=CAKH8qBtQf9g8Ye7QZ5+s2j+srJEkBwy3GAXPwPaxVeoc056C_A@mail.gmail.com \
    --to=sdf@google.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --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=song@kernel.org \
    --cc=toke@kernel.org \
    --cc=vinicius.gomes@intel.com \
    --cc=willemb@google.com \
    --cc=xdp-hints@xdp-project.net \
    --cc=yhs@fb.com \
    --cc=yoong.siang.song@intel.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