netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Lorenzo Bianconi <lorenzo@kernel.org>
To: Tom Herbert <tom@herbertland.com>
Cc: bpf@vger.kernel.org,
	"Linux Kernel Network Developers" <netdev@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Jakub Kicinski" <kuba@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Eelco Chaudron" <echaudro@redhat.com>,
	"David Ahern" <dsahern@gmail.com>,
	magnus.karlsson@intel.com,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	bjorn@kernel.org,
	"Maciej Fijałkowski (Intel)" <maciej.fijalkowski@intel.com>,
	"john fastabend" <john.fastabend@gmail.com>
Subject: Re: [RFC bpf-next 1/4] net: xdp: introduce flags field in xdp_buff and xdp_frame
Date: Sat, 29 May 2021 15:34:18 +0200	[thread overview]
Message-ID: <YLJC2ox7HmAL8dnX@lore-desk> (raw)
In-Reply-To: <CALx6S34cmsFX6QwUq0sRpHok1j6ecBBJ7WC2BwjEmxok+CHjqg@mail.gmail.com>

[-- Attachment #1: Type: text/plain, Size: 4945 bytes --]

> On Fri, May 28, 2021 at 10:44 AM Lorenzo Bianconi <lorenzo@kernel.org> wrote:
> >
> > Introduce flag field in xdp_buff and xdp_frame data structure in order
> > to report xdp_buffer metadata. For the moment just hw checksum hints
> > are defined but flags field will be reused for xdp multi-buffer
> > For the moment just CHECKSUM_UNNECESSARY is supported.
> > CHECKSUM_COMPLETE will need to set csum value in metada space.
> >
> Lorenzo,

Hi Tom,

> 
> This isn't sufficient for the checksum-unnecessary interface, we'd
> also need ability to set csum_level for cases the device validated
> more than one checksum.

ack, right. I guess we can put this info in xdp metadata or do you think we can
add it in xdp_buff/xdp_frame as well?

> 
> IMO, we shouldn't support CHECKSUM_UNNECESSARY for new uses like this.
> For years now, the Linux community has been pleading with vendors to
> provide CHECKSUM_COMPLETE which is far more useful and robust than
> CHECSUM_UNNECESSARY, and yet some still haven't got with the program
> even though we see more and more instances where CHECKSUM_UNNECESSARY
> doesn't even work at all (e.g. cases with SRv6, new encaps device
> doesn't understand). I believe it's time to take a stand! :-)

I completely agree CHECKSUM_COMPLETE is more useful and robust than
CHECSUM_UNNECESSARY and I want to add support for it as soon as we
agree on the best way to do it. At the same time there are plenty of
XDP NICs where this feature is quite useful since they support just
CHECSUM_UNNECESSARY.

Regards,
Lorenzo

