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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <64c056686b527_3a4d294e6@willemb.c.googlers.com.notmuch>
Date: Tue, 25 Jul 2023 19:10:32 -0400
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Stanislav Fomichev <sdf@...gle.com>, 
 Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc: bpf@...r.kernel.org, 
 ast@...nel.org, 
 daniel@...earbox.net, 
 andrii@...nel.org, 
 martin.lau@...ux.dev, 
 song@...nel.org, 
 yhs@...com, 
 john.fastabend@...il.com, 
 kpsingh@...nel.org, 
 haoluo@...gle.com, 
 jolsa@...nel.org, 
 kuba@...nel.org, 
 toke@...nel.org, 
 willemb@...gle.com, 
 dsahern@...nel.org, 
 magnus.karlsson@...el.com, 
 bjorn@...nel.org, 
 maciej.fijalkowski@...el.com, 
 hawk@...nel.org, 
 netdev@...r.kernel.org, 
 xdp-hints@...-project.net
Subject: Re: [RFC net-next v4 2/8] xsk: add TX timestamp and TX checksum
 offload support

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