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

[Stanislav Fomichev <sdf@google.com>]

On 07/25, Simon Horman wrote:
> On Mon, Jul 24, 2023 at 04:59:51PM -0700, Stanislav Fomichev wrote:
> 
> ...
> 
> Hi Stan,
> 
> > diff --git a/include/uapi/linux/netdev.h b/include/uapi/linux/netdev.h
> > index bf71698a1e82..cf1e11c76339 100644
> > --- a/include/uapi/linux/netdev.h
> > +++ b/include/uapi/linux/netdev.h
> > @@ -37,11 +37,26 @@ enum netdev_xdp_act {
> >  	NETDEV_XDP_ACT_MASK = 127,
> >  };
> >  
> > +/**
> > + * enum netdev_xsk_flags
> > + * @NETDEV_XSK_FLAGS_TX_TIMESTAMP: HW timestamping egress packets is supported
> > + *   by the driver.
> > + * @NETDEV_XSK_FLAGS_TX_CHECKSUM: L3 checksum HW offload is supported by the
> > + *   driver.
> > + */
> > +enum netdev_xsk_flags {
> > +	NETDEV_XSK_FLAGS_TX_TIMESTAMP = 1,
> > +	NETDEV_XSK_FLAGS_TX_CHECKSUM = 2,
> > +
> 
> I know that it isn't the practice in this file.
> but adding the following makes kernel-doc happier
> about NETDEV_XSK_FLAGS_MASK not being documented.
> 
> 	/* private: */

This is autogenerated file :-( But I guess I can try to extend ynl
scripts to put this comment before the mask. Let me look into that...
 
 
> > +	NETDEV_XSK_FLAGS_MASK = 3,
> > +};
> > +
> >  enum {
> >  	NETDEV_A_DEV_IFINDEX = 1,
> >  	NETDEV_A_DEV_PAD,
> >  	NETDEV_A_DEV_XDP_FEATURES,
> >  	NETDEV_A_DEV_XDP_ZC_MAX_SEGS,
> > +	NETDEV_A_DEV_XSK_FEATURES,
> >  
> >  	__NETDEV_A_DEV_MAX,
> >  	NETDEV_A_DEV_MAX = (__NETDEV_A_DEV_MAX - 1)
> 
> ...
> 
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> 
> ...
> 
> > @@ -626,6 +635,7 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> >  static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >  				     struct xdp_desc *desc)
> >  {
> > +	struct xsk_tx_metadata *meta = NULL;
> >  	struct net_device *dev = xs->dev;
> >  	struct sk_buff *skb = xs->skb;
> >  	int err;
> > @@ -678,12 +688,40 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
> >  
> >  			skb_add_rx_frag(skb, nr_frags, page, 0, len, 0);
> >  		}
> > +
> > +		if (desc->options & XDP_TX_METADATA) {
> > +			if (unlikely(xs->tx_metadata_len == 0)) {
> > +				err = -EINVAL;
> > +				goto free_err;
> > +			}
> > +
> > +			meta = buffer - xs->tx_metadata_len;
> > +
> > +			if (meta->flags & XDP_TX_METADATA_CHECKSUM) {
> > +				if (unlikely(meta->csum_start + meta->csum_offset +
> > +					     sizeof(__sum16) > len)) {
> > +					err = -EINVAL;
> > +					goto free_err;
> > +				}
> > +
> > +				skb->csum_start = hr + meta->csum_start;
> 
> hr seems to only be set - by earlier, existing code in this function -
> if skb is NULL. Is it always safe to use it here?
> 
> Smatch flags hr as being potentially used uninitialised,
> the above is my understanding of why it thinks that is so.

Ugh, good point, I've missed that. This part is new and it's from
multi-frag support. I need to be more careful here and apply metadata
only from the first descriptor in the chain. Thanks!