[PATCH net-next 0/4] bnxt_en: Add XDP_REDIRECT support.

[PATCH net-next 0/4] bnxt_en: Add XDP_REDIRECT support.

[Michael Chan]

This patch series adds XDP_REDIRECT support by Andy Gospodarek.

Andy Gospodarek (3):
  bnxt_en: rename some xdp functions
  bnxt_en: optimized XDP_REDIRECT support
  bnxt_en: add page_pool support

Michael Chan (1):
  bnxt_en: Refactor __bnxt_xmit_xdp().

 drivers/net/ethernet/broadcom/Kconfig             |   1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt.c         |  65 +++++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  17 ++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |   2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c     | 144 +++++++++++++++++++---
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h     |   7 +-
 6 files changed, 208 insertions(+), 28 deletions(-)

-- 
2.5.1

[PATCH net-next 1/4] bnxt_en: rename some xdp functions

[Michael Chan]

From: Andy Gospodarek <gospo@broadcom.com>

Renaming bnxt_xmit_xdp to __bnxt_xmit_xdp to get ready for XDP_REDIRECT
support and reduce confusion/namespace collision.

Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c | 2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c     | 8 ++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h     | 4 ++--
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index a6c7baf..21a0431 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -2799,7 +2799,7 @@ static int bnxt_run_loopback(struct bnxt *bp)
 		dev_kfree_skb(skb);
 		return -EIO;
 	}
-	bnxt_xmit_xdp(bp, txr, map, pkt_size, 0);
+	__bnxt_xmit_xdp(bp, txr, map, pkt_size, 0);
 
 	/* Sync BD data before updating doorbell */
 	wmb();
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 0184ef6..4bc9595 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -19,8 +19,8 @@
 #include "bnxt.h"
 #include "bnxt_xdp.h"
 
-void bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
-		   dma_addr_t mapping, u32 len, u16 rx_prod)
+void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
+		     dma_addr_t mapping, u32 len, u16 rx_prod)
 {
 	struct bnxt_sw_tx_bd *tx_buf;
 	struct tx_bd *txbd;
@@ -132,8 +132,8 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 		*event = BNXT_TX_EVENT;
 		dma_sync_single_for_device(&pdev->dev, mapping + offset, *len,
 					   bp->rx_dir);
-		bnxt_xmit_xdp(bp, txr, mapping + offset, *len,
-			      NEXT_RX(rxr->rx_prod));
+		__bnxt_xmit_xdp(bp, txr, mapping + offset, *len,
+				NEXT_RX(rxr->rx_prod));
 		bnxt_reuse_rx_data(rxr, cons, page);
 		return true;
 	default:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
index 414b748..b36087b 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
@@ -10,8 +10,8 @@
 #ifndef BNXT_XDP_H
 #define BNXT_XDP_H
 
-void bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
-		   dma_addr_t mapping, u32 len, u16 rx_prod);
+void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
+		     dma_addr_t mapping, u32 len, u16 rx_prod);
 void bnxt_tx_int_xdp(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts);
 bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 		 struct page *page, u8 **data_ptr, unsigned int *len,
-- 
2.5.1

[PATCH net-next 2/4] bnxt_en: Refactor __bnxt_xmit_xdp().

[Michael Chan]

__bnxt_xmit_xdp() is used by XDP_TX and ethtool loopback packet transmit.
Refactor it so that it can be re-used by the XDP_REDIRECT logic.
Restructure the TX interrupt handler logic to cleanly separate XDP_TX
logic in preparation for XDP_REDIRECT.

