add XDP support to mvneta driver

drivers/net/ethernet/marvell/mvneta.c +11 -0

+static int mvneta_create_page_pool(struct mvneta_port *pp,
+				   struct mvneta_rx_queue *rxq, int size)
+{
+	struct page_pool_params pp_params = {
+		.order = 0,
+		.flags = PP_FLAG_DMA_MAP,
+		.pool_size = size,
+		.nid = cpu_to_node(0),
+		.dev = pp->dev->dev.parent,
+		.dma_dir = DMA_FROM_DEVICE,
+	};

drivers/net/ethernet/marvell/mvneta.c +16 -0

+	if (err < 0)
+		goto err_free_pp;
+
+	err = xdp_rxq_info_reg_mem_model(&rxq->xdp_rxq, MEM_TYPE_PAGE_POOL,
+					 rxq->page_pool);
+	if (err)
+		goto err_unregister_rxq;
+
+	return 0;
+
+err_unregister_rxq:
+	xdp_rxq_info_unreg(&rxq->xdp_rxq);
+err_free_pp:
+	page_pool_destroy(rxq->page_pool);
+	return err;
+}

Quoted 16 lines

drivers/net/ethernet/marvell/mvneta.c +16 -0

+static int mvneta_create_page_pool(struct mvneta_port *pp,
+				   struct mvneta_rx_queue *rxq, int size)
+{
+	struct page_pool_params pp_params = {
+		.order = 0,
+		.flags = PP_FLAG_DMA_MAP,
+		.pool_size = size,
+		.nid = cpu_to_node(0),
+		.dev = pp->dev->dev.parent,
+		.dma_dir = DMA_FROM_DEVICE,
+	};
+	int err;
+
+	rxq->page_pool = page_pool_create(&pp_params);
+	if (IS_ERR(rxq->page_pool)) {
+		err = PTR_ERR(rxq->page_pool);

drivers/net/ethernet/marvell/mvneta.c static int mvneta_poll(struct napi_struct *napi, int budget) · line 2849

Quoted 5 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+		rxq->page_pool = NULL;
+		return err;
+	}
+
+	err = xdp_rxq_info_reg(&rxq->xdp_rxq, pp->dev, 0);

The queue_index is always passed as 0, is there only a single queue? XDP programs can read this field.

Quoted 17 lines

drivers/net/ethernet/marvell/mvneta.c +16 -0

+	if (err < 0)
+		goto err_free_pp;
+
+	err = xdp_rxq_info_reg_mem_model(&rxq->xdp_rxq, MEM_TYPE_PAGE_POOL,
+					 rxq->page_pool);
+	if (err)
+		goto err_unregister_rxq;
+
+	return 0;
+
+err_unregister_rxq:
+	xdp_rxq_info_unreg(&rxq->xdp_rxq);
+err_free_pp:
+	page_pool_destroy(rxq->page_pool);
+	return err;
+}

mvneta driver can run on not cache coherent devices so it is necessary to sync DMA buffers before sending them to the device in order to avoid memory corruptions. Running perf analysis we can see a performance cost associated with this DMA-sync (anyway it is already there in the original driver code). In follow up patches we will add more logic to reduce DMA-sync as much as possible.

Signed-off-by Jesper Dangaard Brouer <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

Quoted 7 lines · Lorenzo Bianconi

mvneta driver can run on not cache coherent devices so it is necessary to sync DMA buffers before sending them to the device in order to avoid memory corruptions. Running perf analysis we can see a performance cost associated with this DMA-sync (anyway it is already there in the original driver code). In follow up patches we will add more logic to reduce DMA-sync as much as possible.

Quoted 2 lines

Signed-off-by Jesper Dangaard Brouer <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

Should this not be squashed into patch 2? Isn't there a transient bug otherwise?

Add basic XDP support to mvneta driver for devices that rely on software buffer management. Currently supported verdicts are:
- XDP_DROP
- XDP_PASS
- XDP_REDIRECT
- XDP_ABORTED

- iptables drop:
$iptables -t raw -I PREROUTING -p udp --dport 9 -j DROP $nstat -n && sleep 1 && nstat
IpInReceives 151169 0.0
IpExtInOctets 6953544 0.0
IpExtInNoECTPkts 151165 0.0

- XDP_DROP via xdp1
$./samples/bpf/xdp1 3
proto 0: 421419 pkt/s
proto 0: 421444 pkt/s
proto 0: 421393 pkt/s
proto 0: 421440 pkt/s
proto 0: 421184 pkt/s

Tested-by Matteo Croce <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 147 ++++++++++++++++++++++++--
 1 file changed, 138 insertions(+), 9 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +4 -1

index 1722dffe265d..b47a44cf9610 100644
@@ -38,6 +38,7 @@
 #include <net/ipv6.h>
 #include <net/tso.h>
 #include <net/page_pool.h>
+#include <linux/bpf_trace.h>
 
 /* Registers */
 #define MVNETA_RXQ_CONFIG_REG(q)                (0x1400 + ((q) << 2))

@@ -323,8 +324,10 @@
 	      ETH_HLEN + ETH_FCS_LEN,			     \
 	      cache_line_size())
 
