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

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

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.