Re: [PATCH net-next v3 4/9] tsnep: Add XDP TX support

[Alexander Lobakin <alexandr.lobakin@intel.com>]

From: Gerhard Engleder <gerhard@engleder-embedded.com>
Date: Wed Jan 04 2023 20:41:27 GMT+0100

> Implement ndo_xdp_xmit() for XDP TX support. Support for fragmented XDP
> frames is included.
> 
> Signed-off-by: Gerhard Engleder <gerhard@engleder-embedded.com>
> ---
>  drivers/net/ethernet/engleder/tsnep.h      |  12 +-
>  drivers/net/ethernet/engleder/tsnep_main.c | 208 ++++++++++++++++++++-
>  2 files changed, 209 insertions(+), 11 deletions(-)

[...]

> diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
> index 56c8cae6251e..2c7252ded23a 100644
> --- a/drivers/net/ethernet/engleder/tsnep_main.c
> +++ b/drivers/net/ethernet/engleder/tsnep_main.c
> @@ -310,10 +310,11 @@ static void tsnep_tx_activate(struct tsnep_tx *tx, int index, int length,
>  	struct tsnep_tx_entry *entry = &tx->entry[index];
>  
>  	entry->properties = 0;
> -	if (entry->skb) {
> +	if (entry->skb || entry->xdpf) {
>  		entry->properties = length & TSNEP_DESC_LENGTH_MASK;
>  		entry->properties |= TSNEP_DESC_INTERRUPT_FLAG;
> -		if (skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)
> +		if (entry->type == TSNEP_TX_TYPE_SKB &&
> +		    skb_shinfo(entry->skb)->tx_flags & SKBTX_IN_PROGRESS)

Please enclose bitops (& here) hanging around any logical ops (&& here
in their own set of braces ().

>  			entry->properties |= TSNEP_DESC_EXTENDED_WRITEBACK_FLAG;
>  
>  		/* toggle user flag to prevent false acknowledge

[...]

> @@ -417,12 +420,13 @@ static int tsnep_tx_unmap(struct tsnep_tx *tx, int index, int count)
>  		entry = &tx->entry[(index + i) % TSNEP_RING_SIZE];
>  
>  		if (entry->len) {
> -			if (i == 0)
> +			if (i == 0 && entry->type == TSNEP_TX_TYPE_SKB)

`if (!i && ...)`, I think even checkpatch warns or was warning about
preferring !a over `a == 0` at some point.

>  				dma_unmap_single(dmadev,
>  						 dma_unmap_addr(entry, dma),
>  						 dma_unmap_len(entry, len),
>  						 DMA_TO_DEVICE);
> -			else
> +			else if (entry->type == TSNEP_TX_TYPE_SKB ||
> +				 entry->type == TSNEP_TX_TYPE_XDP_NDO)
>  				dma_unmap_page(dmadev,
>  					       dma_unmap_addr(entry, dma),
>  					       dma_unmap_len(entry, len),
> @@ -502,12 +506,134 @@ static netdev_tx_t tsnep_xmit_frame_ring(struct sk_buff *skb,
>  	return NETDEV_TX_OK;
>  }
>  
> +static int tsnep_xdp_tx_map(struct xdp_frame *xdpf, struct tsnep_tx *tx,
> +			    struct skb_shared_info *shinfo, int count,

I believe most of those pointers, if not all of them, can be const, you
only read data from them (probably except @tx).

> +			    enum tsnep_tx_type type)
> +{
> +	struct device *dmadev = tx->adapter->dmadev;
> +	struct tsnep_tx_entry *entry;
> +	struct page *page;
> +	skb_frag_t *frag;

This one as well (also please check for @page, can't say for sure).

> +	unsigned int len;
> +	int map_len = 0;
> +	dma_addr_t dma;
> +	void *data;
> +	int i;

[...]

> +		entry->len = len;
> +		dma_unmap_addr_set(entry, dma, dma);
> +
> +		entry->desc->tx = __cpu_to_le64(dma);
> +
> +		map_len += len;
> +
> +		if ((i + 1) < count) {

Those braces are redundant here.

> +			frag = &shinfo->frags[i];
> +			len = skb_frag_size(frag);
> +		}
> +	}
> +
> +	return map_len;
> +}
> +
> +/* This function requires __netif_tx_lock is held by the caller. */
> +static bool tsnep_xdp_xmit_frame_ring(struct xdp_frame *xdpf,
> +				      struct tsnep_tx *tx,
> +				      enum tsnep_tx_type type)
> +{
> +	struct skb_shared_info *shinfo = xdp_get_shared_info_from_frame(xdpf);

Same for this one (const).

> +	struct tsnep_tx_entry *entry;
> +	int count = 1;
> +	int length;
> +	int retval;
> +	int i;

Maybe squash some of them into one line as they are of the same type?

> +
> +	if (unlikely(xdp_frame_has_frags(xdpf)))
> +		count += shinfo->nr_frags;
> +
> +	spin_lock_bh(&tx->lock);

[...]

> +	retval = tsnep_xdp_tx_map(xdpf, tx, shinfo, count, type);
> +	if (retval < 0) {
> +		tsnep_tx_unmap(tx, tx->write, count);
> +		entry->xdpf = NULL;
> +
> +		tx->dropped++;
> +
> +		spin_unlock_bh(&tx->lock);
> +
> +		return false;
> +	}
> +	length = retval;
> +
> +	for (i = 0; i < count; i++)
> +		tsnep_tx_activate(tx, (tx->write + i) % TSNEP_RING_SIZE, length,
> +				  i == (count - 1));

Redundant braces around `count - 1`.

> +	tx->write = (tx->write + count) % TSNEP_RING_SIZE;
> +
> +	/* descriptor properties shall be valid before hardware is notified */
> +	dma_wmb();
> +
> +	spin_unlock_bh(&tx->lock);
> +
> +	return true;
> +}
> +
> +static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
> +{
> +	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
> +}
> +
>  static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  {
> -	int budget = 128;
>  	struct tsnep_tx_entry *entry;
> -	int count;
> +	struct xdp_frame_bulk bq;
> +	int budget = 128;

BTW, why do you ignore NAPI budget and use your own?

>  	int length;
> +	int count;
> +
> +	xdp_frame_bulk_init(&bq);
> +
> +	rcu_read_lock(); /* need for xdp_return_frame_bulk */
>  
>  	spin_lock_bh(&tx->lock);
>  

[...]

> @@ -552,8 +683,20 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  			skb_tstamp_tx(entry->skb, &hwtstamps);
>  		}
>  
> -		napi_consume_skb(entry->skb, budget);
> -		entry->skb = NULL;
> +		switch (entry->type) {
> +		case TSNEP_TX_TYPE_SKB:
> +			napi_consume_skb(entry->skb, budget);
> +			entry->skb = NULL;
> +			break;
> +		case TSNEP_TX_TYPE_XDP_TX:
> +			xdp_return_frame_rx_napi(entry->xdpf);
> +			entry->xdpf = NULL;
> +			break;
> +		case TSNEP_TX_TYPE_XDP_NDO:
> +			xdp_return_frame_bulk(entry->xdpf, &bq);
> +			entry->xdpf = NULL;
> +			break;
> +		}

entry ::skb and ::xdpf share the same slot, you could nullify it here
once instead of duplicating the same op across each case.

>  
>  		tx->read = (tx->read + count) % TSNEP_RING_SIZE;
>  
> @@ -570,6 +713,10 @@ static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
>  
>  	spin_unlock_bh(&tx->lock);
>  
> +	xdp_flush_frame_bulk(&bq);
> +
> +	rcu_read_unlock();
> +
>  	return (budget != 0);

Also redundant braces.

>  }
>  
> @@ -1330,6 +1477,46 @@ static ktime_t tsnep_netdev_get_tstamp(struct net_device *netdev,
>  	return ns_to_ktime(timestamp);
>  }
>  
> +static int tsnep_netdev_xdp_xmit(struct net_device *dev, int n,
> +				 struct xdp_frame **xdp, u32 flags)
> +{
> +	struct tsnep_adapter *adapter = netdev_priv(dev);
> +	int cpu = smp_processor_id();

Can be u32.

> +	struct netdev_queue *nq;
> +	int nxmit;
> +	int queue;

Squash?

> +	bool xmit;
> +
> +	if (unlikely(test_bit(__TSNEP_DOWN, &adapter->state)))
> +		return -ENETDOWN;
> +
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
> +		return -EINVAL;

[...]

Thanks,
Olek