Acked-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.h         |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  2 +-
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c     | 33 ++++++++++++++++-------
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h     |  5 ++--
 4 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 4b3ae92..bf12cfc 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -596,6 +596,7 @@ struct bnxt_sw_tx_bd {
 	DEFINE_DMA_UNMAP_ADDR(mapping);
 	u8			is_gso;
 	u8			is_push;
+	u8			action;
 	union {
 		unsigned short		nr_frags;
 		u16			rx_prod;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index 21a0431..a0f3277 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -2799,7 +2799,7 @@ static int bnxt_run_loopback(struct bnxt *bp)
 		dev_kfree_skb(skb);
 		return -EIO;
 	}
-	__bnxt_xmit_xdp(bp, txr, map, pkt_size, 0);
+	bnxt_xmit_bd(bp, txr, map, pkt_size);
 
 	/* Sync BD data before updating doorbell */
 	wmb();
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 4bc9595..41e232e 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -19,8 +19,9 @@
 #include "bnxt.h"
 #include "bnxt_xdp.h"
 
-void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
-		     dma_addr_t mapping, u32 len, u16 rx_prod)
+struct bnxt_sw_tx_bd *bnxt_xmit_bd(struct bnxt *bp,
+				   struct bnxt_tx_ring_info *txr,
+				   dma_addr_t mapping, u32 len)
 {
 	struct bnxt_sw_tx_bd *tx_buf;
 	struct tx_bd *txbd;
@@ -29,7 +30,6 @@ void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 
 	prod = txr->tx_prod;
 	tx_buf = &txr->tx_buf_ring[prod];
-	tx_buf->rx_prod = rx_prod;
 
 	txbd = &txr->tx_desc_ring[TX_RING(prod)][TX_IDX(prod)];
 	flags = (len << TX_BD_LEN_SHIFT) | (1 << TX_BD_FLAGS_BD_CNT_SHIFT) |
@@ -40,30 +40,43 @@ void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 
 	prod = NEXT_TX(prod);
 	txr->tx_prod = prod;
+	return tx_buf;
+}
+
+static void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
+			    dma_addr_t mapping, u32 len, u16 rx_prod)
+{
+	struct bnxt_sw_tx_bd *tx_buf;
+
+	tx_buf = bnxt_xmit_bd(bp, txr, mapping, len);
+	tx_buf->rx_prod = rx_prod;
+	tx_buf->action = XDP_TX;
 }
 
 void bnxt_tx_int_xdp(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
 {
 	struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
 	struct bnxt_rx_ring_info *rxr = bnapi->rx_ring;
+	bool rx_doorbell_needed = false;
 	struct bnxt_sw_tx_bd *tx_buf;
 	u16 tx_cons = txr->tx_cons;
 	u16 last_tx_cons = tx_cons;
-	u16 rx_prod;
 	int i;
 
 	for (i = 0; i < nr_pkts; i++) {
-		last_tx_cons = tx_cons;
+		tx_buf = &txr->tx_buf_ring[tx_cons];
+
+		if (tx_buf->action == XDP_TX) {
+			rx_doorbell_needed = true;
+			last_tx_cons = tx_cons;
+		}
 		tx_cons = NEXT_TX(tx_cons);
 	}
 	txr->tx_cons = tx_cons;
-	if (bnxt_tx_avail(bp, txr) == bp->tx_ring_size) {
-		rx_prod = rxr->rx_prod;
-	} else {
+	if (rx_doorbell_needed) {
 		tx_buf = &txr->tx_buf_ring[last_tx_cons];
-		rx_prod = tx_buf->rx_prod;
+		bnxt_db_write(bp, &rxr->rx_db, tx_buf->rx_prod);
 	}
-	bnxt_db_write(bp, &rxr->rx_db, rx_prod);
 }
 
 /* returns the following:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
index b36087b..20e470c 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
@@ -10,8 +10,9 @@
 #ifndef BNXT_XDP_H
 #define BNXT_XDP_H
 
-void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
-		     dma_addr_t mapping, u32 len, u16 rx_prod);
+struct bnxt_sw_tx_bd *bnxt_xmit_bd(struct bnxt *bp,
+				   struct bnxt_tx_ring_info *txr,
+				   dma_addr_t mapping, u32 len);
 void bnxt_tx_int_xdp(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts);
 bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 		 struct page *page, u8 **data_ptr, unsigned int *len,
-- 
2.5.1

[PATCH net-next 3/4] bnxt_en: optimized XDP_REDIRECT support

[Michael Chan]

From: Andy Gospodarek <gospo@broadcom.com>

This adds basic support for XDP_REDIRECT in the bnxt_en driver.  Next
patch adds the more optimized page pool support.

Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     |  27 ++++++-
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  13 +++-
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 108 ++++++++++++++++++++++++--
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h |   2 +
 4 files changed, 140 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index b7b6227..d8f0846 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -1989,6 +1989,9 @@ static int __bnxt_poll_work(struct bnxt *bp, struct bnxt_cp_ring_info *cpr,
 		}
 	}
 
+	if (event & BNXT_REDIRECT_EVENT)
+		xdp_do_flush_map();
+
 	if (event & BNXT_TX_EVENT) {
 		struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
 		u16 prod = txr->tx_prod;
@@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
 
 		for (j = 0; j < max_idx;) {
 			struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
-			struct sk_buff *skb = tx_buf->skb;
+			struct sk_buff *skb;
 			int k, last;
 
+			if (i < bp->tx_nr_rings_xdp &&
+			    tx_buf->action == XDP_REDIRECT) {
+				dma_unmap_single(&pdev->dev,
+					dma_unmap_addr(tx_buf, mapping),
+					dma_unmap_len(tx_buf, len),
+					PCI_DMA_TODEVICE);
+				xdp_return_frame(tx_buf->xdpf);
+				tx_buf->action = 0;
+				tx_buf->xdpf = NULL;
+				j++;
+				continue;
+			}
+
+			skb = tx_buf->skb;
 			if (!skb) {
 				j++;
 				continue;
@@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
 		if (rc < 0)
 			return rc;
 
+		rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
+						MEM_TYPE_PAGE_SHARED, NULL);
+		if (rc) {
+			xdp_rxq_info_unreg(&rxr->xdp_rxq);
+			return rc;
+		}
+
 		rc = bnxt_alloc_ring(bp, &ring->ring_mem);
 		if (rc)
 			return rc;
@@ -10233,6 +10257,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
 	.ndo_udp_tunnel_add	= bnxt_udp_tunnel_add,
 	.ndo_udp_tunnel_del	= bnxt_udp_tunnel_del,
 	.ndo_bpf		= bnxt_xdp,
+	.ndo_xdp_xmit		= bnxt_xdp_xmit,
 	.ndo_bridge_getlink	= bnxt_bridge_getlink,
 	.ndo_bridge_setlink	= bnxt_bridge_setlink,
 	.ndo_get_devlink_port	= bnxt_get_devlink_port,
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index bf12cfc..8ac51fa 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -587,13 +587,18 @@ struct nqe_cn {
 #define BNXT_HWRM_CHNL_CHIMP	0
 #define BNXT_HWRM_CHNL_KONG	1
 
-#define BNXT_RX_EVENT	1
-#define BNXT_AGG_EVENT	2
-#define BNXT_TX_EVENT	4
+#define BNXT_RX_EVENT		1
+#define BNXT_AGG_EVENT		2
+#define BNXT_TX_EVENT		4
+#define BNXT_REDIRECT_EVENT	8
 
 struct bnxt_sw_tx_bd {
-	struct sk_buff		*skb;
+	union {
+		struct sk_buff		*skb;
+		struct xdp_frame	*xdpf;
+	};
 	DEFINE_DMA_UNMAP_ADDR(mapping);
+	DEFINE_DMA_UNMAP_LEN(len);
 	u8			is_gso;
 	u8			is_push;
 	u8			action;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 41e232e..12489d2 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -53,6 +53,20 @@ static void __bnxt_xmit_xdp(struct bnxt *bp, struct bnxt_tx_ring_info *txr,
 	tx_buf->action = XDP_TX;
 }
 
+static void __bnxt_xmit_xdp_redirect(struct bnxt *bp,
+				     struct bnxt_tx_ring_info *txr,
+				     dma_addr_t mapping, u32 len,
+				     struct xdp_frame *xdpf)
+{
+	struct bnxt_sw_tx_bd *tx_buf;
+
+	tx_buf = bnxt_xmit_bd(bp, txr, mapping, len);
+	tx_buf->action = XDP_REDIRECT;
+	tx_buf->xdpf = xdpf;
+	dma_unmap_addr_set(tx_buf, mapping, mapping);
+	dma_unmap_len_set(tx_buf, len, 0);
+}
+
 void bnxt_tx_int_xdp(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
 {
 	struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
@@ -66,7 +80,17 @@ void bnxt_tx_int_xdp(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
 	for (i = 0; i < nr_pkts; i++) {
 		tx_buf = &txr->tx_buf_ring[tx_cons];
 
-		if (tx_buf->action == XDP_TX) {
+		if (tx_buf->action == XDP_REDIRECT) {
+			struct pci_dev *pdev = bp->pdev;
+
+			dma_unmap_single(&pdev->dev,
+					 dma_unmap_addr(tx_buf, mapping),
+					 dma_unmap_len(tx_buf, len),
+					 PCI_DMA_TODEVICE);
+			xdp_return_frame(tx_buf->xdpf);
+			tx_buf->action = 0;
+			tx_buf->xdpf = NULL;
+		} else if (tx_buf->action == XDP_TX) {
 			rx_doorbell_needed = true;
 			last_tx_cons = tx_cons;
 		}
@@ -101,19 +125,19 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 		return false;
 
 	pdev = bp->pdev;
-	txr = rxr->bnapi->tx_ring;
 	rx_buf = &rxr->rx_buf_ring[cons];
 	offset = bp->rx_offset;
 
+	mapping = rx_buf->mapping - bp->rx_dma_offset;
+	dma_sync_single_for_cpu(&pdev->dev, mapping + offset, *len, bp->rx_dir);
+
+	txr = rxr->bnapi->tx_ring;
 	xdp.data_hard_start = *data_ptr - offset;
 	xdp.data = *data_ptr;
 	xdp_set_data_meta_invalid(&xdp);
 	xdp.data_end = *data_ptr + *len;
 	xdp.rxq = &rxr->xdp_rxq;
 	orig_data = xdp.data;
-	mapping = rx_buf->mapping - bp->rx_dma_offset;
-
-	dma_sync_single_for_cpu(&pdev->dev, mapping + offset, *len, bp->rx_dir);
 
 	rcu_read_lock();
 	act = bpf_prog_run_xdp(xdp_prog, &xdp);
@@ -149,6 +173,30 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 				NEXT_RX(rxr->rx_prod));
 		bnxt_reuse_rx_data(rxr, cons, page);
 		return true;
+	case XDP_REDIRECT:
+		/* if we are calling this here then we know that the
+		 * redirect is coming from a frame received by the
+		 * bnxt_en driver.
+		 */
+		dma_unmap_page_attrs(&pdev->dev, mapping,
+				     PAGE_SIZE, bp->rx_dir,
+				     DMA_ATTR_WEAK_ORDERING);
+
+		/* if we are unable to allocate a new buffer, abort and reuse */
+		if (bnxt_alloc_rx_data(bp, rxr, rxr->rx_prod, GFP_ATOMIC)) {
+			trace_xdp_exception(bp->dev, xdp_prog, act);
+			bnxt_reuse_rx_data(rxr, cons, page);
+			return true;
+		}
+
+		if (xdp_do_redirect(bp->dev, &xdp, xdp_prog)) {
+			trace_xdp_exception(bp->dev, xdp_prog, act);
+			__free_page(page);
+			return true;
+		}
+
+		*event |= BNXT_REDIRECT_EVENT;
+		break;
 	default:
 		bpf_warn_invalid_xdp_action(act);
 		/* Fall thru */
@@ -162,6 +210,56 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 	return true;
 }
 
+int bnxt_xdp_xmit(struct net_device *dev, int num_frames,
+		  struct xdp_frame **frames, u32 flags)
+{
+	struct bnxt *bp = netdev_priv(dev);
+	struct bpf_prog *xdp_prog = READ_ONCE(bp->xdp_prog);
+	struct pci_dev *pdev = bp->pdev;
+	struct bnxt_tx_ring_info *txr;
+	dma_addr_t mapping;
+	int drops = 0;
+	int ring;
+	int i;
+
+	if (!test_bit(BNXT_STATE_OPEN, &bp->state) ||
+	    !bp->tx_nr_rings_xdp ||
+	    !xdp_prog)
+		return -EINVAL;
+
+	ring = smp_processor_id() % bp->tx_nr_rings_xdp;
+	txr = &bp->tx_ring[ring];
+
+	for (i = 0; i < num_frames; i++) {
+		struct xdp_frame *xdp = frames[i];
+
+		if (!txr || !bnxt_tx_avail(bp, txr) ||
+		    !(bp->bnapi[ring]->flags & BNXT_NAPI_FLAG_XDP)) {
+			xdp_return_frame_rx_napi(xdp);
+			drops++;
+			continue;
+		}
+
+		mapping = dma_map_single(&pdev->dev, xdp->data, xdp->len,
+					 DMA_TO_DEVICE);
+
+		if (dma_mapping_error(&pdev->dev, mapping)) {
+			xdp_return_frame_rx_napi(xdp);
+			drops++;
+			continue;
+		}
+		__bnxt_xmit_xdp_redirect(bp, txr, mapping, xdp->len, xdp);
+	}
+
+	if (flags & XDP_XMIT_FLUSH) {
+		/* Sync BD data before updating doorbell */
+		wmb();
+		bnxt_db_write(bp, &txr->tx_db, txr->tx_prod);
+	}
+
+	return num_frames - drops;
+}
+
 /* Under rtnl_lock */
 static int bnxt_xdp_set(struct bnxt *bp, struct bpf_prog *prog)
 {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
index 20e470c..0df40c3 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.h
@@ -18,5 +18,7 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 		 struct page *page, u8 **data_ptr, unsigned int *len,
 		 u8 *event);
 int bnxt_xdp(struct net_device *dev, struct netdev_bpf *xdp);
+int bnxt_xdp_xmit(struct net_device *dev, int num_frames,
+		  struct xdp_frame **frames, u32 flags);
 
 #endif
-- 
2.5.1

[PATCH net-next 4/4] bnxt_en: add page_pool support

[Michael Chan]

From: Andy Gospodarek <gospo@broadcom.com>

This removes contention over page allocation for XDP_REDIRECT actions by
adding page_pool support per queue for the driver.  The performance for
XDP_REDIRECT actions scales linearly with the number of cores performing
redirect actions when using the page pools instead of the standard page
allocator.

Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
Signed-off-by: Michael Chan <michael.chan@broadcom.com>
---
 drivers/net/ethernet/broadcom/Kconfig         |  1 +
 drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 40 +++++++++++++++++++++++----
 drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 ++
 drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  3 +-
 4 files changed, 41 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/Kconfig b/drivers/net/ethernet/broadcom/Kconfig
index 2e4a8c7..e9017ca 100644
--- a/drivers/net/ethernet/broadcom/Kconfig
+++ b/drivers/net/ethernet/broadcom/Kconfig
@@ -199,6 +199,7 @@ config BNXT
 	select FW_LOADER
 	select LIBCRC32C
 	select NET_DEVLINK
+	select PAGE_POOL
 	---help---
 	  This driver supports Broadcom NetXtreme-C/E 10/25/40/50 gigabit
 	  Ethernet cards.  To compile this driver as a module, choose M here:
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index d8f0846..b6777e5 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -54,6 +54,7 @@
 #include <net/pkt_cls.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
+#include <net/page_pool.h>
 
 #include "bnxt_hsi.h"
 #include "bnxt.h"
@@ -668,19 +669,20 @@ static void bnxt_tx_int(struct bnxt *bp, struct bnxt_napi *bnapi, int nr_pkts)
 }
 
 static struct page *__bnxt_alloc_rx_page(struct bnxt *bp, dma_addr_t *mapping,
+					 struct bnxt_rx_ring_info *rxr,
 					 gfp_t gfp)
 {
 	struct device *dev = &bp->pdev->dev;
 	struct page *page;
 
-	page = alloc_page(gfp);
+	page = page_pool_dev_alloc_pages(rxr->page_pool);
 	if (!page)
 		return NULL;
 
 	*mapping = dma_map_page_attrs(dev, page, 0, PAGE_SIZE, bp->rx_dir,
 				      DMA_ATTR_WEAK_ORDERING);
 	if (dma_mapping_error(dev, *mapping)) {
-		__free_page(page);
+		page_pool_recycle_direct(rxr->page_pool, page);
 		return NULL;
 	}
 	*mapping += bp->rx_dma_offset;
@@ -716,7 +718,8 @@ int bnxt_alloc_rx_data(struct bnxt *bp, struct bnxt_rx_ring_info *rxr,
 	dma_addr_t mapping;
 
 	if (BNXT_RX_PAGE_MODE(bp)) {
-		struct page *page = __bnxt_alloc_rx_page(bp, &mapping, gfp);
+		struct page *page =
+			__bnxt_alloc_rx_page(bp, &mapping, rxr, gfp);
 
 		if (!page)
 			return -ENOMEM;
@@ -2360,7 +2363,7 @@ static void bnxt_free_rx_skbs(struct bnxt *bp)
 				dma_unmap_page_attrs(&pdev->dev, mapping,
 						     PAGE_SIZE, bp->rx_dir,
 						     DMA_ATTR_WEAK_ORDERING);
-				__free_page(data);
+				page_pool_recycle_direct(rxr->page_pool, data);
 			} else {
 				dma_unmap_single_attrs(&pdev->dev, mapping,
 						       bp->rx_buf_use_size,
@@ -2497,6 +2500,8 @@ static void bnxt_free_rx_rings(struct bnxt *bp)
 		if (xdp_rxq_info_is_reg(&rxr->xdp_rxq))
 			xdp_rxq_info_unreg(&rxr->xdp_rxq);
 
+		rxr->page_pool = NULL;
+
 		kfree(rxr->rx_tpa);
 		rxr->rx_tpa = NULL;
 
@@ -2511,6 +2516,26 @@ static void bnxt_free_rx_rings(struct bnxt *bp)
 	}
 }
 
+static int bnxt_alloc_rx_page_pool(struct bnxt *bp,
+				   struct bnxt_rx_ring_info *rxr)
+{
+	struct page_pool_params pp = { 0 };
+
+	pp.pool_size = bp->rx_ring_size;
+	pp.nid = dev_to_node(&bp->pdev->dev);
+	pp.dev = &bp->pdev->dev;
+	pp.dma_dir = DMA_BIDIRECTIONAL;
+
+	rxr->page_pool = page_pool_create(&pp);
+	if (IS_ERR(rxr->page_pool)) {
+		int err = PTR_ERR(rxr->page_pool);
+
+		rxr->page_pool = NULL;
+		return err;
+	}
+	return 0;
+}
+
 static int bnxt_alloc_rx_rings(struct bnxt *bp)
 {
 	int i, rc, agg_rings = 0, tpa_rings = 0;
@@ -2530,12 +2555,17 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
 
 		ring = &rxr->rx_ring_struct;
 
+		rc = bnxt_alloc_rx_page_pool(bp, rxr);
+		if (rc)
+			return rc;
+
 		rc = xdp_rxq_info_reg(&rxr->xdp_rxq, bp->dev, i);
 		if (rc < 0)
 			return rc;
 
 		rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
-						MEM_TYPE_PAGE_SHARED, NULL);
+						MEM_TYPE_PAGE_POOL,
+						rxr->page_pool);
 		if (rc) {
 			xdp_rxq_info_unreg(&rxr->xdp_rxq);
 			return rc;
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.h b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
index 8ac51fa..16694b7 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.h
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.h
@@ -26,6 +26,8 @@
 #include <net/xdp.h>
 #include <linux/dim.h>
 
+struct page_pool;
+
 struct tx_bd {
 	__le32 tx_bd_len_flags_type;
 	#define TX_BD_TYPE					(0x3f << 0)
@@ -799,6 +801,7 @@ struct bnxt_rx_ring_info {
 	struct bnxt_ring_struct	rx_ring_struct;
 	struct bnxt_ring_struct	rx_agg_ring_struct;
 	struct xdp_rxq_info	xdp_rxq;
+	struct page_pool	*page_pool;
 };
 
 struct bnxt_cp_ring_info {
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
index 12489d2..c6f6f20 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c
@@ -15,6 +15,7 @@
 #include <linux/bpf.h>
 #include <linux/bpf_trace.h>
 #include <linux/filter.h>
+#include <net/page_pool.h>
 #include "bnxt_hsi.h"
 #include "bnxt.h"
 #include "bnxt_xdp.h"
@@ -191,7 +192,7 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons,
 
 		if (xdp_do_redirect(bp->dev, &xdp, xdp_prog)) {
 			trace_xdp_exception(bp->dev, xdp_prog, act);
-			__free_page(page);
+			page_pool_recycle_direct(rxr->page_pool, page);
 			return true;
 		}
 
-- 
2.5.1

Re: [PATCH net-next 0/4] bnxt_en: Add XDP_REDIRECT support.

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Sat,  6 Jul 2019 03:36:14 -0400

> This patch series adds XDP_REDIRECT support by Andy Gospodarek.

I'll give Jesper et al. a chance to review this.

Re: [PATCH net-next 3/4] bnxt_en: optimized XDP_REDIRECT support

[Ilias Apalodimas]

Thanks Andy, Michael

> +	if (event & BNXT_REDIRECT_EVENT)
> +		xdp_do_flush_map();
> +
>  	if (event & BNXT_TX_EVENT) {
>  		struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
>  		u16 prod = txr->tx_prod;
> @@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
>  
>  		for (j = 0; j < max_idx;) {
>  			struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> -			struct sk_buff *skb = tx_buf->skb;
> +			struct sk_buff *skb;
>  			int k, last;
>  
> +			if (i < bp->tx_nr_rings_xdp &&
> +			    tx_buf->action == XDP_REDIRECT) {
> +				dma_unmap_single(&pdev->dev,
> +					dma_unmap_addr(tx_buf, mapping),
> +					dma_unmap_len(tx_buf, len),
> +					PCI_DMA_TODEVICE);
> +				xdp_return_frame(tx_buf->xdpf);
> +				tx_buf->action = 0;
> +				tx_buf->xdpf = NULL;
> +				j++;
> +				continue;
> +			}
> +

Can't see the whole file here and maybe i am missing something, but since you
optimize for that and start using page_pool, XDP_TX will be a re-synced (and
not remapped)  buffer that can be returned to the pool and resynced for 
device usage. 
Is that happening later on the tx clean function?

> +			skb = tx_buf->skb;
>  			if (!skb) {
>  				j++;
>  				continue;
> @@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
>  		if (rc < 0)
>  			return rc;
>  
> +		rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> +						MEM_TYPE_PAGE_SHARED, NULL);
> +		if (rc) {
> +			xdp_rxq_info_unreg(&rxr->xdp_rxq);

I think you can use page_pool_free directly here (and pge_pool_destroy once
Ivan's patchset gets nerged), that's what mlx5 does iirc. Can we keep that
common please?

If Ivan's patch get merged please note you'll have to explicitly
page_pool_destroy, after calling xdp_rxq_info_unreg() in the general unregister
case (not the error habdling here). Sorry for the confusion this might bring!

> +			return rc;
> +		}
> +
>  		rc = bnxt_alloc_ring(bp, &ring->ring_mem);
>  		if (rc)
>  			return rc;
> @@ -10233,6 +10257,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
[...]

Thanks!
/Ilias

Re: [PATCH net-next 0/4] bnxt_en: Add XDP_REDIRECT support.

[Toke Høiland-Jørgensen]

David Miller <davem@davemloft.net> writes:

> From: Michael Chan <michael.chan@broadcom.com>
> Date: Sat,  6 Jul 2019 03:36:14 -0400
>
>> This patch series adds XDP_REDIRECT support by Andy Gospodarek.
>
> I'll give Jesper et al. a chance to review this.

Couldn't find any issues other than what Ilias already pointed out. So,
assuming these get resolved, you can add my

Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH net-next 3/4] bnxt_en: optimized XDP_REDIRECT support

[Andy Gospodarek]

On Mon, Jul 08, 2019 at 11:28:03AM +0300, Ilias Apalodimas wrote:
> Thanks Andy, Michael
> 
> > +	if (event & BNXT_REDIRECT_EVENT)
> > +		xdp_do_flush_map();
> > +
> >  	if (event & BNXT_TX_EVENT) {
> >  		struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
> >  		u16 prod = txr->tx_prod;
> > @@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
> >  
> >  		for (j = 0; j < max_idx;) {
> >  			struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> > -			struct sk_buff *skb = tx_buf->skb;
> > +			struct sk_buff *skb;
> >  			int k, last;
> >  
> > +			if (i < bp->tx_nr_rings_xdp &&
> > +			    tx_buf->action == XDP_REDIRECT) {
> > +				dma_unmap_single(&pdev->dev,
> > +					dma_unmap_addr(tx_buf, mapping),
> > +					dma_unmap_len(tx_buf, len),
> > +					PCI_DMA_TODEVICE);
> > +				xdp_return_frame(tx_buf->xdpf);
> > +				tx_buf->action = 0;
> > +				tx_buf->xdpf = NULL;
> > +				j++;
> > +				continue;
> > +			}
> > +
> 
> Can't see the whole file here and maybe i am missing something, but since you
> optimize for that and start using page_pool, XDP_TX will be a re-synced (and
> not remapped)  buffer that can be returned to the pool and resynced for 
> device usage. 
> Is that happening later on the tx clean function?

Take a look at the way we treat the buffers in bnxt_rx_xdp() when we
receive them and then in bnxt_tx_int_xdp() when the transmits have
completed (for XDP_TX and XDP_REDIRECT).  I think we are doing what is
proper with respect to mapping vs sync for both cases, but I would be
fine to be corrected.

> 
> > +			skb = tx_buf->skb;
> >  			if (!skb) {
> >  				j++;
> >  				continue;
> > @@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
> >  		if (rc < 0)
> >  			return rc;
> >  
> > +		rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> > +						MEM_TYPE_PAGE_SHARED, NULL);
> > +		if (rc) {
> > +			xdp_rxq_info_unreg(&rxr->xdp_rxq);
> 
> I think you can use page_pool_free directly here (and pge_pool_destroy once
> Ivan's patchset gets nerged), that's what mlx5 does iirc. Can we keep that
> common please?

That's an easy change, I can do that.

> 
> If Ivan's patch get merged please note you'll have to explicitly
> page_pool_destroy, after calling xdp_rxq_info_unreg() in the general unregister
> case (not the error habdling here). Sorry for the confusion this might bring!

Funny enough the driver was basically doing that until page_pool_destroy
was removed (these patches are not new).  I saw last week there was
discussion to add it back, but I did not want to wait to get this on the
list before that was resolved.

This path works as expected with the code in the tree today so it seemed
like the correct approach to post something that is working, right?  :-)

> 
> > +			return rc;
> > +		}
> > +
> >  		rc = bnxt_alloc_ring(bp, &ring->ring_mem);
> >  		if (rc)
> >  			return rc;
> > @@ -10233,6 +10257,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
> [...]
> 
> Thanks!
> /Ilias

Re: [PATCH net-next 3/4] bnxt_en: optimized XDP_REDIRECT support

[Ilias Apalodimas]

Hi Andy, 

> On Mon, Jul 08, 2019 at 11:28:03AM +0300, Ilias Apalodimas wrote:
> > Thanks Andy, Michael
> > 
> > > +	if (event & BNXT_REDIRECT_EVENT)
> > > +		xdp_do_flush_map();
> > > +
> > >  	if (event & BNXT_TX_EVENT) {
> > >  		struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
> > >  		u16 prod = txr->tx_prod;
> > > @@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
> > >  
> > >  		for (j = 0; j < max_idx;) {
> > >  			struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> > > -			struct sk_buff *skb = tx_buf->skb;
> > > +			struct sk_buff *skb;
> > >  			int k, last;
> > >  
> > > +			if (i < bp->tx_nr_rings_xdp &&
> > > +			    tx_buf->action == XDP_REDIRECT) {
> > > +				dma_unmap_single(&pdev->dev,
> > > +					dma_unmap_addr(tx_buf, mapping),
> > > +					dma_unmap_len(tx_buf, len),
> > > +					PCI_DMA_TODEVICE);
> > > +				xdp_return_frame(tx_buf->xdpf);
> > > +				tx_buf->action = 0;
> > > +				tx_buf->xdpf = NULL;
> > > +				j++;
> > > +				continue;
> > > +			}
> > > +
> > 
> > Can't see the whole file here and maybe i am missing something, but since you
> > optimize for that and start using page_pool, XDP_TX will be a re-synced (and
> > not remapped)  buffer that can be returned to the pool and resynced for 
> > device usage. 
> > Is that happening later on the tx clean function?
> 
> Take a look at the way we treat the buffers in bnxt_rx_xdp() when we
> receive them and then in bnxt_tx_int_xdp() when the transmits have
> completed (for XDP_TX and XDP_REDIRECT).  I think we are doing what is
> proper with respect to mapping vs sync for both cases, but I would be
> fine to be corrected.
> 

Yea seems to be doing the right thing, 
XDP_TX syncs correctly and reuses with bnxt_reuse_rx_data() right?

This might be a bit confusing for someone reading the driver on the first time,
probably because you'll end up with 2 ways of recycling buffers. 

Once a buffers get freed on the XDP path it's either fed back to the pool, so
the next requested buffer get served from the pools cache (ndo_xdp_xmit case in
the patch). If the buffer is used for XDP_TX is's synced correctly but recycled
via bnxt_reuse_rx_data() right? Since you are moving to page pool please
consider having a common approach towards the recycling path. I understand that
means tracking buffers types and make sure you do the right thing on 'tx clean'.
I've done something similar on the netsec driver and i do think this might be a
good thing to add on page_pool API

Again this isn't a blocker at least for me but you already have the buffer type
(via tx_buf->action)

> > 
> > > +			skb = tx_buf->skb;
> > >  			if (!skb) {
> > >  				j++;
> > >  				continue;
> > > @@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
> > >  		if (rc < 0)
> > >  			return rc;
> > >  
> > > +		rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> > > +						MEM_TYPE_PAGE_SHARED, NULL);
> > > +		if (rc) {
> > > +			xdp_rxq_info_unreg(&rxr->xdp_rxq);
> > 
> > I think you can use page_pool_free directly here (and pge_pool_destroy once
> > Ivan's patchset gets nerged), that's what mlx5 does iirc. Can we keep that
> > common please?
> 
> That's an easy change, I can do that.
> 
> > 
> > If Ivan's patch get merged please note you'll have to explicitly
> > page_pool_destroy, after calling xdp_rxq_info_unreg() in the general unregister
> > case (not the error habdling here). Sorry for the confusion this might bring!
> 
> Funny enough the driver was basically doing that until page_pool_destroy
> was removed (these patches are not new).  I saw last week there was
> discussion to add it back, but I did not want to wait to get this on the
> list before that was resolved.

Fair enough

> 
> This path works as expected with the code in the tree today so it seemed
> like the correct approach to post something that is working, right?  :-)

Yes.

It will continue to work even if you dont change the call in the future. 
This is more a 'let's not spread the code' attempt, but removing and re-adding
page_pool_destroy() was/is our mess. We might as well live with the
consequences!

> 
> > 
> > > +			return rc;
> > > +		}
> > > +
> > >  		rc = bnxt_alloc_ring(bp, &ring->ring_mem);
> > >  		if (rc)
> > >  			return rc;
> > > @@ -10233,6 +10257,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
> > [...]
> > 

Thanks!
/Ilias

Re: [PATCH net-next 3/4] bnxt_en: optimized XDP_REDIRECT support

[Andy Gospodarek]

On Mon, Jul 08, 2019 at 05:51:37PM +0300, Ilias Apalodimas wrote:
> Hi Andy, 
> 
> > On Mon, Jul 08, 2019 at 11:28:03AM +0300, Ilias Apalodimas wrote:
> > > Thanks Andy, Michael
> > > 
> > > > +	if (event & BNXT_REDIRECT_EVENT)
> > > > +		xdp_do_flush_map();
> > > > +
> > > >  	if (event & BNXT_TX_EVENT) {
> > > >  		struct bnxt_tx_ring_info *txr = bnapi->tx_ring;
> > > >  		u16 prod = txr->tx_prod;
> > > > @@ -2254,9 +2257,23 @@ static void bnxt_free_tx_skbs(struct bnxt *bp)
> > > >  
> > > >  		for (j = 0; j < max_idx;) {
> > > >  			struct bnxt_sw_tx_bd *tx_buf = &txr->tx_buf_ring[j];
> > > > -			struct sk_buff *skb = tx_buf->skb;
> > > > +			struct sk_buff *skb;
> > > >  			int k, last;
> > > >  
> > > > +			if (i < bp->tx_nr_rings_xdp &&
> > > > +			    tx_buf->action == XDP_REDIRECT) {
> > > > +				dma_unmap_single(&pdev->dev,
> > > > +					dma_unmap_addr(tx_buf, mapping),
> > > > +					dma_unmap_len(tx_buf, len),
> > > > +					PCI_DMA_TODEVICE);
> > > > +				xdp_return_frame(tx_buf->xdpf);
> > > > +				tx_buf->action = 0;
> > > > +				tx_buf->xdpf = NULL;
> > > > +				j++;
> > > > +				continue;
> > > > +			}
> > > > +
> > > 
> > > Can't see the whole file here and maybe i am missing something, but since you
> > > optimize for that and start using page_pool, XDP_TX will be a re-synced (and
> > > not remapped)  buffer that can be returned to the pool and resynced for 
> > > device usage. 
> > > Is that happening later on the tx clean function?
> > 
> > Take a look at the way we treat the buffers in bnxt_rx_xdp() when we
> > receive them and then in bnxt_tx_int_xdp() when the transmits have
> > completed (for XDP_TX and XDP_REDIRECT).  I think we are doing what is
> > proper with respect to mapping vs sync for both cases, but I would be
> > fine to be corrected.
> > 
> 
> Yea seems to be doing the right thing, 
> XDP_TX syncs correctly and reuses with bnxt_reuse_rx_data() right?
> 
> This might be a bit confusing for someone reading the driver on the first time,
> probably because you'll end up with 2 ways of recycling buffers. 
> 
> Once a buffers get freed on the XDP path it's either fed back to the pool, so
> the next requested buffer get served from the pools cache (ndo_xdp_xmit case in
> the patch). If the buffer is used for XDP_TX is's synced correctly but recycled
> via bnxt_reuse_rx_data() right? Since you are moving to page pool please
> consider having a common approach towards the recycling path. I understand that
> means tracking buffers types and make sure you do the right thing on 'tx clean'.
> I've done something similar on the netsec driver and i do think this might be a
> good thing to add on page_pool API
> 
> Again this isn't a blocker at least for me but you already have the buffer type
> (via tx_buf->action)

Thanks for the confirmation.  I agree things are not totally clear as I
had to learn how all of it worked to do this.  We can work on that.

> 
> > > 
> > > > +			skb = tx_buf->skb;
> > > >  			if (!skb) {
> > > >  				j++;
> > > >  				continue;
> > > > @@ -2517,6 +2534,13 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
> > > >  		if (rc < 0)
> > > >  			return rc;
> > > >  
> > > > +		rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> > > > +						MEM_TYPE_PAGE_SHARED, NULL);
> > > > +		if (rc) {
> > > > +			xdp_rxq_info_unreg(&rxr->xdp_rxq);
> > > 
> > > I think you can use page_pool_free directly here (and pge_pool_destroy once
> > > Ivan's patchset gets nerged), that's what mlx5 does iirc. Can we keep that
> > > common please?
> > 
> > That's an easy change, I can do that.
> > 

I'll reply to myself here and note that you are correct that we need to
fixup the error case, but it actually belongs in patch 4 in the series
since that is the patch that adds page_pool support.  I'll reply to that
one in just a min once I've tested my patch.

> > > 
> > > If Ivan's patch get merged please note you'll have to explicitly
> > > page_pool_destroy, after calling xdp_rxq_info_unreg() in the general unregister
> > > case (not the error habdling here). Sorry for the confusion this might bring!
> > 
> > Funny enough the driver was basically doing that until page_pool_destroy
> > was removed (these patches are not new).  I saw last week there was
> > discussion to add it back, but I did not want to wait to get this on the
> > list before that was resolved.
> 
> Fair enough
> 
> > 
> > This path works as expected with the code in the tree today so it seemed
> > like the correct approach to post something that is working, right?  :-)
> 
> Yes.
> 
> It will continue to work even if you dont change the call in the future. 
> This is more a 'let's not spread the code' attempt, but removing and re-adding
> page_pool_destroy() was/is our mess. We might as well live with the
> consequences!

So as someone who ends up doing some of this work after the trail has
already been blazed upstream on other drivers, etc, I definitely
understand the desire to keep things more common.  I think the page_pool
bits are a nice step in that direction, so I enjoy any attempts to help
make repeated tasks easier for everyone.

> 
> > 
> > > 
> > > > +			return rc;
> > > > +		}
> > > > +
> > > >  		rc = bnxt_alloc_ring(bp, &ring->ring_mem);
> > > >  		if (rc)
> > > >  			return rc;
> > > > @@ -10233,6 +10257,7 @@ static const struct net_device_ops bnxt_netdev_ops = {
> > > [...]
> > > 
> 
> Thanks!
> /Ilias

Re: [PATCH net-next 4/4] bnxt_en: add page_pool support

[Andy Gospodarek]

On Sat, Jul 06, 2019 at 03:36:18AM -0400, Michael Chan wrote:
> From: Andy Gospodarek <gospo@broadcom.com>
> 
> This removes contention over page allocation for XDP_REDIRECT actions by
> adding page_pool support per queue for the driver.  The performance for
> XDP_REDIRECT actions scales linearly with the number of cores performing
> redirect actions when using the page pools instead of the standard page
> allocator.
> 
> Signed-off-by: Andy Gospodarek <gospo@broadcom.com>
> Signed-off-by: Michael Chan <michael.chan@broadcom.com>
> ---
>  drivers/net/ethernet/broadcom/Kconfig         |  1 +
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c     | 40 +++++++++++++++++++++++----
>  drivers/net/ethernet/broadcom/bnxt/bnxt.h     |  3 ++
>  drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c |  3 +-
>  4 files changed, 41 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index d8f0846..b6777e5 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
[...]
> @@ -2530,12 +2555,17 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
>  
>  		ring = &rxr->rx_ring_struct;
>  
> +		rc = bnxt_alloc_rx_page_pool(bp, rxr);
> +		if (rc)
> +			return rc;
> +
>  		rc = xdp_rxq_info_reg(&rxr->xdp_rxq, bp->dev, i);
>  		if (rc < 0)
>  			return rc;
>  
>  		rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
> -						MEM_TYPE_PAGE_SHARED, NULL);
> +						MEM_TYPE_PAGE_POOL,
> +						rxr->page_pool);
>  		if (rc) {
>  			xdp_rxq_info_unreg(&rxr->xdp_rxq);
>  			return rc;

I think we want to amend and the chunk above to be:

@@ -2530,14 +2557,24 @@ static int bnxt_alloc_rx_rings(struct bnxt *bp)
 
                ring = &rxr->rx_ring_struct;
 
+               rc = bnxt_alloc_rx_page_pool(bp, rxr);
+               if (rc)
+                       return rc;
+
                rc = xdp_rxq_info_reg(&rxr->xdp_rxq, bp->dev, i);
-               if (rc < 0)
+               if (rc < 0) {
+                       page_pool_free(rxr->page_pool);
+                       rxr->page_pool = NULL;
                        return rc;
+               }
 
                rc = xdp_rxq_info_reg_mem_model(&rxr->xdp_rxq,
-                                               MEM_TYPE_PAGE_SHARED, NULL);
+                                               MEM_TYPE_PAGE_POOL,
+                                               rxr->page_pool);
                if (rc) {
                        xdp_rxq_info_unreg(&rxr->xdp_rxq);
+                       page_pool_free(rxr->page_pool);
+                       rxr->page_pool = NULL;
                        return rc;
                }
 

That should take care of the freeing of the page_pool that is allocated
but there is a failure during xdp_rxq_info_reg() or
xdp_rxq_info_reg_mem_model().

I agree that we do not need to call page_pool_free in the normal
shutdown case.

Re: [PATCH net-next 0/4] bnxt_en: Add XDP_REDIRECT support.

[David Miller]

From: Michael Chan <michael.chan@broadcom.com>
Date: Sat,  6 Jul 2019 03:36:14 -0400

> This patch series adds XDP_REDIRECT support by Andy Gospodarek.
> 
> Andy Gospodarek (3):
>   bnxt_en: rename some xdp functions
>   bnxt_en: optimized XDP_REDIRECT support
>   bnxt_en: add page_pool support
> 
> Michael Chan (1):
>   bnxt_en: Refactor __bnxt_xmit_xdp().

Andy et al. please respin this ASAP with the feedback you've received
and I will apply it.

Thank you.