Re: [PATCH net-next v4 09/10] tsnep: Add XDP RX support

On 10.01.23 18:40, Alexander H Duyck wrote:

On Mon, 2023-01-09 at 20:15 +0100, Gerhard Engleder wrote:

quoted

If BPF program is set up, then run BPF program for every received frame
and execute the selected action.

Signed-off-by: Gerhard Engleder <redacted>
---
  drivers/net/ethernet/engleder/tsnep_main.c | 122 ++++++++++++++++++++-
  1 file changed, 120 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/engleder/tsnep_main.c b/drivers/net/ethernet/engleder/tsnep_main.c
index 451ad1849b9d..002c879639db 100644
--- a/drivers/net/ethernet/engleder/tsnep_main.c
+++ b/drivers/net/ethernet/engleder/tsnep_main.c

@@ -27,6 +27,7 @@
  #include <linux/phy.h>
  #include <linux/iopoll.h>
  #include <linux/bpf.h>
+#include <linux/bpf_trace.h>
  
  #define TSNEP_SKB_PAD (NET_SKB_PAD + NET_IP_ALIGN)
  #define TSNEP_HEADROOM ALIGN(max(TSNEP_SKB_PAD, XDP_PACKET_HEADROOM), 4)

@@ -44,6 +45,9 @@
  #define TSNEP_COALESCE_USECS_MAX     ((ECM_INT_DELAY_MASK >> ECM_INT_DELAY_SHIFT) * \
  				      ECM_INT_DELAY_BASE_US + ECM_INT_DELAY_BASE_US - 1)
  
+#define TSNEP_XDP_TX		BIT(0)
+#define TSNEP_XDP_REDIRECT	BIT(1)
+
  enum {
  	__TSNEP_DOWN,
  };

@@ -625,6 +629,28 @@ static void tsnep_xdp_xmit_flush(struct tsnep_tx *tx)
  	iowrite32(TSNEP_CONTROL_TX_ENABLE, tx->addr + TSNEP_CONTROL);
  }
  
+static bool tsnep_xdp_xmit_back(struct tsnep_adapter *adapter,
+				struct xdp_buff *xdp,
+				struct netdev_queue *tx_nq, struct tsnep_tx *tx)
+{
+	struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp);
+	bool xmit;
+
+	if (unlikely(!xdpf))
+		return false;
+
+	__netif_tx_lock(tx_nq, smp_processor_id());
+
+	/* Avoid transmit queue timeout since we share it with the slow path */
+	txq_trans_cond_update(tx_nq);
+
+	xmit = tsnep_xdp_xmit_frame_ring(xdpf, tx, TSNEP_TX_TYPE_XDP_TX);
+

Again the trans_cond_update should be after the xmit and only if it is
not indicating it completed the transmit.

tsnep_xdp_xmit_frame_ring() only adds xpdf to the descriptor ring, so it
cannot complete the transmit. Therefore and in line with your previous
comment trans_cond_update() should be called here if xpdf is
successfully placed in the descriptor ring. Is that right?

quoted

+	__netif_tx_unlock(tx_nq);
+
+	return xmit;
+}
+
  static bool tsnep_tx_poll(struct tsnep_tx *tx, int napi_budget)
  {
  	struct tsnep_tx_entry *entry;

@@ -983,6 +1009,62 @@ static int tsnep_rx_refill(struct tsnep_rx *rx, int count, bool reuse)
  	return i;
  }
  
+static bool tsnep_xdp_run_prog(struct tsnep_rx *rx, struct bpf_prog *prog,
+			       struct xdp_buff *xdp, int *status,
+			       struct netdev_queue *tx_nq, struct tsnep_tx *tx)
+{
+	unsigned int length;
+	unsigned int sync;
+	u32 act;
+
+	length = xdp->data_end - xdp->data_hard_start - XDP_PACKET_HEADROOM;
+
+	act = bpf_prog_run_xdp(prog, xdp);
+
+	/* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */
+	sync = xdp->data_end - xdp->data_hard_start - XDP_PACKET_HEADROOM;
+	sync = max(sync, length);
+
+	switch (act) {
+	case XDP_PASS:
+		return false;
+	case XDP_TX:
+		if (!tsnep_xdp_xmit_back(rx->adapter, xdp, tx_nq, tx))
+			goto out_failure;
+		*status |= TSNEP_XDP_TX;
+		return true;
+	case XDP_REDIRECT:
+		if (xdp_do_redirect(rx->adapter->netdev, xdp, prog) < 0)
+			goto out_failure;
+		*status |= TSNEP_XDP_REDIRECT;
+		return true;
+	default:
+		bpf_warn_invalid_xdp_action(rx->adapter->netdev, prog, act);
+		fallthrough;
+	case XDP_ABORTED:
+out_failure:
+		trace_xdp_exception(rx->adapter->netdev, prog, act);
+		fallthrough;
+	case XDP_DROP:
+		page_pool_put_page(rx->page_pool, virt_to_head_page(xdp->data),
+				   sync, true);
+		return true;
+	}
+}
+
+static void tsnep_finalize_xdp(struct tsnep_adapter *adapter, int status,
+			       struct netdev_queue *tx_nq, struct tsnep_tx *tx)
+{
+	if (status & TSNEP_XDP_TX) {
+		__netif_tx_lock(tx_nq, smp_processor_id());
+		tsnep_xdp_xmit_flush(tx);
+		__netif_tx_unlock(tx_nq);
+	}
+
+	if (status & TSNEP_XDP_REDIRECT)
+		xdp_do_flush();
+}
+
  static struct sk_buff *tsnep_build_skb(struct tsnep_rx *rx, struct page *page,
  				       int length)
  {

@@ -1018,15 +1100,29 @@ static int tsnep_rx_poll(struct tsnep_rx *rx, struct napi_struct *napi,
  			 int budget)
  {
  	struct device *dmadev = rx->adapter->dmadev;
-	int desc_available;
-	int done = 0;
  	enum dma_data_direction dma_dir;
  	struct tsnep_rx_entry *entry;
+	struct netdev_queue *tx_nq;
+	struct bpf_prog *prog;
+	struct xdp_buff xdp;
  	struct sk_buff *skb;
+	struct tsnep_tx *tx;
+	int desc_available;
+	int xdp_status = 0;
+	int done = 0;
  	int length;
  
  	desc_available = tsnep_rx_desc_available(rx);
  	dma_dir = page_pool_get_dma_dir(rx->page_pool);
+	prog = READ_ONCE(rx->adapter->xdp_prog);
+	if (prog) {
+		int queue = smp_processor_id() % rx->adapter->num_tx_queues;
+

As I mentioned before. Take a look at how this was addressed in
skb_tx_hash. The modulus division is really expensive.

Also does this make sense. I am assuming you have a 1:1 Tx to Rx
mapping for your queues don't you? If so it might make more sense to
use the Tx queue that you clean in this queue pair.

Sounds reasonable. I will work on that.

Gerhard