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

[Stanislav Fomichev <sdf@google.com>]

On 10/23, Jakub Kicinski wrote:
> 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.

Ok. I'll cleanup xdp-rx-metadata in the same path. Might we worth it
to limit copy-paste spread..
 
> > > > +/* 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?

Sg, will rename.

> > > > +/* 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 :)

Good point on the descriptor bits. Let me try to move to a setsockopt.

> > 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.

Yeah, agreed, worst case we can have some static_branch to disable it.
But fair point that unlikely we'll see it cause any issues.