+#define MVNETA_SKB_HEADROOM	(max(XDP_PACKET_HEADROOM, NET_SKB_PAD) + \
+				 NET_IP_ALIGN)
 #define MVNETA_SKB_PAD	(SKB_DATA_ALIGN(sizeof(struct skb_shared_info) + \
-			 NET_SKB_PAD))
+			 MVNETA_SKB_HEADROOM))
 #define MVNETA_SKB_SIZE(len)	(SKB_DATA_ALIGN(len) + MVNETA_SKB_PAD)
 #define MVNETA_MAX_RX_BUF_SIZE	(PAGE_SIZE - MVNETA_SKB_PAD)
 

drivers/net/ethernet/marvell/mvneta.c +12 -0

+#define MVNETA_XDP_TX		BIT(2)
+#define MVNETA_XDP_REDIR	BIT(3)
+
 static const struct mvneta_statistic mvneta_statistics[] = {
 	{ 0x3000, T_REG_64, "good_octets_received", },
 	{ 0x3010, T_REG_32, "good_frames_received", },

@@ -431,6 +439,8 @@ struct mvneta_port {
 	u32 cause_rx_tx;
 	struct napi_struct napi;
 
+	struct bpf_prog *xdp_prog;
+
 	/* Core clock */
 	struct clk *clk;
 	/* AXI clock */

@@ -1950,11 +1960,51 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 	return i;
 }
 
+static int
+mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
+	       struct bpf_prog *prog, struct xdp_buff *xdp)
+{
+	u32 ret, act = bpf_prog_run_xdp(prog, xdp);
+
+	switch (act) {

drivers/net/ethernet/marvell/mvneta.c +5 -0

+		} else {
+			ret = MVNETA_XDP_REDIR;
+		}
+		break;
+	}

drivers/net/ethernet/marvell/mvneta.c +11 -2

+		break;
+	}
+
+	return ret;
+}
+
 static int
 mvneta_swbm_rx_frame(struct mvneta_port *pp,
 		     struct mvneta_rx_desc *rx_desc,
 		     struct mvneta_rx_queue *rxq,
-		     struct page *page)
+		     struct xdp_buff *xdp,
+		     struct bpf_prog *xdp_prog,
+		     struct page *page, u32 *xdp_ret)
 {
 	unsigned char *data = page_address(page);
 	int data_len = -MVNETA_MH_SIZE, len;

@@ -1974,7 +2024,26 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp,
 				rx_desc->buf_phys_addr,
 				len, dma_dir);
 
-	rxq->skb = build_skb(data, PAGE_SIZE);
+	xdp->data_hard_start = data;
+	xdp->data = data + MVNETA_SKB_HEADROOM + MVNETA_MH_SIZE;

drivers/net/ethernet/marvell/mvneta.c +12 -3

+					    false);
+			rx_desc->buf_phys_addr = 0;
+			*xdp_ret |= ret;
+			return ret;
+		}
+	}
+
+	rxq->skb = build_skb(xdp->data_hard_start, PAGE_SIZE);
 	if (unlikely(!rxq->skb)) {
 		netdev_err(dev,
 			   "Can't allocate skb on queue %d\n",

@@ -1985,8 +2054,9 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp,
 	}
 	page_pool_release_page(rxq->page_pool, page);
 
-	skb_reserve(rxq->skb, MVNETA_MH_SIZE + NET_SKB_PAD);
-	skb_put(rxq->skb, data_len);
+	skb_reserve(rxq->skb,
+		    xdp->data - xdp->data_hard_start);
+	skb_put(rxq->skb, xdp->data_end - xdp->data);
 	mvneta_rx_csum(pp, rx_desc->status, rxq->skb);
 
 	rxq->left_size = rx_desc->data_size - len;

@@ -2020,7 +2090,7 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
 		/* refill descriptor with new buffer later */
 		skb_add_rx_frag(rxq->skb,
 				skb_shinfo(rxq->skb)->nr_frags,
-				page, NET_SKB_PAD, data_len,
+				page, MVNETA_SKB_HEADROOM, data_len,
 				PAGE_SIZE);
 	}
 	page_pool_release_page(rxq->page_pool, page);

@@ -2035,10 +2105,17 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
 {
 	int rcvd_pkts = 0, rcvd_bytes = 0;
 	int rx_pending, refill, done = 0;

drivers/net/ethernet/marvell/mvneta.c +6 -1

 	/* Fairness NAPI loop */
 	while (done < budget && done < rx_pending) {
 		struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);

@@ -2066,7 +2143,8 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
 				continue;
 			}
 
-			err = mvneta_swbm_rx_frame(pp, rx_desc, rxq, page);
+			err = mvneta_swbm_rx_frame(pp, rx_desc, rxq, &xdp_buf,
+						   xdp_prog, page, &xdp_ret);
 			if (err)
 				continue;
 		} else {

@@ -2101,6 +2179,10 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
 		/* clean uncomplete skb pointer in queue */
 		rxq->skb = NULL;
 	}
+	rcu_read_unlock();
+
+	if (xdp_ret & MVNETA_XDP_REDIR)
+		xdp_do_flush_map();
 
 	mvneta_update_stats(pp, rcvd_pkts, rcvd_bytes, false);
 

Quoted 56 lines · On Mon, 14 Oct 2019 12:49:52 +0200 Lorenzo Bianconi <[email protected]> wrote:

Add basic XDP support to mvneta driver for devices that rely on software buffer management. Currently supported verdicts are:
- XDP_DROP
- XDP_PASS
- XDP_REDIRECT
- XDP_ABORTED

- iptables drop:
$iptables -t raw -I PREROUTING -p udp --dport 9 -j DROP $nstat -n && sleep 1 && nstat
IpInReceives 151169 0.0
IpExtInOctets 6953544 0.0
IpExtInNoECTPkts 151165 0.0

- XDP_DROP via xdp1
$./samples/bpf/xdp1 3
proto 0: 421419 pkt/s
proto 0: 421444 pkt/s
proto 0: 421393 pkt/s
proto 0: 421440 pkt/s
proto 0: 421184 pkt/s

Tested-by Matteo Croce <[email protected]>

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 147 ++++++++++++++++++++++++--
 1 file changed, 138 insertions(+), 9 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +4 -1

index 1722dffe265d..b47a44cf9610 100644
@@ -38,6 +38,7 @@
 #include <net/ipv6.h>
 #include <net/tso.h>
 #include <net/page_pool.h>
+#include <linux/bpf_trace.h>
 
 /* Registers */
 #define MVNETA_RXQ_CONFIG_REG(q)                (0x1400 + ((q) << 2))

@@ -323,8 +324,10 @@
 	      ETH_HLEN + ETH_FCS_LEN,			     \
 	      cache_line_size())
 
+#define MVNETA_SKB_HEADROOM	(max(XDP_PACKET_HEADROOM, NET_SKB_PAD) + \
+				 NET_IP_ALIGN)
 #define MVNETA_SKB_PAD	(SKB_DATA_ALIGN(sizeof(struct skb_shared_info) + \
-			 NET_SKB_PAD))
+			 MVNETA_SKB_HEADROOM))
 #define MVNETA_SKB_SIZE(len)	(SKB_DATA_ALIGN(len) + MVNETA_SKB_PAD)
 #define MVNETA_MAX_RX_BUF_SIZE	(PAGE_SIZE - MVNETA_SKB_PAD)
 

