[xdp-hints] Re: [PATCH bpf-next v4 02/11] xsk: Add TX timestamp and TX checksum offload support

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 23 Oct 2023 10:21:53 -0700 Stanislav Fomichev wrote:
> On 10/20, Jakub Kicinski wrote:
> > On Thu, 19 Oct 2023 10:49:35 -0700 Stanislav Fomichev wrote:  
> > > diff --git a/Documentation/netlink/specs/netdev.yaml b/Documentation/netlink/specs/netdev.yaml
> > > index 14511b13f305..22d2649a34ee 100644
> > > --- a/Documentation/netlink/specs/netdev.yaml
> > > +++ b/Documentation/netlink/specs/netdev.yaml
> > > @@ -55,6 +55,19 @@ name: netdev
> > >          name: hash
> > >          doc:
> > >            Device is capable of exposing receive packet hash via bpf_xdp_metadata_rx_hash().
> > > +  -
> > > +    type: flags
> > > +    name: xsk-flags
> > > +    render-max: true  
> > 
> > I don't think you're using the MAX, maybe don't render it.
> > IDK what purpose it'd serve for feature flag enums.  
> 
> I was gonna say 'to iterate over every possible bit', but we are using
> that 'xxx > 1U << i' implementation (which you also found a bug in).
> 
> I can drop it, but the question is: should I drop it from the rest as
> well? xdp-act and xdp-rx-metadata have it.

The xdp-act one looks used. xdp-rx-metadata looks unused, so you could
drop. But up to you if you want to clean it up.

> > > +/* Request transmit timestamp. Upon completion, put it into tx_timestamp
> > > + * field of struct xsk_tx_metadata.
> > > + */
> > > +#define XDP_TX_METADATA_TIMESTAMP		(1 << 0)
> > > +
> > > +/* Request transmit checksum offload. Checksum start position and offset
> > > + * are communicated via csum_start and csum_offset fields of struct
> > > + * xsk_tx_metadata.
> > > + */
> > > +#define XDP_TX_METADATA_CHECKSUM		(1 << 1)  
> > 
> > Reuse of enum netdev_xsk_flags is not an option?  
> 
> It is an option, but probably better to keep them separate? Netlink is
> for observability, and here have a tighter control over the defines and
> UAPI (and the don't have to map 1:1 as in the case of
> XDP_TX_METADATA_CHECKSUM_SW, for example).

The duplication is rather apparent, and they are flags so compiler
can't help us catch misuses of one set vs the other.

If you prefer to keep the separate defines - I'd rename them to tie 
them to the field more strongly. Specifically they should have the
word "flags" in them?

XDP_TXMD_FLAGS_TIMESTAMP
XDP_TXMD_FLAGS_CHECKSUM

maybe?

> > > +/* Force checksum calculation in software. Can be used for testing or
> > > + * working around potential HW issues. This option causes performance
> > > + * degradation and only works in XDP_COPY mode.
> > > + */
> > > +#define XDP_TX_METADATA_CHECKSUM_SW		(1 << 2)  
> > 
> > Is there a need for this to be on packet-by-packet basis?
> > HW issues should generally be fixed by the driver, is there 
> > any type of problem in particular you have in mind here?  
> 
> No, not really, do you think it makes sense to move it to a setsockopt
> or something? We'd still have to check it on a per-packet case
> though (from xsk_sock), so not sure it is strictly better?

Setsockopt or just ethtool -K $ifc tx off ? And check device features?
Maybe I'm overly sensitive but descriptor bits are usually super
precious :)

> Regarding HW issues: I don't have a good problem in mind, but I
> think having a SW path is useful. It least it was useful for me
> during developing (to compare the checksum) and I hope it will be
> useful for other people as well (mostly as well during development).
> Because the API is still a bit complicated and requires getting
> pseudo header csum right. Plus the fact that csum_offset is an
> offset from csum_start was not super intuitive to me.

Okay, I'm not strongly opposed, I just wanted to flag it.
If nobody else feels the same way, and you like the separate bit - 
perfectly fine by me.

> > > +			meta = buffer - xs->pool->tx_metadata_len;
> > > +
> > > +			if (meta->flags & XDP_TX_METADATA_CHECKSUM) {  
> > 
> > Do we need to worry about reserved / unsupported meta->flags ?  
> 
> I don't think so, probably not worth the cycles to check for the
> unsupported bits? Or do you think it makes sense to clearly return
> an error here and this extra check won't actually affect anything?

Hm, it is uAPI, isn't it? We try to validate anything kernel gets these
days, why would the flags be different? Shouldn't be more than 2 cycles.