> 
> Tom
> 
> > Signed-off-by: David Ahern <dsahern@kernel.org>
> > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org>
> > ---
> >  include/net/xdp.h | 36 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 36 insertions(+)
> >
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 5533f0ab2afc..e81ac505752b 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -66,6 +66,13 @@ struct xdp_txq_info {
> >         struct net_device *dev;
> >  };
> >
> > +/* xdp metadata bitmask */
> > +#define XDP_CSUM_MASK          GENMASK(1, 0)
> > +enum xdp_flags {
> > +       XDP_CSUM_UNNECESSARY    = BIT(0),
> > +       XDP_CSUM_COMPLETE       = BIT(1),
> > +};
> > +
> >  struct xdp_buff {
> >         void *data;
> >         void *data_end;
> > @@ -74,6 +81,7 @@ struct xdp_buff {
> >         struct xdp_rxq_info *rxq;
> >         struct xdp_txq_info *txq;
> >         u32 frame_sz; /* frame size to deduce data_hard_end/reserved tailroom*/
> > +       u16 flags; /* xdp_flags */
> >  };
> >
> >  static __always_inline void
> > @@ -81,6 +89,7 @@ xdp_init_buff(struct xdp_buff *xdp, u32 frame_sz, struct xdp_rxq_info *rxq)
> >  {
> >         xdp->frame_sz = frame_sz;
> >         xdp->rxq = rxq;
> > +       xdp->flags = 0;
> >  }
> >
> >  static __always_inline void
> > @@ -95,6 +104,18 @@ xdp_prepare_buff(struct xdp_buff *xdp, unsigned char *hard_start,
> >         xdp->data_meta = meta_valid ? data : data + 1;
> >  }
> >
> > +static __always_inline void
> > +xdp_buff_get_csum(struct xdp_buff *xdp, struct sk_buff *skb)
> > +{
> > +       switch (xdp->flags & XDP_CSUM_MASK) {
> > +       case XDP_CSUM_UNNECESSARY:
> > +               skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +}
> > +
> >  /* Reserve memory area at end-of data area.
> >   *
> >   * This macro reserves tailroom in the XDP buffer by limiting the
> > @@ -122,8 +143,21 @@ struct xdp_frame {
> >          */
> >         struct xdp_mem_info mem;
> >         struct net_device *dev_rx; /* used by cpumap */
> > +       u16 flags; /* xdp_flags */
> >  };
> >
> > +static __always_inline void
> > +xdp_frame_get_csum(struct xdp_frame *xdpf, struct sk_buff *skb)
> > +{
> > +       switch (xdpf->flags & XDP_CSUM_MASK) {
> > +       case XDP_CSUM_UNNECESSARY:
> > +               skb->ip_summed = CHECKSUM_UNNECESSARY;
> > +               break;
> > +       default:
> > +               break;
> > +       }
> > +}
> > +
> >  #define XDP_BULK_QUEUE_SIZE    16
> >  struct xdp_frame_bulk {
> >         int count;
> > @@ -180,6 +214,7 @@ void xdp_convert_frame_to_buff(struct xdp_frame *frame, struct xdp_buff *xdp)
> >         xdp->data_end = frame->data + frame->len;
> >         xdp->data_meta = frame->data - frame->metasize;
> >         xdp->frame_sz = frame->frame_sz;
> > +       xdp->flags = frame->flags;
> >  }
> >
> >  static inline
> > @@ -206,6 +241,7 @@ int xdp_update_frame_from_buff(struct xdp_buff *xdp,
> >         xdp_frame->headroom = headroom - sizeof(*xdp_frame);
> >         xdp_frame->metasize = metasize;
> >         xdp_frame->frame_sz = xdp->frame_sz;
> > +       xdp_frame->flags = xdp->flags;
> >
> >         return 0;
> >  }
> > --
> > 2.31.1
> >

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

  parent reply	other threads:[~2021-05-29 13:34 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-28 17:43 [RFC bpf-next 0/4] add partial rx hw csum offload support for XDP Lorenzo Bianconi
2021-05-28 17:43 ` [RFC bpf-next 1/4] net: xdp: introduce flags field in xdp_buff and xdp_frame Lorenzo Bianconi
2021-05-28 21:18   ` Tom Herbert
2021-05-29  1:33     ` David Ahern
2021-05-29  4:56     ` Jakub Kicinski
2021-05-29 13:37       ` Lorenzo Bianconi
2021-05-29 13:34     ` Lorenzo Bianconi [this message]
2021-05-31 11:07       ` Jesper Dangaard Brouer
2021-05-28 17:43 ` [RFC bpf-next 2/4] mvneta: return csum computation result from mvneta_rx_csum Lorenzo Bianconi
2021-05-28 17:43 ` [RFC bpf-next 3/4] net: mvneta: report csum result in xdp_buff Lorenzo Bianconi
2021-05-28 17:43 ` [RFC bpf-next 4/4] net: xdp: update csum building the skb Lorenzo Bianconi
2021-05-28 18:01 ` [RFC bpf-next 0/4] add partial rx hw csum offload support for XDP David Ahern

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

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

  git send-email \
    --in-reply-to=YLJC2ox7HmAL8dnX@lore-desk \
    --to=lorenzo@kernel.org \
    --cc=ast@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=echaudro@redhat.com \
    --cc=john.fastabend@gmail.com \
    --cc=kuba@kernel.org \
    --cc=maciej.fijalkowski@intel.com \
    --cc=magnus.karlsson@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=toke@redhat.com \
    --cc=tom@herbertland.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;
as well as URLs for NNTP newsgroup(s).