@@ -352,6 +355,11 @@ struct mvneta_statistic {
 #define T_REG_64	64
 #define T_SW		1
 

drivers/net/ethernet/marvell/mvneta.c struct mvneta_statistic { · line 359

Quoted 2 lines

drivers/net/ethernet/marvell/mvneta.c +2 -0

+#define MVNETA_XDP_PASS		BIT(0)
+#define MVNETA_XDP_CONSUMED	BIT(1)

I find it confusing that you call it "consumed" (MVNETA_XDP_CONSUMED), because if I follow the code these are all drop-cases that are due to errors.

Can we call it MVNETA_XDP_DROPPED?

I also checked, your XDP_TX patch[8/8], that all the return paths for MVNETA_XDP_CONSUMED also leads to drop of the xdp_buff.

Quoted 26 lines

drivers/net/ethernet/marvell/mvneta.c +12 -0

+#define MVNETA_XDP_TX		BIT(2)
+#define MVNETA_XDP_REDIR	BIT(3)
+
 static const struct mvneta_statistic mvneta_statistics[] = {
 	{ 0x3000, T_REG_64, "good_octets_received", },
 	{ 0x3010, T_REG_32, "good_frames_received", },

@@ -431,6 +439,8 @@ struct mvneta_port {
 	u32 cause_rx_tx;
 	struct napi_struct napi;
 
+	struct bpf_prog *xdp_prog;
+
 	/* Core clock */
 	struct clk *clk;
 	/* AXI clock */

@@ -1950,11 +1960,51 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 	return i;
 }
 
+static int
+mvneta_run_xdp(struct mvneta_port *pp, struct mvneta_rx_queue *rxq,
+	       struct bpf_prog *prog, struct xdp_buff *xdp)
+{
+	u32 ret, act = bpf_prog_run_xdp(prog, xdp);
+
+	switch (act) {

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 1979

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c +10 -0

+	case XDP_PASS:
+		ret = MVNETA_XDP_PASS;
+		break;
+	case XDP_REDIRECT: {
+		int err;
+
+		err = xdp_do_redirect(pp->dev, xdp, prog);
+		if (err) {
+			ret = MVNETA_XDP_CONSUMED;
+			xdp_return_buff(xdp);

Dropped in case of errors. As this is an error case, I don't mind that you use the slower xdp_return_buff().

Quoted 5 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+		} else {
+			ret = MVNETA_XDP_REDIR;
+		}
+		break;
+	}

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 1994

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c +10 -0

+	default:
+		bpf_warn_invalid_xdp_action(act);
+		/* fall through */
+	case XDP_ABORTED:
+		trace_xdp_exception(pp->dev, prog, act);
+		/* fall through */
+	case XDP_DROP:
+		page_pool_recycle_direct(rxq->page_pool,
+					 virt_to_head_page(xdp->data));
+		ret = MVNETA_XDP_CONSUMED;

Also drop case.

Quoted 24 lines

drivers/net/ethernet/marvell/mvneta.c +11 -2

+		break;
+	}
+
+	return ret;
+}
+
 static int
 mvneta_swbm_rx_frame(struct mvneta_port *pp,
 		     struct mvneta_rx_desc *rx_desc,
 		     struct mvneta_rx_queue *rxq,
-		     struct page *page)
+		     struct xdp_buff *xdp,
+		     struct bpf_prog *xdp_prog,
+		     struct page *page, u32 *xdp_ret)
 {
 	unsigned char *data = page_address(page);
 	int data_len = -MVNETA_MH_SIZE, len;

@@ -1974,7 +2024,26 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp,
 				rx_desc->buf_phys_addr,
 				len, dma_dir);
 
-	rxq->skb = build_skb(data, PAGE_SIZE);
+	xdp->data_hard_start = data;
+	xdp->data = data + MVNETA_SKB_HEADROOM + MVNETA_MH_SIZE;

drivers/net/ethernet/marvell/mvneta.c mvneta_swbm_rx_frame(struct mvneta_port *pp, · line 2038

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c +10 -0

+	xdp->data_end = xdp->data + data_len;
+	xdp_set_data_meta_invalid(xdp);
+
+	if (xdp_prog) {
+		u32 ret;
+
+		ret = mvneta_run_xdp(pp, rxq, xdp_prog, xdp);
+		if (ret != MVNETA_XDP_PASS) {
+			mvneta_update_stats(pp, 1,
+					    xdp->data_end - xdp->data,

Good, you took into account that data_len cannot be used, as BPF/XDP program could have changed data pointer, thus affecting the length.

Quoted 36 lines

drivers/net/ethernet/marvell/mvneta.c +12 -3

+					    false);
+			rx_desc->buf_phys_addr = 0;
+			*xdp_ret |= ret;
+			return ret;
+		}
+	}
+
+	rxq->skb = build_skb(xdp->data_hard_start, PAGE_SIZE);
 	if (unlikely(!rxq->skb)) {
 		netdev_err(dev,
 			   "Can't allocate skb on queue %d\n",

@@ -1985,8 +2054,9 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp,
 	}
 	page_pool_release_page(rxq->page_pool, page);
 
-	skb_reserve(rxq->skb, MVNETA_MH_SIZE + NET_SKB_PAD);
-	skb_put(rxq->skb, data_len);
+	skb_reserve(rxq->skb,
+		    xdp->data - xdp->data_hard_start);
+	skb_put(rxq->skb, xdp->data_end - xdp->data);
 	mvneta_rx_csum(pp, rx_desc->status, rxq->skb);
 
 	rxq->left_size = rx_desc->data_size - len;

@@ -2020,7 +2090,7 @@ mvneta_swbm_add_rx_fragment(struct mvneta_port *pp,
 		/* refill descriptor with new buffer later */
 		skb_add_rx_frag(rxq->skb,
 				skb_shinfo(rxq->skb)->nr_frags,
-				page, NET_SKB_PAD, data_len,
+				page, MVNETA_SKB_HEADROOM, data_len,
 				PAGE_SIZE);
 	}
 	page_pool_release_page(rxq->page_pool, page);

@@ -2035,10 +2105,17 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
 {
 	int rcvd_pkts = 0, rcvd_bytes = 0;
 	int rx_pending, refill, done = 0;

drivers/net/ethernet/marvell/mvneta.c static int mvneta_rx_swbm(struct napi_struct *napi, · line 2117

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c +6 -0

+	struct bpf_prog *xdp_prog;
+	struct xdp_buff xdp_buf;
+	u32 xdp_ret = 0;
 
 	/* Get number of received packets */
 	rx_pending = mvneta_rxq_busy_desc_num_get(pp, rxq);
 
+	rcu_read_lock();
+	xdp_prog = READ_ONCE(pp->xdp_prog);
+	xdp_buf.rxq = &rxq->xdp_rxq;

Ok, thanks for following my review comments from last.

Quoted 24 lines

drivers/net/ethernet/marvell/mvneta.c +6 -1

 	/* Fairness NAPI loop */
 	while (done < budget && done < rx_pending) {
 		struct mvneta_rx_desc *rx_desc = mvneta_rxq_next_desc_get(rxq);

@@ -2066,7 +2143,8 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
 				continue;
 			}
 
-			err = mvneta_swbm_rx_frame(pp, rx_desc, rxq, page);
+			err = mvneta_swbm_rx_frame(pp, rx_desc, rxq, &xdp_buf,
+						   xdp_prog, page, &xdp_ret);
 			if (err)
 				continue;
 		} else {

@@ -2101,6 +2179,10 @@ static int mvneta_rx_swbm(struct napi_struct *napi,
 		/* clean uncomplete skb pointer in queue */
 		rxq->skb = NULL;
 	}
+	rcu_read_unlock();
+
+	if (xdp_ret & MVNETA_XDP_REDIR)
+		xdp_do_flush_map();
 
 	mvneta_update_stats(pp, rcvd_pkts, rcvd_bytes, false);
 

[...]

Signature · 4 lines

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

drivers/net/ethernet/marvell/mvneta.c +10 -0

@@ -3983,6 +4071,46 @@ static int mvneta_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return phylink_mii_ioctl(pp->phylink, ifr, cmd);
 }
 
+static int mvneta_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
+			    struct netlink_ext_ack *extack)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	struct bpf_prog *old_prog;
+
+	if (prog && dev->mtu > MVNETA_MAX_RX_BUF_SIZE) {
+		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported on XDP");
+		return -EOPNOTSUPP;
+	}

drivers/net/ethernet/marvell/mvneta.c +5 -0

+	return 0;
+}
+
+static int mvneta_xdp(struct net_device *dev, struct netdev_bpf *xdp)
+{

drivers/net/ethernet/marvell/mvneta.c +3 -0

+		return -EINVAL;
+	}
+}

Quoted 20 lines

drivers/net/ethernet/marvell/mvneta.c +16 -0

@@ -3983,6 +4071,46 @@ static int mvneta_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
 	return phylink_mii_ioctl(pp->phylink, ifr, cmd);
 }
 
+static int mvneta_xdp_setup(struct net_device *dev, struct bpf_prog *prog,
+			    struct netlink_ext_ack *extack)
+{
+	struct mvneta_port *pp = netdev_priv(dev);
+	struct bpf_prog *old_prog;
+
+	if (prog && dev->mtu > MVNETA_MAX_RX_BUF_SIZE) {
+		NL_SET_ERR_MSG_MOD(extack, "Jumbo frames not supported on XDP");
+		return -EOPNOTSUPP;
+	}
+
+	if (netif_running(dev))
+		mvneta_stop(dev);
+
+	old_prog = xchg(&pp->xdp_prog, prog);
+	if (old_prog)

drivers/net/ethernet/marvell/mvneta.c static int mvneta_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) · line 4093

Quoted 4 lines

drivers/net/ethernet/marvell/mvneta.c +4 -0

+		bpf_prog_put(old_prog);
+
+	if (netif_running(dev))
+		mvneta_open(dev);

Ah, the stopping and starting of the interface is sad. If start fails the interface is left in a funky state until someone does a stop/start cycle. Not that you introduced that.

Quoted 6 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+	return 0;
+}
+
+static int mvneta_xdp(struct net_device *dev, struct netdev_bpf *xdp)
+{

drivers/net/ethernet/marvell/mvneta.c static int mvneta_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd) · line 4109

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c +10 -0

