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

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Sat, Jul 29, 2023 at 9:15 AM Willem de Bruijn
<willemdebruijn.kernel@gmail.com> wrote:
>
> Alexei Starovoitov wrote:
> > On Fri, Jul 28, 2023 at 07:39:14PM +0200, Larysa Zaremba wrote:
> > >
> > > +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_PARTIAL makes sense on TX, but this RX. I don't see in the above.
>
> It can be observed on RX when packets are looped.
>
> This may be observed even in XDP on veth.

veth and XDP is a broken combination. GSO packets coming out of containers
cannot be parsed properly by XDP.
It was added mainly for testing. Just like "generic XDP".
bpf progs at skb layer is much better fit for veth.

> > > +   /* Checksum, calculated over the whole packet.
> > > +    * Available, if ``status & XDP_CHECKSUM_COMPLETE``.
> > > +    */
> > > +   u32 checksum;
> >
> > imo XDP RX should only support XDP_CHECKSUM_COMPLETE with u32 checksum
> > or XDP_CHECKSUM_UNNECESSARY.
> >
> > > +};
> > > +
> > > +enum xdp_csum_status {
> > > +   /* HW had parsed several transport headers and validated their
> > > +    * checksums, same as ``CHECKSUM_UNNECESSARY`` in ``sk_buff``.
> > > +    * 3 least significant bytes contain number of consecutive checksums,
> > > +    * starting with the outermost, reported by hardware as valid.
> > > +    * ``sk_buff`` checksum level (``csum_level``) notation is provided
> > > +    * for driver developers.
> > > +    */
> > > +   XDP_CHECKSUM_VALID_LVL0         = 1,    /* 1 outermost checksum */
> > > +   XDP_CHECKSUM_VALID_LVL1         = 2,    /* 2 outermost checksums */
> > > +   XDP_CHECKSUM_VALID_LVL2         = 3,    /* 3 outermost checksums */
> > > +   XDP_CHECKSUM_VALID_LVL3         = 4,    /* 4 outermost checksums */
> > > +   XDP_CHECKSUM_VALID_NUM_MASK     = GENMASK(2, 0),
> > > +   XDP_CHECKSUM_VALID              = XDP_CHECKSUM_VALID_NUM_MASK,
> >
> > I don't see what bpf prog suppose to do with these levels.
> > The driver should pick between 3:
> > XDP_CHECKSUM_UNNECESSARY, XDP_CHECKSUM_COMPLETE, XDP_CHECKSUM_NONE.
> >
> > No levels and no anything partial. please.
>
> This levels business is an unfortunate side effect of
> CHECKSUM_UNNECESSARY. For a packet with multiple checksum fields, what
> does the boolean actually mean? With these levels, at least that is
> well defined: the first N checksum fields.

If I understand this correctly this is intel specific feature that
other NICs don't have. skb layer also doesn't have such concept.
The driver should say CHECKSUM_UNNECESSARY when it's sure
or don't pretend that it checks the checksum and just say NONE.