[xdp-hints] Re: [RFC net-next v4 2/8] xsk: add TX timestamp and TX checksum offload support

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

Stanislav Fomichev wrote:
> On 07/25, Willem de Bruijn wrote:
> > Stanislav Fomichev wrote:
> > > This change actually defines the (initial) metadata layout
> > > that should be used by AF_XDP userspace (xsk_tx_metadata).
> > > The first field is flags which requests appropriate offloads,
> > > followed by the offload-specific fields. The supported per-device
> > > offloads are exported via netlink (new xsk-flags).
> > > 
> > > The offloads themselves are still implemented in a bit of a
> > > framework-y fashion that's left from my initial kfunc attempt.
> > > I'm introducing new xsk_tx_metadata_ops which drivers are
> > > supposed to implement. The drivers are also supposed
> > > to call xsk_tx_metadata_request/xsk_tx_metadata_complete in
> > > the right places. Since xsk_tx_metadata_{request,_complete}
> > > are static inline, we don't incur any extra overhead doing
> > > indirect calls.
> > > 
> > > The benefit of this scheme is as follows:
> > > - keeps all metadata layout parsing away from driver code
> > > - makes it easy to grep and see which drivers implement what
> > > - don't need any extra flags to maintain to keep track of that
> > >   offloads are implemented; if the callback is implemented - the offload
> > >   is supported (used by netlink reporting code)
> > > 
> > > Two offloads are defined right now:
> > > 1. XDP_TX_METADATA_CHECKSUM: skb-style csum_start+csum_offset
> > > 2. XDP_TX_METADATA_TIMESTAMP: writes TX timestamp back into metadata
> > >    area upon completion (tx_timestamp field)
> > > 
> > > The offloads are also implemented for copy mode:
> > > 1. Extra XDP_TX_METADATA_CHECKSUM_SW to trigger skb_checksum_help; this
> > >    might be useful as a reference implementation and for testing
> > > 2. XDP_TX_METADATA_TIMESTAMP writes SW timestamp from the skb
> > >    destructor (note I'm reusing hwtstamps to pass metadata pointer)
> > > 
> > > The struct is forward-compatible and can be extended in the future
> > > by appending more fields.
> > > 
> > > Signed-off-by: Stanislav Fomichev <sdf@google.com>

> > > +/* 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)
> > > +
> > > +/* 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)
> > 
> > Not sure how useful this is, especially if only for copy mode.
> 
> Seems useful at least as a reference implementation? But I'm happy
> to drop. It's used only in the tests for now. I was using it to
> verify csum_offset/start field values.

If testing over veth, does anything even look at the checksum?

> > > +struct xsk_tx_metadata {
> > > +	__u32 flags;
> > > +
> > > +	/* XDP_TX_METADATA_CHECKSUM */
> > > +
> > > +	/* Offset from desc->addr where checksumming should start. */
> > > +	__u16 csum_start;
> > > +	/* Offset from csum_start where checksum should be stored. */
> > > +	__u16 csum_offset;
> > > +
> > > +	/* XDP_TX_METADATA_TIMESTAMP */
> > > +
> > > +	__u64 tx_timestamp;
> > > +};
> > 
> > Is this structure easily extensible for future offloads,
> > such as USO?
> 
> We can append more field. What do we need for USO? Something akin
> to gso_size/gso_segs/gso_type ?

Yes, a bit to set the feature (gso_type) and a field to store the
segment size (gso_size).

Pacing offload is the other feature that comes to mind. That could
conceivably use the tx_timestamp field.