Message-ID: <64a656273ee15_b20ce2087a@john.notmuch>
Date: Wed, 05 Jul 2023 22:50:31 -0700
From: John Fastabend <john.fastabend@...il.com>
To: Larysa Zaremba <larysa.zaremba@...el.com>, 
 Jesper Dangaard Brouer <jbrouer@...hat.com>
Cc: John Fastabend <john.fastabend@...il.com>, 
 brouer@...hat.com, 
 bpf@...r.kernel.org, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 martin.lau@...ux.dev, 
 song@...nel.org, 
 yhs@...com, 
 kpsingh@...nel.org, 
 sdf@...gle.com, 
 haoluo@...gle.com, 
 jolsa@...nel.org, 
 David Ahern <dsahern@...il.com>, 
 Jakub Kicinski <kuba@...nel.org>, 
 Willem de Bruijn <willemb@...gle.com>, 
 Anatoly Burakov <anatoly.burakov@...el.com>, 
 Alexander Lobakin <alexandr.lobakin@...el.com>, 
 Magnus Karlsson <magnus.karlsson@...il.com>, 
 Maryam Tahhan <mtahhan@...hat.com>, 
 xdp-hints@...-project.net, 
 netdev@...r.kernel.org, 
 "David S. Miller" <davem@...emloft.net>, 
 Alexander Duyck <alexander.duyck@...il.com>
Subject: Re: [PATCH bpf-next v2 12/20] xdp: Add checksum level hint