+	struct mvneta_port *pp = netdev_priv(dev);
+
+	switch (xdp->command) {
+	case XDP_SETUP_PROG:
+		return mvneta_xdp_setup(dev, xdp->prog, xdp->extack);
+	case XDP_QUERY_PROG:
+		xdp->prog_id = pp->xdp_prog ? pp->xdp_prog->aux->id : 0;
+		return 0;
+	default:
+		NL_SET_ERR_MSG_MOD(xdp->extack, "unknown XDP command");

Please drop this message here, there are commands you legitimately don't care about, just return -EINVAL, no need to risk leaking a meaningless warning to the user space.

Quoted 3 lines

drivers/net/ethernet/marvell/mvneta.c +3 -0

+		return -EINVAL;
+	}
+}

Allow tx buffer array to contain both skb and xdp buffers in order to enable xdp frame recycling adding XDP_TX verdict support

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 66 +++++++++++++++++----------
 1 file changed, 43 insertions(+), 23 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +0 -0

index a79d81c9be7a..477ae6592fa3 100644
@@ -561,6 +561,20 @@ struct mvneta_rx_desc {
 };

drivers/net/ethernet/marvell/mvneta.c +5 -0

+	union {
+		struct xdp_frame *xdpf;
+		struct sk_buff *skb;
+	};
+};

Quoted 16 lines · Lorenzo Bianconi

Allow tx buffer array to contain both skb and xdp buffers in order to enable xdp frame recycling adding XDP_TX verdict support

Signed-off-by Lorenzo Bianconi <[email protected]>

---
 drivers/net/ethernet/marvell/mvneta.c | 66 +++++++++++++++++----------
 1 file changed, 43 insertions(+), 23 deletions(-)

drivers/net/ethernet/marvell/mvneta.c +0 -0

index a79d81c9be7a..477ae6592fa3 100644
@@ -561,6 +561,20 @@ struct mvneta_rx_desc {
 };
 #endif
 

