Re: [PATCH net-next v3 5/6] sfc: handle XDP_TX outcomes of XDP eBPF programs

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Wed, 30 Oct 2019 10:52:04 +0000
Charles McLachlan <cmclachlan@solarflare.com> wrote:

> +/* Transmit a packet from an XDP buffer
> + *
> + * Returns number of packets sent on success, error code otherwise.
> + * Runs in NAPI context, either in our poll (for XDP TX) or a different NIC
> + * (for XDP redirect).
> + */
> +int efx_xdp_tx_buffers(struct efx_nic *efx, int n, struct xdp_frame **xdpfs,
> +		       bool flush)
> +{
> +	struct efx_tx_buffer *tx_buffer;
> +	struct efx_tx_queue *tx_queue;
> +	struct xdp_frame *xdpf;
> +	dma_addr_t dma_addr;
> +	unsigned int len;
> +	int space;
> +	int cpu;
> +	int i;
> +
> +	cpu = raw_smp_processor_id();
> +
> +	if (!efx->xdp_tx_queue_count ||
> +	    unlikely(cpu >= efx->xdp_tx_queue_count))
> +		return -EINVAL;
> +
> +	tx_queue = efx->xdp_tx_queues[cpu];
> +	if (unlikely(!tx_queue))
> +		return -EINVAL;
> +

If you return a negative value, then it is the caller responsibility
to free *all* the xdp_frame's, e.g. see bq_xmit_all() in devmap.c.

BUT if you start to process packets, and return a positive value, then
this function MUST handle freeing the frames it couldn't send.

> +	if (n && xdpfs) {
> +		/* Check for available space. We should never need multiple
> +		 * descriptors per frame.
> +		 */
> +		space = efx->txq_entries +
> +			tx_queue->read_count - tx_queue->insert_count;
> +		n = min(n, space);

This looks broken.  In case 'space' is too small, there will be
xdp_frame's left in xdpfs[] that was not send.  As desc above, this
function have to free those.


> +		for (i = 0; i < n; i++) {
> +			xdpf = xdpfs[i];
> +
> +			/* We'll want a descriptor for this tx. */
> +			prefetchw(__efx_tx_queue_get_insert_buffer(tx_queue));
> +
> +			len = xdpf->len;
> +
> +			/* Map for DMA. */
> +			dma_addr = dma_map_single(&efx->pci_dev->dev,
> +						  xdpf->data, len,
> +						  DMA_TO_DEVICE);
> +			if (dma_mapping_error(&efx->pci_dev->dev, dma_addr))
> +				return -EIO;
> +
> +			/*  Create descriptor and set up for unmapping DMA. */
> +			tx_buffer = efx_tx_map_chunk(tx_queue, dma_addr, len);
> +			tx_buffer->xdpf = xdpf;
> +			tx_buffer->flags = EFX_TX_BUF_XDP |
> +					   EFX_TX_BUF_MAP_SINGLE;
> +			tx_buffer->dma_offset = 0;
> +			tx_buffer->unmap_len = len;
> +		}
> +	}
> +
> +	/* Pass to hardware. */
> +	if (flush)
> +		efx_nic_push_buffers(tx_queue);
> +
> +	tx_queue->tx_packets += n;
> +
> +	return n;
> +}
> +


-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer