On Thu, Jul 20, 2023 at 06:27:41PM -0400, Willem de Bruijn wrote:
> Zaremba, Larysa wrote:
> > On Thu, Jul 20, 2023 at 09:55:16AM -0400, Willem de Bruijn wrote:
> > > Zaremba, Larysa wrote:
> > > > On Thu, Jul 20, 2023 at 09:57:05AM +0000, Zaremba, Larysa wrote:
> > > > > On Wed, Jul 19, 2023 at 05:42:04PM -0400, Willem de Bruijn wrote:
> > > > > > Larysa Zaremba wrote:
> > > > > > > Implement functionality that enables drivers to expose to XDP code checksum
> > > > > > > information that consists of:
> > > > > > >
> > > > > > > - Checksum status - bitfield that consists of
> > > > > > > - number of consecutive validated checksums. This is almost the same as
> > > > > > > csum_level in skb, but starts with 1. Enum names for those bits still
> > > > > > > use checksum level concept, so it is less confusing for driver
> > > > > > > developers.
> > > > > > > - Is checksum partial? This bit cannot coexist with any other
> > > > > > > - Is there a complete checksum available?
> > > > > > > - Additional checksum data, a union of:
> > > > > > > - checksum start and offset, if checksum is partial
> > > > > > > - complete checksum, if available
> > > > > > >
> > > > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@xxxxxxxxx>
> > > > > > > ---
> > > > > > > Documentation/networking/xdp-rx-metadata.rst | 3 ++
> > > > > > > include/linux/netdevice.h | 3 ++
> > > > > > > include/net/xdp.h | 46 ++++++++++++++++++++
> > > > > > > kernel/bpf/offload.c | 2 +
> > > > > > > net/core/xdp.c | 23 ++++++++++
> > > > > > > 5 files changed, 77 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst
> > > > > > > index ea6dd79a21d3..7f056a44f682 100644
> > > > > > > --- a/Documentation/networking/xdp-rx-metadata.rst
> > > > > > > +++ b/Documentation/networking/xdp-rx-metadata.rst
> > > > > > > @@ -26,6 +26,9 @@ metadata is supported, this set will grow:
> > > > > > > .. kernel-doc:: net/core/xdp.c
> > > > > > > :identifiers: bpf_xdp_metadata_rx_vlan_tag
> > > > > > >
> > > > > > > +.. kernel-doc:: net/core/xdp.c
> > > > > > > + :identifiers: bpf_xdp_metadata_rx_csum
> > > > > > > +
> > > > > > > An XDP program can use these kfuncs to read the metadata into stack
> > > > > > > variables for its own consumption. Or, to pass the metadata on to other
> > > > > > > consumers, an XDP program can store it into the metadata area carried
> > > > > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > > > > > > index 1749f4f75c64..4f6da36ac123 100644
> > > > > > > --- a/include/linux/netdevice.h
> > > > > > > +++ b/include/linux/netdevice.h
> > > > > > > @@ -1660,6 +1660,9 @@ struct xdp_metadata_ops {
> > > > > > > enum xdp_rss_hash_type *rss_type);
> > > > > > > int (*xmo_rx_vlan_tag)(const struct xdp_md *ctx, u16 *vlan_tci,
> > > > > > > __be16 *vlan_proto);
> > > > > > > + int (*xmo_rx_csum)(const struct xdp_md *ctx,
> > > > > > > + enum xdp_csum_status *csum_status,
> > > > > > > + union xdp_csum_info *csum_info);
> > > > > > > };
> > > > > > >
> > > > > > > /**
> > > > > > > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > > > > > > index 89c58f56ffc6..2b7a7d678ff4 100644
> > > > > > > --- a/include/net/xdp.h
> > > > > > > +++ b/include/net/xdp.h
> > > > > > > @@ -391,6 +391,8 @@ void xdp_attachment_setup(struct xdp_attachment_info *info,
> > > > > > > bpf_xdp_metadata_rx_hash) \
> > > > > > > XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_VLAN_TAG, \
> > > > > > > bpf_xdp_metadata_rx_vlan_tag) \
> > > > > > > + XDP_METADATA_KFUNC(XDP_METADATA_KFUNC_RX_CSUM, \
> > > > > > > + bpf_xdp_metadata_rx_csum) \
> > > > > > >
> > > > > > > enum {
> > > > > > > #define XDP_METADATA_KFUNC(name, _) name,
> > > > > > > @@ -448,6 +450,50 @@ enum xdp_rss_hash_type {
> > > > > > > XDP_RSS_TYPE_L4_IPV6_SCTP_EX = XDP_RSS_TYPE_L4_IPV6_SCTP | XDP_RSS_L3_DYNHDR,
> > > > > > > };
> > > > > > >
> > > > > > > +union xdp_csum_info {
> > > > > > > + /* Checksum referred to by ``csum_start + csum_offset`` is considered
> > > > > > > + * valid, but was never calculated, TX device has to do this,
> > > > > > > + * starting from csum_start packet byte.
> > > > > > > + * Any preceding checksums are also considered valid.
> > > > > > > + * Available, if ``status == XDP_CHECKSUM_PARTIAL``.
> > > > > > > + */
> > > > > > > + struct {
> > > > > > > + u16 csum_start;
> > > > > > > + u16 csum_offset;
> > > > > > > + };
> > > > > > > +
> > > > > > > + /* Checksum, calculated over the whole packet.
> > > > > > > + * Available, if ``status & XDP_CHECKSUM_COMPLETE``.
> > > > > > > + */
> > > > > > > + u32 checksum;
> > > > > > > +};
> > > > > > > +
> > > > > > > +enum xdp_csum_status {
> > > > > > > + /* HW had parsed several transport headers and validated their
> > > > > > > + * checksums, same as ``CHECKSUM_UNNECESSARY`` in ``sk_buff``.
> > > > > > > + * 3 least significat bytes contain number of consecutive checksum,
> > > > > >
> > > > > > typo: significant
> > > > > >
> > > > > > (I did not scan for typos, just came across this when trying to understand
> > > > > > the skb->csum_level + 1 trick. Probably good to run a spell check).
> > > > > >
> > > >
> > > > Oh, and about skb->csum_level + 1, maybe this way it would be more
> > > > understandable: XDP_CHECKSUM_VALID_LVL0 + skb->csum_level?
> > >
> > > Agreed, that would help document the intent.
> > >
> > > > Using number of valid checksums (starts with 1) instead of checksum level
> > > > (starts with 0) is a debatable decision, but I have decided to go with it under
> > > > 2 assumptions:
> > > >
> > > > - the only reason checksum level in skb starts with 0 is to use less bits
> > > > - checksum number would be more intuitive for XDP/AF_XDP application developers
> > > >
> > > > I encourage everyone to share their opinion on that.
> > >
> > > I assumed this offset by one was because csum_status zero implicitly
> > > meant XDP_CHECKSUM_NONE. Is that not correct? That should probably
> > > get an explicit name.
> > >
> >
> > Well, I was not sure, whether I should add XDP_CHECKSUM_NONE, because it would
> > be equal to returning -ENODATA from kfunc, but after giving it some thought now,
> > it is worth to have XDP_CHECKSUM_NONE for packets that have no checksum to
> > check, like for hash there is XDP_RSS_TYPE_L2.
>
> On receive, CHECKSUM_NONE means that the packet has not been checked, not
> necessarily that it has no checksum. Perhaps the device was unable to
> parse the protocol.
>
> (on transmit, it conveys that a transmit checksum is not required.)
Oh, my bad, I have re-read the docs and for packets without checksum,
CHECKSUM_UNNECESSARY instead conveys "CRC in OK". In such case,
XDP_CHECKSUM_NONE becomes a full equivalent of returning -ENODATA from kfunc, so
I do not think XDP_CHECKSUM_NONE enum is worth including, because it coud lead
to new people writing programs in such way:
if (bpf_xdp_metadata_rx_csum(ctx, &csum_status, &rx_csum_info))
fallback();
if (csum_status == XDP_CHECKSUM_NONE)
fallback();
[...]