drivers/net/ethernet/marvell/mvneta.c struct mvneta_rx_desc { · line 571

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c +8 -0

+enum mvneta_tx_buf_type {
+	MVNETA_TYPE_SKB,
+	MVNETA_TYPE_XDP_TX,
+	MVNETA_TYPE_XDP_NDO,
+};
+
+struct mvneta_tx_buf {
+	enum mvneta_tx_buf_type type;

I'd be tempted to try to encode type on the low bits of the pointer, otherwise you're increasing the cache pressure here. I'm not 100% sure it's worth the hassle, perhaps could be a future optimization.

drivers/net/ethernet/marvell/mvneta.c struct mvneta_rx_desc { · line 576

Quoted 5 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+	union {
+		struct xdp_frame *xdpf;
+		struct sk_buff *skb;
+	};
+};

Implement XDP_TX verdict and ndo_xdp_xmit net_device_ops function pointer

Signed-off-by Lorenzo Bianconi <[email protected]>

drivers/net/ethernet/marvell/mvneta.c +20 -0

@@ -1972,6 +1975,109 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 	return i;
 }
 
+static int
+mvneta_xdp_submit_frame(struct mvneta_port *pp, struct mvneta_tx_queue *txq,
+			struct xdp_frame *xdpf, bool dma_map)
+{
+	struct mvneta_tx_desc *tx_desc;
+	struct mvneta_tx_buf *buf;
+	dma_addr_t dma_addr;
+
+	if (txq->count >= txq->tx_stop_threshold)
+		return MVNETA_XDP_CONSUMED;
+
+	tx_desc = mvneta_txq_next_desc_get(txq);
+
+	buf = &txq->buf[txq->txq_put_index];
+	if (dma_map) {
+		/* ndo_xdp_xmit */
+		dma_addr = dma_map_single(pp->dev->dev.parent, xdpf->data,
+					  xdpf->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(pp->dev->dev.parent, dma_addr)) {
+			mvneta_txq_desc_put(txq);

drivers/net/ethernet/marvell/mvneta.c +17 -0

+		buf->type = MVNETA_TYPE_XDP_TX;
+	}
+	buf->xdpf = xdpf;
+
+	tx_desc->command = MVNETA_TXD_FLZ_DESC;
+	tx_desc->buf_phys_addr = dma_addr;
+	tx_desc->data_size = xdpf->len;
+
+	mvneta_update_stats(pp, 1, xdpf->len, true);
+	mvneta_txq_inc_put(txq);
+	txq->pending++;
+	txq->count++;
+
+	return MVNETA_XDP_TX;
+}
+
+static int

drivers/net/ethernet/marvell/mvneta.c +12 -0

+
+	txq = &pp->txqs[cpu % txq_number];
+	nq = netdev_get_tx_queue(pp->dev, txq->id);
+
+	__netif_tx_lock(nq, cpu);
+	ret = mvneta_xdp_submit_frame(pp, txq, xdpf, false);
+	if (ret == MVNETA_XDP_TX)
+		mvneta_txq_pend_desc_add(pp, txq, 0);
+	__netif_tx_unlock(nq);
+
+	return ret;
+}

Quoted 35 lines

Implement XDP_TX verdict and ndo_xdp_xmit net_device_ops function pointer

Signed-off-by Lorenzo Bianconi <[email protected]>

drivers/net/ethernet/marvell/mvneta.c +26 -0

@@ -1972,6 +1975,109 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 	return i;
 }
 
+static int
+mvneta_xdp_submit_frame(struct mvneta_port *pp, struct mvneta_tx_queue *txq,
+			struct xdp_frame *xdpf, bool dma_map)
+{
+	struct mvneta_tx_desc *tx_desc;
+	struct mvneta_tx_buf *buf;
+	dma_addr_t dma_addr;
+
+	if (txq->count >= txq->tx_stop_threshold)
+		return MVNETA_XDP_CONSUMED;
+
+	tx_desc = mvneta_txq_next_desc_get(txq);
+
+	buf = &txq->buf[txq->txq_put_index];
+	if (dma_map) {
+		/* ndo_xdp_xmit */
+		dma_addr = dma_map_single(pp->dev->dev.parent, xdpf->data,
+					  xdpf->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(pp->dev->dev.parent, dma_addr)) {
+			mvneta_txq_desc_put(txq);
+			return MVNETA_XDP_CONSUMED;
+		}
+		buf->type = MVNETA_TYPE_XDP_NDO;
+	} else {
+		struct page *page = virt_to_page(xdpf->data);
+

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2007

Quoted 4 lines

drivers/net/ethernet/marvell/mvneta.c +4 -0

+		dma_addr = page_pool_get_dma_addr(page) +
+			   pp->rx_offset_correction + MVNETA_MH_SIZE;
+		dma_sync_single_for_device(pp->dev->dev.parent, dma_addr,
+					   xdpf->len, DMA_BIDIRECTIONAL);

This looks a little suspicious, XDP could have moved the start of frame with adjust_head, right? You should also use xdpf->data to find where the frame starts, no?

Quoted 23 lines

drivers/net/ethernet/marvell/mvneta.c +22 -0

+		buf->type = MVNETA_TYPE_XDP_TX;
+	}
+	buf->xdpf = xdpf;
+
+	tx_desc->command = MVNETA_TXD_FLZ_DESC;
+	tx_desc->buf_phys_addr = dma_addr;
+	tx_desc->data_size = xdpf->len;
+
+	mvneta_update_stats(pp, 1, xdpf->len, true);
+	mvneta_txq_inc_put(txq);
+	txq->pending++;
+	txq->count++;
+
+	return MVNETA_XDP_TX;
+}
+
+static int
+mvneta_xdp_xmit_back(struct mvneta_port *pp, struct xdp_buff *xdp)
+{
+	struct xdp_frame *xdpf = convert_to_xdp_frame(xdp);
+	int cpu = smp_processor_id();
+	struct mvneta_tx_queue *txq;

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2034

Quoted 5 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+	struct netdev_queue *nq;
+	u32 ret;
+
+	if (unlikely(!xdpf))
+		return MVNETA_XDP_CONSUMED;

Personally I'd prefer you haven't called a function which return code has to be error checked in local variable init.

Quoted 13 lines

drivers/net/ethernet/marvell/mvneta.c +12 -0

+
+	txq = &pp->txqs[cpu % txq_number];
+	nq = netdev_get_tx_queue(pp->dev, txq->id);
+
+	__netif_tx_lock(nq, cpu);
+	ret = mvneta_xdp_submit_frame(pp, txq, xdpf, false);
+	if (ret == MVNETA_XDP_TX)
+		mvneta_txq_pend_desc_add(pp, txq, 0);
+	__netif_tx_unlock(nq);
+
+	return ret;
+}

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Implement XDP_TX verdict and ndo_xdp_xmit net_device_ops function pointer

