Re: [PATCH bpf-next v3 12/21] xdp: Add checksum hint

["Zaremba, Larysa" <larysa.zaremba@intel.com>]

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@intel.com>
> > > > > > > ---
> > > > > > >  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();
[...]