Larysa Zaremba wrote:
> On Tue, Jul 04, 2023 at 12:39:06PM +0200, Jesper Dangaard Brouer wrote:
> > Cc. DaveM+Alex Duyck, as I value your insights on checksums.
> > 
> > On 04/07/2023 11.24, Larysa Zaremba wrote:
> > > On Mon, Jul 03, 2023 at 01:38:27PM -0700, John Fastabend wrote:
> > > > Larysa Zaremba wrote:
> > > > > Implement functionality that enables drivers to expose to XDP code,
> > > > > whether checksums was checked and on what level.
> > > > > 
> > > > > Signed-off-by: Larysa Zaremba <larysa.zaremba@...el.com>
> > > > > ---
> > > > >   Documentation/networking/xdp-rx-metadata.rst |  3 +++
> > > > >   include/linux/netdevice.h                    |  1 +
> > > > >   include/net/xdp.h                            |  2 ++
> > > > >   kernel/bpf/offload.c                         |  2 ++
> > > > >   net/core/xdp.c                               | 21 ++++++++++++++++++++
> > > > >   5 files changed, 29 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/networking/xdp-rx-metadata.rst b/Documentation/networking/xdp-rx-metadata.rst
> > > > > index ea6dd79a21d3..4ec6ddfd2a52 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_lvl
> > > > > +
> > > > >   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 4fa4380e6d89..569563687172 100644
> > > > > --- a/include/linux/netdevice.h
> > > > > +++ b/include/linux/netdevice.h
> > > > > @@ -1660,6 +1660,7 @@ struct xdp_metadata_ops {
> > > > >   			       enum xdp_rss_hash_type *rss_type);
> > > > >   	int	(*xmo_rx_vlan_tag)(const struct xdp_md *ctx, u16 *vlan_tag,
> > > > >   				   __be16 *vlan_proto);
> > > > > +	int	(*xmo_rx_csum_lvl)(const struct xdp_md *ctx, u8 *csum_level);
> > > > >   };
> > > > >   /**
> > > > > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > > > > index 89c58f56ffc6..61ed38fa79d1 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_LVL, \
> > > > > +			   bpf_xdp_metadata_rx_csum_lvl) \
> > > > >   enum {
> > > > >   #define XDP_METADATA_KFUNC(name, _) name,
> > > > > diff --git a/kernel/bpf/offload.c b/kernel/bpf/offload.c
> > > > > index 986e7becfd42..a133fb775f49 100644
> > > > > --- a/kernel/bpf/offload.c
> > > > > +++ b/kernel/bpf/offload.c
> > > > > @@ -850,6 +850,8 @@ void *bpf_dev_bound_resolve_kfunc(struct bpf_prog *prog, u32 func_id)
> > > > >   		p = ops->xmo_rx_hash;
> > > > >   	else if (func_id == bpf_xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_VLAN_TAG))
> > > > >   		p = ops->xmo_rx_vlan_tag;
> > > > > +	else if (func_id == bpf_xdp_metadata_kfunc_id(XDP_METADATA_KFUNC_RX_CSUM_LVL))
> > > > > +		p = ops->xmo_rx_csum_lvl;
> > > > >   out:
> > > > >   	up_read(&bpf_devs_lock);
> > > > > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > > > > index f6262c90e45f..c666d3e0a26c 100644
> > > > > --- a/net/core/xdp.c
> > > > > +++ b/net/core/xdp.c
> > > > > @@ -758,6 +758,27 @@ __bpf_kfunc int bpf_xdp_metadata_rx_vlan_tag(const struct xdp_md *ctx, u16 *vlan
> > > > >   	return -EOPNOTSUPP;
> > > > >   }
> > > > > +/**
> > > > > + * bpf_xdp_metadata_rx_csum_lvl - Get depth at which HW has checked the checksum.
> > > > > + * @ctx: XDP context pointer.
> > > > > + * @csum_level: Return value pointer.
> > > > > + *
> > > > > + * In case of success, csum_level contains depth of the last verified checksum.
> > > > > + * If only the outermost checksum was verified, csum_level is 0, if both
> > > > > + * encapsulation and inner transport checksums were verified, csum_level is 1,
> > > > > + * and so on.
> > > > > + * For more details, refer to csum_level field in sk_buff.
> > > > > + *
> > > > > + * Return:
> > > > > + * * Returns 0 on success or ``-errno`` on error.
> > > > > + * * ``-EOPNOTSUPP`` : device driver doesn't implement kfunc
> > > > > + * * ``-ENODATA``    : Checksum was not validated
> > > > > + */
> > > > > +__bpf_kfunc int bpf_xdp_metadata_rx_csum_lvl(const struct xdp_md *ctx, u8 *csum_level)
> > > > 
> > > > Istead of ENODATA should we return what would be put in the ip_summed field
> > > > CHECKSUM_{NONE, UNNECESSARY, COMPLETE, PARTIAL}? Then sig would be,
> > 
> > I was thinking the same, what about checksum "type".
> > 
> > > > 
> > > >   bpf_xdp_metadata_rx_csum_lvl(const struct xdp_md *ctx, u8 *type, u8 *lvl);
> > > > 
> > > > or something like that? Or is the thought that its not really necessary?
> > > > I don't have a strong preference but figured it was worth asking.
> > > > 
> > > 
> > > I see no value in returning CHECKSUM_COMPLETE without the actual checksum value.
> > > Same with CHECKSUM_PARTIAL and csum_start. Returning those values too would
> > > overcomplicate the function signature.
> > 
> > So, this kfunc bpf_xdp_metadata_rx_csum_lvl() success is it equivilent to
> > CHECKSUM_UNNECESSARY?
> 
> This is 100% true for physical NICs, it's more complicated for veth, bacause it 
> often receives CHECKSUM_PARTIAL, which shouldn't normally apprear on RX, but is 
> treated by the network stack as a validated checksum, because there is no way 
> internally generated packet could be messed up. I would be grateful if you could 
> look at the veth patch and share your opinion about this.
> 
> > 
> > Looking at documentation[1] (generated from skbuff.h):
> >  [1] https://kernel.org/doc/html/latest/networking/skbuff.html#checksumming-of-received-packets-by-device
> > 
> > Is the idea that we can add another kfunc (new signature) than can deal
> > with the other types of checksums (in a later kernel release)?
> >
> 
> Yes, that is the idea.

If we think there is a chance we might need another kfunc we should add it
in the same kfunc. It would be unfortunate to have to do two kfuncs when
one would work. It shouldn't cost much/anything(?) to hardcode the type for
most cases? I think if we need it later I would advocate for updating this
kfunc to support it. Of course then userspace will have to swivel on the
kfunc signature.

Powered by blists - more mailing lists