Signed-off-by Lorenzo Bianconi <[email protected]>

drivers/net/ethernet/marvell/mvneta.c +26 -0

@@ -1972,6 +1975,109 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 	return i;
 }
 
+static int
+mvneta_xdp_submit_frame(struct mvneta_port *pp, struct mvneta_tx_queue *txq,
+			struct xdp_frame *xdpf, bool dma_map)
+{
+	struct mvneta_tx_desc *tx_desc;
+	struct mvneta_tx_buf *buf;
+	dma_addr_t dma_addr;
+
+	if (txq->count >= txq->tx_stop_threshold)
+		return MVNETA_XDP_CONSUMED;
+
+	tx_desc = mvneta_txq_next_desc_get(txq);
+
+	buf = &txq->buf[txq->txq_put_index];
+	if (dma_map) {
+		/* ndo_xdp_xmit */
+		dma_addr = dma_map_single(pp->dev->dev.parent, xdpf->data,
+					  xdpf->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(pp->dev->dev.parent, dma_addr)) {
+			mvneta_txq_desc_put(txq);
+			return MVNETA_XDP_CONSUMED;
+		}
+		buf->type = MVNETA_TYPE_XDP_NDO;
+	} else {
+		struct page *page = virt_to_page(xdpf->data);
+

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2007

Quoted 4 lines

drivers/net/ethernet/marvell/mvneta.c +4 -0

+		dma_addr = page_pool_get_dma_addr(page) +
+			   pp->rx_offset_correction + MVNETA_MH_SIZE;
+		dma_sync_single_for_device(pp->dev->dev.parent, dma_addr,
+					   xdpf->len, DMA_BIDIRECTIONAL);

This looks a little suspicious, XDP could have moved the start of frame with adjust_head, right? You should also use xdpf->data to find where the frame starts, no?

uhm, right..we need to update the dma_addr doing something like:

dma_addr = page_pool_get_dma_addr(page) + xdpf->data - xdpf;

and then use xdpf->len for dma-sync

Quoted 23 lines

Quoted 23 lines

drivers/net/ethernet/marvell/mvneta.c +22 -0

+		buf->type = MVNETA_TYPE_XDP_TX;
+	}
+	buf->xdpf = xdpf;
+
+	tx_desc->command = MVNETA_TXD_FLZ_DESC;
+	tx_desc->buf_phys_addr = dma_addr;
+	tx_desc->data_size = xdpf->len;
+
+	mvneta_update_stats(pp, 1, xdpf->len, true);
+	mvneta_txq_inc_put(txq);
+	txq->pending++;
+	txq->count++;
+
+	return MVNETA_XDP_TX;
+}
+
+static int
+mvneta_xdp_xmit_back(struct mvneta_port *pp, struct xdp_buff *xdp)
+{
+	struct xdp_frame *xdpf = convert_to_xdp_frame(xdp);
+	int cpu = smp_processor_id();
+	struct mvneta_tx_queue *txq;

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2034

Quoted 5 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+	struct netdev_queue *nq;
+	u32 ret;
+
+	if (unlikely(!xdpf))
+		return MVNETA_XDP_CONSUMED;

Personally I'd prefer you haven't called a function which return code has to be error checked in local variable init.

do you mean moving cpu = smp_processor_id(); after the if condition?

Regards,
Lorenzo

Quoted 1 line

Quoted 4 lines

Quoted 4 lines

drivers/net/ethernet/marvell/mvneta.c +4 -0

+
+	txq = &pp->txqs[cpu % txq_number];
+	nq = netdev_get_tx_queue(pp->dev, txq->id);
+

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2046

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c +8 -0

+	__netif_tx_lock(nq, cpu);
+	ret = mvneta_xdp_submit_frame(pp, txq, xdpf, false);
+	if (ret == MVNETA_XDP_TX)
+		mvneta_txq_pend_desc_add(pp, txq, 0);
+	__netif_tx_unlock(nq);
+
+	return ret;
+}

Quoted 45 lines · Lorenzo Bianconi

Quoted 44 lines · Lorenzo Bianconi

Quoted 29 lines

Implement XDP_TX verdict and ndo_xdp_xmit net_device_ops function pointer

Signed-off-by Lorenzo Bianconi <[email protected]>

drivers/net/ethernet/marvell/mvneta.c +20 -0

@@ -1972,6 +1975,109 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 	return i;
 }
 
+static int
+mvneta_xdp_submit_frame(struct mvneta_port *pp, struct mvneta_tx_queue *txq,
+			struct xdp_frame *xdpf, bool dma_map)
+{
+	struct mvneta_tx_desc *tx_desc;
+	struct mvneta_tx_buf *buf;
+	dma_addr_t dma_addr;
+
+	if (txq->count >= txq->tx_stop_threshold)
+		return MVNETA_XDP_CONSUMED;
+
+	tx_desc = mvneta_txq_next_desc_get(txq);
+
+	buf = &txq->buf[txq->txq_put_index];
+	if (dma_map) {
+		/* ndo_xdp_xmit */
+		dma_addr = dma_map_single(pp->dev->dev.parent, xdpf->data,
+					  xdpf->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(pp->dev->dev.parent, dma_addr)) {
+			mvneta_txq_desc_put(txq);

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2007

Quoted 10 lines

drivers/net/ethernet/marvell/mvneta.c +10 -0

+			return MVNETA_XDP_CONSUMED;
+		}
+		buf->type = MVNETA_TYPE_XDP_NDO;
+	} else {
+		struct page *page = virt_to_page(xdpf->data);
+
+		dma_addr = page_pool_get_dma_addr(page) +
+			   pp->rx_offset_correction + MVNETA_MH_SIZE;
+		dma_sync_single_for_device(pp->dev->dev.parent, dma_addr,
+					   xdpf->len, DMA_BIDIRECTIONAL);  

This looks a little suspicious, XDP could have moved the start of frame with adjust_head, right? You should also use xdpf->data to find where the frame starts, no?

Quoted 3 lines

uhm, right..we need to update the dma_addr doing something like:

dma_addr = page_pool_get_dma_addr(page) + xdpf->data - xdpf;

Can we do page_pool_get_dma_addr(page) + xdpf->headroom as well right?

Quoted 50 lines · On Wed, 16 Oct 2019 13:55:06 +0300 Ilias Apalodimas…

Quoted 48 lines · Lorenzo Bianconi

Quoted 36 lines · Lorenzo Bianconi

Quoted 35 lines

Implement XDP_TX verdict and ndo_xdp_xmit net_device_ops function pointer

Signed-off-by Lorenzo Bianconi <[email protected]>

drivers/net/ethernet/marvell/mvneta.c +26 -0

@@ -1972,6 +1975,109 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq)
 	return i;
 }
 
+static int
+mvneta_xdp_submit_frame(struct mvneta_port *pp, struct mvneta_tx_queue *txq,
+			struct xdp_frame *xdpf, bool dma_map)
+{
+	struct mvneta_tx_desc *tx_desc;
+	struct mvneta_tx_buf *buf;
+	dma_addr_t dma_addr;
+
+	if (txq->count >= txq->tx_stop_threshold)
+		return MVNETA_XDP_CONSUMED;
+
+	tx_desc = mvneta_txq_next_desc_get(txq);
+
+	buf = &txq->buf[txq->txq_put_index];
+	if (dma_map) {
+		/* ndo_xdp_xmit */
+		dma_addr = dma_map_single(pp->dev->dev.parent, xdpf->data,
+					  xdpf->len, DMA_TO_DEVICE);
+		if (dma_mapping_error(pp->dev->dev.parent, dma_addr)) {
+			mvneta_txq_desc_put(txq);
+			return MVNETA_XDP_CONSUMED;
+		}
+		buf->type = MVNETA_TYPE_XDP_NDO;
+	} else {
+		struct page *page = virt_to_page(xdpf->data);
+

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2007

Quoted 4 lines

drivers/net/ethernet/marvell/mvneta.c +4 -0

+		dma_addr = page_pool_get_dma_addr(page) +
+			   pp->rx_offset_correction + MVNETA_MH_SIZE;
+		dma_sync_single_for_device(pp->dev->dev.parent, dma_addr,
+					   xdpf->len, DMA_BIDIRECTIONAL);  

This looks a little suspicious, XDP could have moved the start of frame with adjust_head, right? You should also use xdpf->data to find where the frame starts, no?

uhm, right..we need to update the dma_addr doing something like:

dma_addr = page_pool_get_dma_addr(page) + xdpf->data - xdpf;

Quoted 1 line

Can we do page_pool_get_dma_addr(page) + xdpf->headroom as well right?

We actually have to do:
page_pool_get_dma_addr(page) + xdpf->headroom + sizeof(struct xdp_frame)

As part of part of headroom was reserved to xdpf. I've considered several times to change xdpf->headroom to include sizeof(struct xdp_frame) as use-cases (e.g. in veth and cpumap) ended needing the "full" headroom.

Signature · 5 lines

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

Quoted 6 lines

Quoted 6 lines

drivers/net/ethernet/marvell/mvneta.c +6 -0

+static int
+mvneta_xdp_xmit_back(struct mvneta_port *pp, struct xdp_buff *xdp)
+{
+	struct xdp_frame *xdpf = convert_to_xdp_frame(xdp);
+	int cpu = smp_processor_id();
+	struct mvneta_tx_queue *txq;

Quoted 8 lines

drivers/net/ethernet/marvell/mvneta.c int mvneta_rx_refill_queue(struct mvneta_port *pp, struct mvneta_rx_queue *rxq) · line 2034

Quoted 5 lines

drivers/net/ethernet/marvell/mvneta.c +5 -0

+	struct netdev_queue *nq;
+	u32 ret;
+
+	if (unlikely(!xdpf))
+		return MVNETA_XDP_CONSUMED;  

Personally I'd prefer you haven't called a function which return code has to be error checked in local variable init.

do you mean moving cpu = smp_processor_id(); after the if condition?