Skip to contentSkip to conversation

xdp: introduce bulking for page_pool tx return path

PATCH Superseded Lorenzo Bianconi 5 participants 23 messages Oct 27, 2020 – Oct 29, 2020

Subsystems (8)
Networking Network drivers BPF Mellanox mlx5 core +4 more subsystems

v1 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

Lorenzo Bianconi

author
#0 lore raw

cover xdp: introduce bulking for page_pool tx return path

xdp_return_frame and page_pool_put_page are usually run inside the driver NAPI tx completion loop so it is possible batch them. Introduce bulking capability in xdp tx return path (XDP_REDIRECT). Convert mvneta, mvpp2 and mlx5 drivers to xdp_return_frame_bulk APIs.

Lorenzo Bianconi (4):
net: xdp: introduce bulking for xdp tx return path
net: page_pool: add bulk support for ptr_ring
net: mvpp2: add xdp tx return bulking support
net: mlx5: add xdp tx return bulking support

 drivers/net/ethernet/marvell/mvneta.c         |  5 +-
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  5 +-
 .../net/ethernet/mellanox/mlx5/core/en/xdp.c  |  5 +-
 include/net/page_pool.h                       | 26 +++++++++++
 include/net/xdp.h                             |  9 ++++
 net/core/page_pool.c                          | 33 +++++++++++++
 net/core/xdp.c                                | 46 +++++++++++++++++++
 7 files changed, 126 insertions(+), 3 deletions(-)
Signature · 1 line
2.26.2
author
#1 lore raw

PATCH 1/4 net: xdp: introduce bulking for xdp tx return path

Introduce bulking capability in xdp tx return path (XDP_REDIRECT). xdp_return_frame is usually run inside the driver NAPI tx completion loop so it is possible batch it.
Current implementation considers only page_pool memory model. Convert mvneta driver to xdp_return_frame_bulk APIs.

Suggested-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/ethernet/marvell/mvneta.c |  5 ++-
 include/net/xdp.h                     |  9 +++++
 net/core/xdp.c                        | 51 +++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 1 deletion(-)
drivers/net/ethernet/marvell/mvneta.c +4 -1
index 54b0bf574c05..43ab8a73900e 100644
@@ -1834,8 +1834,10 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
 				 struct netdev_queue *nq, bool napi)
 {
 	unsigned int bytes_compl = 0, pkts_compl = 0;
+	struct xdp_frame_bulk bq;
 	int i;
 
+	bq.xa = NULL;
 	for (i = 0; i < num; i++) {
 		struct mvneta_tx_buf *buf = &txq->buf[txq->txq_get_index];
 		struct mvneta_tx_desc *tx_desc = txq->descs +

@@ -1857,9 +1859,10 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
 			if (napi && buf->type == MVNETA_TYPE_XDP_TX)
 				xdp_return_frame_rx_napi(buf->xdpf);
 			else
-				xdp_return_frame(buf->xdpf);
+				xdp_return_frame_bulk(buf->xdpf, &bq);
 		}
 	}
+	xdp_flush_frame_bulk(&bq);
 
 	netdev_tx_completed_queue(nq, pkts_compl, bytes_compl);
 }
include/net/xdp.h +9 -0
index 3814fb631d52..9567110845ef 100644
@@ -104,6 +104,12 @@ struct xdp_frame {
 	struct net_device *dev_rx; /* used by cpumap */
 };
 
+#define XDP_BULK_QUEUE_SIZE	16
+struct xdp_frame_bulk {
+	void *q[XDP_BULK_QUEUE_SIZE];
+	int count;
+	void *xa;
+};
 
 static inline struct skb_shared_info *
 xdp_get_shared_info_from_frame(struct xdp_frame *frame)

@@ -194,6 +200,9 @@ struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
 void xdp_return_frame(struct xdp_frame *xdpf);
 void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
 void xdp_return_buff(struct xdp_buff *xdp);
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq);
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq);
 
 /* When sending xdp_frame into the network stack, then there is no
  * return point callback, which is needed to release e.g. DMA-mapping
net/core/xdp.c +51 -0

57 lines not on this page · raw message

Signature · 1 line
2.26.2
maintainer
#2 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

Hi Lorenzo,

Quoted 98 lines · Lorenzo Bianconi

Introduce bulking capability in xdp tx return path (XDP_REDIRECT). xdp_return_frame is usually run inside the driver NAPI tx completion loop so it is possible batch it.
Current implementation considers only page_pool memory model. Convert mvneta driver to xdp_return_frame_bulk APIs.

Suggested-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/ethernet/marvell/mvneta.c |  5 ++-
 include/net/xdp.h                     |  9 +++++
 net/core/xdp.c                        | 51 +++++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 1 deletion(-)
drivers/net/ethernet/marvell/mvneta.c +4 -1
index 54b0bf574c05..43ab8a73900e 100644
@@ -1834,8 +1834,10 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
 				 struct netdev_queue *nq, bool napi)
 {
 	unsigned int bytes_compl = 0, pkts_compl = 0;
+	struct xdp_frame_bulk bq;
 	int i;
 
+	bq.xa = NULL;
 	for (i = 0; i < num; i++) {
 		struct mvneta_tx_buf *buf = &txq->buf[txq->txq_get_index];
 		struct mvneta_tx_desc *tx_desc = txq->descs +

@@ -1857,9 +1859,10 @@ static void mvneta_txq_bufs_free(struct mvneta_port *pp,
 			if (napi && buf->type == MVNETA_TYPE_XDP_TX)
 				xdp_return_frame_rx_napi(buf->xdpf);
 			else
-				xdp_return_frame(buf->xdpf);
+				xdp_return_frame_bulk(buf->xdpf, &bq);
 		}
 	}
+	xdp_flush_frame_bulk(&bq);
 
 	netdev_tx_completed_queue(nq, pkts_compl, bytes_compl);
 }
include/net/xdp.h +9 -0
index 3814fb631d52..9567110845ef 100644
@@ -104,6 +104,12 @@ struct xdp_frame {
 	struct net_device *dev_rx; /* used by cpumap */
 };
 
+#define XDP_BULK_QUEUE_SIZE	16
+struct xdp_frame_bulk {
+	void *q[XDP_BULK_QUEUE_SIZE];
+	int count;
+	void *xa;
+};
 
 static inline struct skb_shared_info *
 xdp_get_shared_info_from_frame(struct xdp_frame *frame)

@@ -194,6 +200,9 @@ struct xdp_frame *xdp_convert_buff_to_frame(struct xdp_buff *xdp)
 void xdp_return_frame(struct xdp_frame *xdpf);
 void xdp_return_frame_rx_napi(struct xdp_frame *xdpf);
 void xdp_return_buff(struct xdp_buff *xdp);
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq);
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq);
 
 /* When sending xdp_frame into the network stack, then there is no
  * return point callback, which is needed to release e.g. DMA-mapping
net/core/xdp.c +22 -0
index 48aba933a5a8..93eabd789246 100644
@@ -380,6 +380,57 @@ void xdp_return_frame_rx_napi(struct xdp_frame *xdpf)
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_allocator *xa = bq->xa;
+	int i;
+
+	if (unlikely(!xa))
+		return;
+
+	for (i = 0; i < bq->count; i++) {
+		struct page *page = virt_to_head_page(bq->q[i]);
+
+		page_pool_put_full_page(xa->page_pool, page, false);
+	}
+	bq->count = 0;
+}
+EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
+
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 414

Quoted 10 lines
net/core/xdp.c +10 -0
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {

Why is this marked as unlikely? The driver passes it as NULL. Should unlikely be checked on both xa and the comparison?

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 415

Quoted 1 line
net/core/xdp.c +1 -0
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);

Is there a chance nxa can be NULL?

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 416

Quoted 1 line
net/core/xdp.c +1 -0
+		if (unlikely(!xa)) {

Same here, driver passes it as NULL

Quoted 23 lines
net/core/xdp.c +17 -0
+			bq->count = 0;
+			bq->xa = nxa;
+			xa = nxa;
+		}
+	}
+
+	if (mem->id != xa->mem.id || bq->count == XDP_BULK_QUEUE_SIZE)
+		xdp_flush_frame_bulk(bq);
+
+	bq->q[bq->count++] = xdpf->data;
+	if (mem->id != xa->mem.id)
+		bq->xa = nxa;
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(xdp_return_frame_bulk);
+
 void xdp_return_buff(struct xdp_buff *xdp)
 {
 	__xdp_return(xdp->data, &xdp->rxq->mem, true);
Signature · 1 line
2.26.2

Cheers
/Ilias

author
#3 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

Quoted 1 line

Hi Lorenzo,

Hi Ilias,

thx for the review.

Quoted 2 lines · Lorenzo Bianconi

[...]

Quoted 10 lines
Quoted 10 lines
net/core/xdp.c +10 -0
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
Quoted 8 lines

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 414

Quoted 5 lines
net/core/xdp.c +5 -0
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {

Why is this marked as unlikely? The driver passes it as NULL. Should unlikely be checked on both xa and the comparison?

xa is NULL only for the first xdp_frame in the burst while it is set for subsequent ones. Do you think it is better to remove it?

Quoted 4 lines

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 415

Quoted 2 lines
net/core/xdp.c +1 -0
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);

Is there a chance nxa can be NULL?

I do not think so since the page_pool is not destroyed while there are in-flight pages, right?

Regards,
Lorenzo

Quoted 31 lines

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 416

Quoted 2 lines
net/core/xdp.c +1 -0
+		if (unlikely(!xa)) {

Same here, driver passes it as NULL

Quoted 23 lines
net/core/xdp.c +17 -0
+			bq->count = 0;
+			bq->xa = nxa;
+			xa = nxa;
+		}
+	}
+
+	if (mem->id != xa->mem.id || bq->count == XDP_BULK_QUEUE_SIZE)
+		xdp_flush_frame_bulk(bq);
+
+	bq->q[bq->count++] = xdpf->data;
+	if (mem->id != xa->mem.id)
+		bq->xa = nxa;
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(xdp_return_frame_bulk);
+
 void xdp_return_buff(struct xdp_buff *xdp)
 {
 	__xdp_return(xdp->data, &xdp->rxq->mem, true);
Signature · 1 line
2.26.2

Cheers
/Ilias

maintainer
#4 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

Quoted 30 lines · Lorenzo Bianconi
Quoted 1 line

Hi Lorenzo,

Hi Ilias,

thx for the review.

Quoted 2 lines · Lorenzo Bianconi

[...]

Quoted 10 lines
Quoted 10 lines
net/core/xdp.c +10 -0
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
Quoted 8 lines

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 414

Quoted 5 lines
net/core/xdp.c +5 -0
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {

Why is this marked as unlikely? The driver passes it as NULL. Should unlikely be checked on both xa and the comparison?

Quoted 2 lines

xa is NULL only for the first xdp_frame in the burst while it is set for subsequent ones. Do you think it is better to remove it?

Ah correct, missed the general context of the driver this runs in.

Quoted 8 lines
Quoted 5 lines

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 415

Quoted 3 lines
net/core/xdp.c +1 -0
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);

Is there a chance nxa can be NULL?

I do not think so since the page_pool is not destroyed while there are in-flight pages, right?

I think so but I am not 100% sure. I'll apply the patch and have a closer look

Cheers
/Ilias

author
#5 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

Quoted 38 lines · Lorenzo Bianconi
Quoted 30 lines
Quoted 1 line

Hi Lorenzo,

Hi Ilias,

thx for the review.

Quoted 2 lines · Lorenzo Bianconi

[...]

Quoted 10 lines
Quoted 10 lines
net/core/xdp.c +10 -0
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
Quoted 8 lines

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 414

Quoted 5 lines
net/core/xdp.c +5 -0
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {

Why is this marked as unlikely? The driver passes it as NULL. Should unlikely be checked on both xa and the comparison?

Quoted 2 lines

xa is NULL only for the first xdp_frame in the burst while it is set for subsequent ones. Do you think it is better to remove it?

Ah correct, missed the general context of the driver this runs in.

Quoted 2 lines
Quoted 2 lines
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 415

Quoted 1 line
net/core/xdp.c +1 -0
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);

Is there a chance nxa can be NULL?

I do not think so since the page_pool is not destroyed while there are in-flight pages, right?

I think so but I am not 100% sure. I'll apply the patch and have a closer look

ack, thx. I converted socionext driver to bulking APIs but I have not posted the patch since I have not been able to test it. The code is available here:

https://github.com/LorenzoBianconi/net-next/commit/88c2995bca051fa38860acf7b915c90768460d37

Regards,
Lorenzo

Quoted 3 lines

Cheers
/Ilias

#6 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

In reply to Lorenzo Bianconi
Quoted 7 lines · On Tue, 27 Oct 2020 20:04:07 +0100 Lorenzo Bianconi <[email protected]> wrote:

Introduce bulking capability in xdp tx return path (XDP_REDIRECT). xdp_return_frame is usually run inside the driver NAPI tx completion loop so it is possible batch it.
Current implementation considers only page_pool memory model. Convert mvneta driver to xdp_return_frame_bulk APIs.

Suggested-by Jesper Dangaard Brouer <[email protected]>

I think you/we have to explain better in this commit message, what the idea/concept behind this bulk return is. Or even explain this as a comment above "xdp_return_frame_bulk".

Maybe add/append text to commit below:

The bulk API introduced is a defer and flush API, that will defer the return if the xdp_mem_allocator object is the same, identified via the mem.id field (xdp_mem_info). Thus, the flush operation will operate on the same xdp_mem_allocator object.

The bulk queue size of 16 is no coincident. This is connected to how XDP redirect will bulk xmit (upto 16) frames. Thus, the idea is for the API to find these boundaries (via mem.id match), which is optimal for both the I-cache and D-cache for the memory allocator code and object.

The data structure (xdp_frame_bulk) used for deferred elements is stored/allocated on the function call-stack, which allows lockfree access.

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

---

[...]

include/net/xdp.h struct xdp_frame { · line 107

Quoted 9 lines
include/net/xdp.h +1 -0
index 3814fb631d52..9567110845ef 100644
@@ -104,6 +104,12 @@ struct xdp_frame {
 	struct net_device *dev_rx; /* used by cpumap */
 };
 
+#define XDP_BULK_QUEUE_SIZE	16

Maybe "#define DEV_MAP_BULK_SIZE 16" should be def to XDP_BULK_QUEUE_SIZE, to express the described connection.

include/net/xdp.h struct xdp_frame { · line 111

Quoted 4 lines
include/net/xdp.h +4 -0
+struct xdp_frame_bulk {
+	void *q[XDP_BULK_QUEUE_SIZE];
+	int count;
+	void *xa;

Just a hunch (not benchmarked), but I think it will be more optimal to place 'count' and '*xa' above the '*q' array. (It might not matter at all, as we cannot control the start alignment, when this is on the stack.)

Quoted 1 line
include/net/xdp.h +1 -0
+};

[...]

Quoted 15 lines
net/core/xdp.c +7 -0
index 48aba933a5a8..93eabd789246 100644
@@ -380,6 +380,57 @@ void xdp_return_frame_rx_napi(struct xdp_frame *xdpf)
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_allocator *xa = bq->xa;
+	int i;
+
+	if (unlikely(!xa))
+		return;

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 399

Quoted 10 lines
net/core/xdp.c +10 -0
+
+	for (i = 0; i < bq->count; i++) {
+		struct page *page = virt_to_head_page(bq->q[i]);
+
+		page_pool_put_full_page(xa->page_pool, page, false);
+	}
+	bq->count = 0;
+}
+EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
+

Wondering if we should have a comment that explains the intent and idea behind this function?

/* Defers return when frame belongs to same mem.id as previous frame */

Quoted 33 lines
net/core/xdp.c +33 -0
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
+		if (unlikely(!xa)) {
+			bq->count = 0;
+			bq->xa = nxa;
+			xa = nxa;
+		}
+	}
+
+	if (mem->id != xa->mem.id || bq->count == XDP_BULK_QUEUE_SIZE)
+		xdp_flush_frame_bulk(bq);
+
+	bq->q[bq->count++] = xdpf->data;
+	if (mem->id != xa->mem.id)
+		bq->xa = nxa;
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(xdp_return_frame_bulk);
Signature · 4 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
author
#7 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

Quoted 10 lines
Quoted 7 lines · On Tue, 27 Oct 2020 20:04:07 +0100 Lorenzo Bianconi <[email protected]> wrote:

Introduce bulking capability in xdp tx return path (XDP_REDIRECT). xdp_return_frame is usually run inside the driver NAPI tx completion loop so it is possible batch it.
Current implementation considers only page_pool memory model. Convert mvneta driver to xdp_return_frame_bulk APIs.

Suggested-by Jesper Dangaard Brouer <[email protected]>

Hi Jesper,

thx for the review.

Quoted 12 lines

I think you/we have to explain better in this commit message, what the idea/concept behind this bulk return is. Or even explain this as a comment above "xdp_return_frame_bulk".

Maybe add/append text to commit below:

The bulk API introduced is a defer and flush API, that will defer the return if the xdp_mem_allocator object is the same, identified via the mem.id field (xdp_mem_info). Thus, the flush operation will operate on the same xdp_mem_allocator object.

Quoted 8 lines

The bulk queue size of 16 is no coincident. This is connected to how XDP redirect will bulk xmit (upto 16) frames. Thus, the idea is for the API to find these boundaries (via mem.id match), which is optimal for both the I-cache and D-cache for the memory allocator code and object.

The data structure (xdp_frame_bulk) used for deferred elements is stored/allocated on the function call-stack, which allows lockfree access.

ack, I will add it in v2

Quoted 13 lines
Quoted 4 lines
Signed-off-by Lorenzo Bianconi <[email protected]>

---

[...]

Quoted 8 lines
include/net/xdp.h +0 -0
index 3814fb631d52..9567110845ef 100644
@@ -104,6 +104,12 @@ struct xdp_frame {
 	struct net_device *dev_rx; /* used by cpumap */
 };
Quoted 4 lines

include/net/xdp.h struct xdp_frame { · line 107

Quoted 1 line
include/net/xdp.h +1 -0
+#define XDP_BULK_QUEUE_SIZE	16

Maybe "#define DEV_MAP_BULK_SIZE 16" should be def to XDP_BULK_QUEUE_SIZE, to express the described connection.

ack, I guess we can fix it in a following patch

Quoted 10 lines

include/net/xdp.h struct xdp_frame { · line 111

Quoted 5 lines
include/net/xdp.h +4 -0
+struct xdp_frame_bulk {
+	void *q[XDP_BULK_QUEUE_SIZE];
+	int count;
+	void *xa;

Just a hunch (not benchmarked), but I think it will be more optimal to place 'count' and '*xa' above the '*q' array. (It might not matter at all, as we cannot control the start alignment, when this is on the stack.)

ack. I will fix in v2.

Quoted 33 lines
Quoted 2 lines
include/net/xdp.h +1 -0
+};

[...]

Quoted 15 lines
net/core/xdp.c +7 -0
index 48aba933a5a8..93eabd789246 100644
@@ -380,6 +380,57 @@ void xdp_return_frame_rx_napi(struct xdp_frame *xdpf)
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_allocator *xa = bq->xa;
+	int i;
+
+	if (unlikely(!xa))
+		return;

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 399

Quoted 10 lines
net/core/xdp.c +10 -0
+
+	for (i = 0; i < bq->count; i++) {
+		struct page *page = virt_to_head_page(bq->q[i]);
+
+		page_pool_put_full_page(xa->page_pool, page, false);
+	}
+	bq->count = 0;
+}
+EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
+

Wondering if we should have a comment that explains the intent and idea behind this function?

Quoted 2 lines

/* Defers return when frame belongs to same mem.id as previous frame */

ack.

Regards,
Lorenzo

Quoted 41 lines
Quoted 33 lines
net/core/xdp.c +33 -0
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
+		if (unlikely(!xa)) {
+			bq->count = 0;
+			bq->xa = nxa;
+			xa = nxa;
+		}
+	}
+
+	if (mem->id != xa->mem.id || bq->count == XDP_BULK_QUEUE_SIZE)
+		xdp_flush_frame_bulk(bq);
+
+	bq->q[bq->count++] = xdpf->data;
+	if (mem->id != xa->mem.id)
+		bq->xa = nxa;
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(xdp_return_frame_bulk);
Signature · 4 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
#8 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

In reply to Lorenzo Bianconi
Quoted 48 lines · On Tue, 27 Oct 2020 20:04:07 +0100 Lorenzo Bianconi <[email protected]> wrote:
net/core/xdp.c +40 -0
index 48aba933a5a8..93eabd789246 100644
@@ -380,6 +380,57 @@ void xdp_return_frame_rx_napi(struct xdp_frame *xdpf)
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_allocator *xa = bq->xa;
+	int i;
+
+	if (unlikely(!xa))
+		return;
+
+	for (i = 0; i < bq->count; i++) {
+		struct page *page = virt_to_head_page(bq->q[i]);
+
+		page_pool_put_full_page(xa->page_pool, page, false);
+	}
+	bq->count = 0;
+}
+EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
+
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
+		if (unlikely(!xa)) {
+			bq->count = 0;
+			bq->xa = nxa;
+			xa = nxa;
+		}
+	}
+

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 432

Quoted 10 lines
net/core/xdp.c +10 -0
+	if (mem->id != xa->mem.id || bq->count == XDP_BULK_QUEUE_SIZE)
+		xdp_flush_frame_bulk(bq);
+
+	bq->q[bq->count++] = xdpf->data;
+	if (mem->id != xa->mem.id)
+		bq->xa = nxa;
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(xdp_return_frame_bulk);

We (Ilias my co-maintainer and I) think above code is hard to read and understand (as a reader you need to keep too many cases in your head).

I think we both have proposals to improve this, here is mine:

/* Defers return when frame belongs to same mem.id as previous frame */ void xdp_return_frame_bulk(struct xdp_frame *xdpf,
struct xdp_frame_bulk *bq)

{
        struct xdp_mem_info *mem = &xdpf->mem;
        struct xdp_mem_allocator *xa;

        if (mem->type != MEM_TYPE_PAGE_POOL) {
                __xdp_return(xdpf->data, &xdpf->mem, false);
                return;
        }

        rcu_read_lock();

        xa = bq->xa;
        if (unlikely(!xa)) {
		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
                bq->count = 0;
                bq->xa = xa;
        }

        if (bq->count == XDP_BULK_QUEUE_SIZE)
                xdp_flush_frame_bulk(bq);

        if (mem->id != xa->mem.id) {
		xdp_flush_frame_bulk(bq);
		bq->xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
        }

	bq->q[bq->count++] = xdpf->data;

        rcu_read_unlock();
}

Please review for correctness, and also for readability.

Signature · 4 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
#9 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

Quoted 101 lines
Quoted 48 lines · On Tue, 27 Oct 2020 20:04:07 +0100 Lorenzo Bianconi <[email protected]> wrote:
net/core/xdp.c +40 -0
index 48aba933a5a8..93eabd789246 100644
@@ -380,6 +380,57 @@ void xdp_return_frame_rx_napi(struct xdp_frame *xdpf)
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_allocator *xa = bq->xa;
+	int i;
+
+	if (unlikely(!xa))
+		return;
+
+	for (i = 0; i < bq->count; i++) {
+		struct page *page = virt_to_head_page(bq->q[i]);
+
+		page_pool_put_full_page(xa->page_pool, page, false);
+	}
+	bq->count = 0;
+}
+EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
+
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
+		if (unlikely(!xa)) {
+			bq->count = 0;
+			bq->xa = nxa;
+			xa = nxa;
+		}
+	}
+

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 432

Quoted 10 lines
net/core/xdp.c +10 -0
+	if (mem->id != xa->mem.id || bq->count == XDP_BULK_QUEUE_SIZE)
+		xdp_flush_frame_bulk(bq);
+
+	bq->q[bq->count++] = xdpf->data;
+	if (mem->id != xa->mem.id)
+		bq->xa = nxa;
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(xdp_return_frame_bulk);

We (Ilias my co-maintainer and I) think above code is hard to read and understand (as a reader you need to keep too many cases in your head).

I think we both have proposals to improve this, here is mine:

/* Defers return when frame belongs to same mem.id as previous frame */ void xdp_return_frame_bulk(struct xdp_frame *xdpf,
struct xdp_frame_bulk *bq)

{
        struct xdp_mem_info *mem = &xdpf->mem;
        struct xdp_mem_allocator *xa;

        if (mem->type != MEM_TYPE_PAGE_POOL) {
                __xdp_return(xdpf->data, &xdpf->mem, false);
                return;
        }

        rcu_read_lock();

        xa = bq->xa;
        if (unlikely(!xa)) {
		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
                bq->count = 0;
                bq->xa = xa;
        }

        if (bq->count == XDP_BULK_QUEUE_SIZE)
                xdp_flush_frame_bulk(bq);

        if (mem->id != xa->mem.id) {
		xdp_flush_frame_bulk(bq);
		bq->xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
        }

	bq->q[bq->count++] = xdpf->data;

        rcu_read_unlock();
}
Quoted 1 line

Please review for correctness, and also for readability.

the code seems fine to me (and even easier to read :)). I will update v2 using this approach. Thx.

Regards,
Lorenzo

Quoted 7 lines
Signature · 5 lines
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
maintainer
#10 lore raw

Re: [PATCH net-next 1/4] net: xdp: introduce bulking for xdp tx return path

Quoted 98 lines · Lorenzo Bianconi
Quoted 98 lines
Quoted 48 lines · On Tue, 27 Oct 2020 20:04:07 +0100 Lorenzo Bianconi <[email protected]> wrote:
net/core/xdp.c +40 -0
index 48aba933a5a8..93eabd789246 100644
@@ -380,6 +380,57 @@ void xdp_return_frame_rx_napi(struct xdp_frame *xdpf)
 }
 EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 
+void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_allocator *xa = bq->xa;
+	int i;
+
+	if (unlikely(!xa))
+		return;
+
+	for (i = 0; i < bq->count; i++) {
+		struct page *page = virt_to_head_page(bq->q[i]);
+
+		page_pool_put_full_page(xa->page_pool, page, false);
+	}
+	bq->count = 0;
+}
+EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
+
+void xdp_return_frame_bulk(struct xdp_frame *xdpf,
+			   struct xdp_frame_bulk *bq)
+{
+	struct xdp_mem_info *mem = &xdpf->mem;
+	struct xdp_mem_allocator *xa, *nxa;
+
+	if (mem->type != MEM_TYPE_PAGE_POOL) {
+		__xdp_return(xdpf->data, &xdpf->mem, false);
+		return;
+	}
+
+	rcu_read_lock();
+
+	xa = bq->xa;
+	if (unlikely(!xa || mem->id != xa->mem.id)) {
+		nxa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
+		if (unlikely(!xa)) {
+			bq->count = 0;
+			bq->xa = nxa;
+			xa = nxa;
+		}
+	}
+

net/core/xdp.c void xdp_return_frame_rx_napi(struct xdp_frame *xdpf) · line 432

Quoted 10 lines
net/core/xdp.c +10 -0
+	if (mem->id != xa->mem.id || bq->count == XDP_BULK_QUEUE_SIZE)
+		xdp_flush_frame_bulk(bq);
+
+	bq->q[bq->count++] = xdpf->data;
+	if (mem->id != xa->mem.id)
+		bq->xa = nxa;
+
+	rcu_read_unlock();
+}
+EXPORT_SYMBOL_GPL(xdp_return_frame_bulk);

We (Ilias my co-maintainer and I) think above code is hard to read and understand (as a reader you need to keep too many cases in your head).

I think we both have proposals to improve this, here is mine:

/* Defers return when frame belongs to same mem.id as previous frame */ void xdp_return_frame_bulk(struct xdp_frame *xdpf,
struct xdp_frame_bulk *bq)

{
        struct xdp_mem_info *mem = &xdpf->mem;
        struct xdp_mem_allocator *xa;

        if (mem->type != MEM_TYPE_PAGE_POOL) {
                __xdp_return(xdpf->data, &xdpf->mem, false);
                return;
        }

        rcu_read_lock();

        xa = bq->xa;
        if (unlikely(!xa)) {
		xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
                bq->count = 0;
                bq->xa = xa;
        }

        if (bq->count == XDP_BULK_QUEUE_SIZE)
                xdp_flush_frame_bulk(bq);

        if (mem->id != xa->mem.id) {
		xdp_flush_frame_bulk(bq);
		bq->xa = rhashtable_lookup(mem_id_ht, &mem->id, mem_id_rht_params);
        }

	bq->q[bq->count++] = xdpf->data;
Quoted 7 lines
Quoted 4 lines
        rcu_read_unlock();
}

Please review for correctness, and also for readability.

the code seems fine to me (and even easier to read :)). I will update v2 using this approach. Thx.

+1 this is close to what we discussed this morning and it detangles 1 more 'weird' if case

Thanks
/Ilias

Quoted 11 lines

Regards,
Lorenzo

Quoted 7 lines
Signature · 5 lines
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
author
#11 lore raw

PATCH 2/4 net: page_pool: add bulk support for ptr_ring

In reply to Lorenzo Bianconi

Introduce the capability to batch page_pool ptr_ring refill since it is usually run inside the driver NAPI tx completion loop.

Suggested-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 include/net/page_pool.h | 26 ++++++++++++++++++++++++++
 net/core/page_pool.c    | 33 +++++++++++++++++++++++++++++++++
 net/core/xdp.c          |  9 ++-------
 3 files changed, 61 insertions(+), 7 deletions(-)
include/net/page_pool.h +26 -0
index 81d7773f96cd..b5b195305346 100644
@@ -152,6 +152,8 @@ struct page_pool *page_pool_create(const struct page_pool_params *params);
 void page_pool_destroy(struct page_pool *pool);
 void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *));
 void page_pool_release_page(struct page_pool *pool, struct page *page);
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count);
 #else
 static inline void page_pool_destroy(struct page_pool *pool)
 {

@@ -165,6 +167,11 @@ static inline void page_pool_release_page(struct page_pool *pool,
 					  struct page *page)
 {
 }
+
+static inline void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+					   int count)
+{
+}
 #endif
 
 void page_pool_put_page(struct page_pool *pool, struct page *page,

@@ -215,4 +222,23 @@ static inline void page_pool_nid_changed(struct page_pool *pool, int new_nid)
 	if (unlikely(pool->p.nid != new_nid))
 		page_pool_update_nid(pool, new_nid);
 }
+
+static inline void page_pool_ring_lock(struct page_pool *pool)
+	__acquires(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_lock(&pool->ring.producer_lock);
+	else
+		spin_lock_bh(&pool->ring.producer_lock);
+}
+
+static inline void page_pool_ring_unlock(struct page_pool *pool)
+	__releases(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_unlock(&pool->ring.producer_lock);
+	else
+		spin_unlock_bh(&pool->ring.producer_lock);
+}
+
 #endif /* _NET_PAGE_POOL_H */
net/core/page_pool.c +33 -0
index ef98372facf6..84fb21f8865e 100644
@@ -11,6 +11,8 @@
 #include <linux/device.h>
 
 #include <net/page_pool.h>
+#include <net/xdp.h>
+
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/page-flags.h>

@@ -408,6 +410,37 @@ void page_pool_put_page(struct page_pool *pool, struct page *page,
 }
 EXPORT_SYMBOL(page_pool_put_page);
 
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);
+			put_page(page);
+			continue;
+		}
+
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);
+
+		page_ring[len++] = page;
+	}
+
+	page_pool_ring_lock(pool);
+	for (i = 0; i < len; i++) {
+		if (__ptr_ring_produce(&pool->ring, page_ring[i]))
+			page_pool_return_page(pool, page_ring[i]);
+	}
+	page_pool_ring_unlock(pool);
+}
+EXPORT_SYMBOL(page_pool_put_page_bulk);
+
 static void page_pool_empty_ring(struct page_pool *pool)
 {
 	struct page *page;
net/core/xdp.c +2 -7
index 93eabd789246..9f9a8d14df38 100644
@@ -383,16 +383,11 @@ EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
 {
 	struct xdp_mem_allocator *xa = bq->xa;
-	int i;
 
-	if (unlikely(!xa))
+	if (unlikely(!xa || !bq->count))
 		return;
 
-	for (i = 0; i < bq->count; i++) {
-		struct page *page = virt_to_head_page(bq->q[i]);
-
-		page_pool_put_full_page(xa->page_pool, page, false);
-	}
+	page_pool_put_page_bulk(xa->page_pool, bq->q, bq->count);
 	bq->count = 0;
 }
 EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
Signature · 1 line
2.26.2
maintainer
#12 lore raw

Re: [PATCH net-next 2/4] net: page_pool: add bulk support for ptr_ring

Quoted 93 lines · Lorenzo Bianconi

Introduce the capability to batch page_pool ptr_ring refill since it is usually run inside the driver NAPI tx completion loop.

Suggested-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 include/net/page_pool.h | 26 ++++++++++++++++++++++++++
 net/core/page_pool.c    | 33 +++++++++++++++++++++++++++++++++
 net/core/xdp.c          |  9 ++-------
 3 files changed, 61 insertions(+), 7 deletions(-)
include/net/page_pool.h +26 -0
index 81d7773f96cd..b5b195305346 100644
@@ -152,6 +152,8 @@ struct page_pool *page_pool_create(const struct page_pool_params *params);
 void page_pool_destroy(struct page_pool *pool);
 void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *));
 void page_pool_release_page(struct page_pool *pool, struct page *page);
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count);
 #else
 static inline void page_pool_destroy(struct page_pool *pool)
 {

@@ -165,6 +167,11 @@ static inline void page_pool_release_page(struct page_pool *pool,
 					  struct page *page)
 {
 }
+
+static inline void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+					   int count)
+{
+}
 #endif
 
 void page_pool_put_page(struct page_pool *pool, struct page *page,

@@ -215,4 +222,23 @@ static inline void page_pool_nid_changed(struct page_pool *pool, int new_nid)
 	if (unlikely(pool->p.nid != new_nid))
 		page_pool_update_nid(pool, new_nid);
 }
+
+static inline void page_pool_ring_lock(struct page_pool *pool)
+	__acquires(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_lock(&pool->ring.producer_lock);
+	else
+		spin_lock_bh(&pool->ring.producer_lock);
+}
+
+static inline void page_pool_ring_unlock(struct page_pool *pool)
+	__releases(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_unlock(&pool->ring.producer_lock);
+	else
+		spin_unlock_bh(&pool->ring.producer_lock);
+}
+
 #endif /* _NET_PAGE_POOL_H */
net/core/page_pool.c +18 -0
index ef98372facf6..84fb21f8865e 100644
@@ -11,6 +11,8 @@
 #include <linux/device.h>
 
 #include <net/page_pool.h>
+#include <net/xdp.h>
+
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/page-flags.h>

@@ -408,6 +410,37 @@ void page_pool_put_page(struct page_pool *pool, struct page *page,
 }
 EXPORT_SYMBOL(page_pool_put_page);
 
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);
+			put_page(page);
+			continue;
+		}
+

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 438

Quoted 10 lines
net/core/page_pool.c +10 -0
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);
+
+		page_ring[len++] = page;
+	}
+
+	page_pool_ring_lock(pool);
+	for (i = 0; i < len; i++) {
+		if (__ptr_ring_produce(&pool->ring, page_ring[i]))
+			page_pool_return_page(pool, page_ring[i]);

Can we add a comment here on why the explicit spinlock needs to protect page_pool_return_page() as well instead of just using ptr_ring_produce()?

Quoted 34 lines
net/core/page_pool.c +5 -0
+	}
+	page_pool_ring_unlock(pool);
+}
+EXPORT_SYMBOL(page_pool_put_page_bulk);
+
 static void page_pool_empty_ring(struct page_pool *pool)
 {
 	struct page *page;
net/core/xdp.c +2 -7
index 93eabd789246..9f9a8d14df38 100644
@@ -383,16 +383,11 @@ EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
 {
 	struct xdp_mem_allocator *xa = bq->xa;
-	int i;
 
-	if (unlikely(!xa))
+	if (unlikely(!xa || !bq->count))
 		return;
 
-	for (i = 0; i < bq->count; i++) {
-		struct page *page = virt_to_head_page(bq->q[i]);
-
-		page_pool_put_full_page(xa->page_pool, page, false);
-	}
+	page_pool_put_page_bulk(xa->page_pool, bq->q, bq->count);
 	bq->count = 0;
 }
 EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
Signature · 1 line
2.26.2

Thanks
/Ilias

#13 lore raw

Re: [PATCH net-next 2/4] net: page_pool: add bulk support for ptr_ring

Quoted 14 lines · Lorenzo Bianconi
Quoted 13 lines

Introduce the capability to batch page_pool ptr_ring refill since it is usually run inside the driver NAPI tx completion loop.

Suggested-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 include/net/page_pool.h | 26 ++++++++++++++++++++++++++
 net/core/page_pool.c    | 33 +++++++++++++++++++++++++++++++++
 net/core/xdp.c          |  9 ++-------
 3 files changed, 61 insertions(+), 7 deletions(-)
include/net/page_pool.h +0 -0
index 81d7773f96cd..b5b195305346 100644

[...]

Quoted 21 lines
Quoted 21 lines
include/net/page_pool.h +21 -0
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);
+			put_page(page);
+			continue;
+		}
+
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);
+
+		page_ring[len++] = page;
+	}
Quoted 8 lines

include/net/page_pool.h

Quoted 5 lines
include/net/page_pool.h +5 -0
+
+	page_pool_ring_lock(pool);
+	for (i = 0; i < len; i++) {
+		if (__ptr_ring_produce(&pool->ring, page_ring[i]))
+			page_pool_return_page(pool, page_ring[i]);

Can we add a comment here on why the explicit spinlock needs to protect page_pool_return_page() as well instead of just using ptr_ring_produce()?

ack, will do in v2.

Regards,
Lorenzo

Quoted 39 lines
Quoted 35 lines
include/net/page_pool.h +5 -0
+	}
+	page_pool_ring_unlock(pool);
+}
+EXPORT_SYMBOL(page_pool_put_page_bulk);
+
 static void page_pool_empty_ring(struct page_pool *pool)
 {
 	struct page *page;
net/core/xdp.c +2 -7
index 93eabd789246..9f9a8d14df38 100644
@@ -383,16 +383,11 @@ EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
 {
 	struct xdp_mem_allocator *xa = bq->xa;
-	int i;
 
-	if (unlikely(!xa))
+	if (unlikely(!xa || !bq->count))
 		return;
 
-	for (i = 0; i < bq->count; i++) {
-		struct page *page = virt_to_head_page(bq->q[i]);
-
-		page_pool_put_full_page(xa->page_pool, page, false);
-	}
+	page_pool_put_page_bulk(xa->page_pool, bq->q, bq->count);
 	bq->count = 0;
 }
 EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
Signature · 1 line
2.26.2

Thanks
/Ilias

maintainer
#14 lore raw

Re: [PATCH net-next 2/4] net: page_pool: add bulk support for ptr_ring

In reply to Lorenzo Bianconi

Hi Lorenzo,

Quoted 79 lines · Lorenzo Bianconi

Introduce the capability to batch page_pool ptr_ring refill since it is usually run inside the driver NAPI tx completion loop.

Suggested-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 include/net/page_pool.h | 26 ++++++++++++++++++++++++++
 net/core/page_pool.c    | 33 +++++++++++++++++++++++++++++++++
 net/core/xdp.c          |  9 ++-------
 3 files changed, 61 insertions(+), 7 deletions(-)
include/net/page_pool.h +26 -0
index 81d7773f96cd..b5b195305346 100644
@@ -152,6 +152,8 @@ struct page_pool *page_pool_create(const struct page_pool_params *params);
 void page_pool_destroy(struct page_pool *pool);
 void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *));
 void page_pool_release_page(struct page_pool *pool, struct page *page);
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count);
 #else
 static inline void page_pool_destroy(struct page_pool *pool)
 {

@@ -165,6 +167,11 @@ static inline void page_pool_release_page(struct page_pool *pool,
 					  struct page *page)
 {
 }
+
+static inline void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+					   int count)
+{
+}
 #endif
 
 void page_pool_put_page(struct page_pool *pool, struct page *page,

@@ -215,4 +222,23 @@ static inline void page_pool_nid_changed(struct page_pool *pool, int new_nid)
 	if (unlikely(pool->p.nid != new_nid))
 		page_pool_update_nid(pool, new_nid);
 }
+
+static inline void page_pool_ring_lock(struct page_pool *pool)
+	__acquires(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_lock(&pool->ring.producer_lock);
+	else
+		spin_lock_bh(&pool->ring.producer_lock);
+}
+
+static inline void page_pool_ring_unlock(struct page_pool *pool)
+	__releases(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_unlock(&pool->ring.producer_lock);
+	else
+		spin_unlock_bh(&pool->ring.producer_lock);
+}
+
 #endif /* _NET_PAGE_POOL_H */
net/core/page_pool.c +4 -0
index ef98372facf6..84fb21f8865e 100644
@@ -11,6 +11,8 @@
 #include <linux/device.h>
 
 #include <net/page_pool.h>
+#include <net/xdp.h>
+
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/page-flags.h>

@@ -408,6 +410,37 @@ void page_pool_put_page(struct page_pool *pool, struct page *page,
 }
 EXPORT_SYMBOL(page_pool_put_page);
 
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 424

Quoted 10 lines
net/core/page_pool.c +10 -0
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);

Mind switching this similarly to how page_pool_put_page() is using it? unlikely -> likely and remove the !

Quoted 48 lines
net/core/page_pool.c +19 -0
+			put_page(page);
+			continue;
+		}
+
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);
+
+		page_ring[len++] = page;
+	}
+
+	page_pool_ring_lock(pool);
+	for (i = 0; i < len; i++) {
+		if (__ptr_ring_produce(&pool->ring, page_ring[i]))
+			page_pool_return_page(pool, page_ring[i]);
+	}
+	page_pool_ring_unlock(pool);
+}
+EXPORT_SYMBOL(page_pool_put_page_bulk);
+
 static void page_pool_empty_ring(struct page_pool *pool)
 {
 	struct page *page;
net/core/xdp.c +2 -7
index 93eabd789246..9f9a8d14df38 100644
@@ -383,16 +383,11 @@ EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
 {
 	struct xdp_mem_allocator *xa = bq->xa;
-	int i;
 
-	if (unlikely(!xa))
+	if (unlikely(!xa || !bq->count))
 		return;
 
-	for (i = 0; i < bq->count; i++) {
-		struct page *page = virt_to_head_page(bq->q[i]);
-
-		page_pool_put_full_page(xa->page_pool, page, false);
-	}
+	page_pool_put_page_bulk(xa->page_pool, bq->q, bq->count);
 	bq->count = 0;
 }
 EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
Signature · 1 line
2.26.2

Cheers
/Ilias

#15 lore raw

Re: [PATCH net-next 2/4] net: page_pool: add bulk support for ptr_ring

Quoted 80 lines

Hi Lorenzo,

Quoted 77 lines · Lorenzo Bianconi

Introduce the capability to batch page_pool ptr_ring refill since it is usually run inside the driver NAPI tx completion loop.

Suggested-by Jesper Dangaard Brouer <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 include/net/page_pool.h | 26 ++++++++++++++++++++++++++
 net/core/page_pool.c    | 33 +++++++++++++++++++++++++++++++++
 net/core/xdp.c          |  9 ++-------
 3 files changed, 61 insertions(+), 7 deletions(-)
include/net/page_pool.h +26 -0
index 81d7773f96cd..b5b195305346 100644
@@ -152,6 +152,8 @@ struct page_pool *page_pool_create(const struct page_pool_params *params);
 void page_pool_destroy(struct page_pool *pool);
 void page_pool_use_xdp_mem(struct page_pool *pool, void (*disconnect)(void *));
 void page_pool_release_page(struct page_pool *pool, struct page *page);
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count);
 #else
 static inline void page_pool_destroy(struct page_pool *pool)
 {

@@ -165,6 +167,11 @@ static inline void page_pool_release_page(struct page_pool *pool,
 					  struct page *page)
 {
 }
+
+static inline void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+					   int count)
+{
+}
 #endif
 
 void page_pool_put_page(struct page_pool *pool, struct page *page,

@@ -215,4 +222,23 @@ static inline void page_pool_nid_changed(struct page_pool *pool, int new_nid)
 	if (unlikely(pool->p.nid != new_nid))
 		page_pool_update_nid(pool, new_nid);
 }
+
+static inline void page_pool_ring_lock(struct page_pool *pool)
+	__acquires(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_lock(&pool->ring.producer_lock);
+	else
+		spin_lock_bh(&pool->ring.producer_lock);
+}
+
+static inline void page_pool_ring_unlock(struct page_pool *pool)
+	__releases(&pool->ring.producer_lock)
+{
+	if (in_serving_softirq())
+		spin_unlock(&pool->ring.producer_lock);
+	else
+		spin_unlock_bh(&pool->ring.producer_lock);
+}
+
 #endif /* _NET_PAGE_POOL_H */
net/core/page_pool.c +2 -0
index ef98372facf6..84fb21f8865e 100644
@@ -11,6 +11,8 @@
 #include <linux/device.h>
 
 #include <net/page_pool.h>
+#include <net/xdp.h>
+
 #include <linux/dma-direction.h>
 #include <linux/dma-mapping.h>
 #include <linux/page-flags.h>

@@ -408,6 +410,37 @@ void page_pool_put_page(struct page_pool *pool, struct page *page,
 }
 EXPORT_SYMBOL(page_pool_put_page);
Quoted 15 lines

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 424

Quoted 12 lines
net/core/page_pool.c +12 -0
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);

Mind switching this similarly to how page_pool_put_page() is using it? unlikely -> likely and remove the !

Hi Ilias,

thx for the review. ack, I will do it in v2

Regards,
Lorenzo

Quoted 53 lines
Quoted 49 lines
net/core/page_pool.c +19 -0
+			put_page(page);
+			continue;
+		}
+
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);
+
+		page_ring[len++] = page;
+	}
+
+	page_pool_ring_lock(pool);
+	for (i = 0; i < len; i++) {
+		if (__ptr_ring_produce(&pool->ring, page_ring[i]))
+			page_pool_return_page(pool, page_ring[i]);
+	}
+	page_pool_ring_unlock(pool);
+}
+EXPORT_SYMBOL(page_pool_put_page_bulk);
+
 static void page_pool_empty_ring(struct page_pool *pool)
 {
 	struct page *page;
net/core/xdp.c +2 -7
index 93eabd789246..9f9a8d14df38 100644
@@ -383,16 +383,11 @@ EXPORT_SYMBOL_GPL(xdp_return_frame_rx_napi);
 void xdp_flush_frame_bulk(struct xdp_frame_bulk *bq)
 {
 	struct xdp_mem_allocator *xa = bq->xa;
-	int i;
 
-	if (unlikely(!xa))
+	if (unlikely(!xa || !bq->count))
 		return;
 
-	for (i = 0; i < bq->count; i++) {
-		struct page *page = virt_to_head_page(bq->q[i]);
-
-		page_pool_put_full_page(xa->page_pool, page, false);
-	}
+	page_pool_put_page_bulk(xa->page_pool, bq->q, bq->count);
 	bq->count = 0;
 }
 EXPORT_SYMBOL_GPL(xdp_flush_frame_bulk);
Signature · 1 line
2.26.2

Cheers
/Ilias

#16 lore raw

Re: [PATCH net-next 2/4] net: page_pool: add bulk support for ptr_ring

In reply to Lorenzo Bianconi

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 416

Quoted 4 lines · On Tue, 27 Oct 2020 20:04:08 +0100 Lorenzo Bianconi <[email protected]> wrote:
net/core/page_pool.c +4 -0
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];

Maybe we could reuse the 'data' array instead of creating a new array (2 cache-lines long) for the array of pages?

Quoted 4 lines
net/core/page_pool.c +4 -0
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 430

Quoted 10 lines
net/core/page_pool.c +10 -0
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);
+			put_page(page);
+			continue;
+		}
+
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);

Here we sync the entire DMA area (-1), which have a *huge* cost for mvneta (especially on EspressoBin HW). For this xdp_frame->len is unfortunately not enough. We will need the *maximum* length touch by
(1) CPU and (2) remote device DMA engine. DMA-TX completion knows the length for (2). The CPU length (1) is max of original xdp_buff size and xdp_frame->len, because BPF-helpers could have shrinked the size. (tricky part is that xdp_frame->len isn't correct in-case of header adjustments, thus like mvneta_run_xdp we to calc dma_sync size, and store this in xdp_frame, maybe via param to xdp_do_redirect). Well, not sure if it is too much work to transfer this info, for this use-case.

Quoted 13 lines
net/core/page_pool.c +12 -0
+
+		page_ring[len++] = page;
 
+	}
+
+	page_pool_ring_lock(pool);
+	for (i = 0; i < len; i++) {
+		if (__ptr_ring_produce(&pool->ring, page_ring[i]))
+			page_pool_return_page(pool, page_ring[i]);
+	}
+	page_pool_ring_unlock(pool);
+}
+EXPORT_SYMBOL(page_pool_put_page_bulk);
Signature · 4 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
#17 lore raw

Re: [PATCH net-next 2/4] net: page_pool: add bulk support for ptr_ring

Quoted 10 lines

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 416

Quoted 4 lines · On Tue, 27 Oct 2020 20:04:08 +0100 Lorenzo Bianconi <[email protected]> wrote:
net/core/page_pool.c +4 -0
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];

Maybe we could reuse the 'data' array instead of creating a new array (2 cache-lines long) for the array of pages?

I agree, I will try to reuse the data array for that

Quoted 18 lines
Quoted 5 lines
net/core/page_pool.c +4 -0
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 430

Quoted 10 lines
net/core/page_pool.c +10 -0
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);
+			put_page(page);
+			continue;
+		}
+
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);

Here we sync the entire DMA area (-1), which have a *huge* cost for mvneta (especially on EspressoBin HW). For this xdp_frame->len is

Quoted 8 lines

unfortunately not enough. We will need the *maximum* length touch by
(1) CPU and (2) remote device DMA engine. DMA-TX completion knows the length for (2). The CPU length (1) is max of original xdp_buff size and xdp_frame->len, because BPF-helpers could have shrinked the size. (tricky part is that xdp_frame->len isn't correct in-case of header adjustments, thus like mvneta_run_xdp we to calc dma_sync size, and store this in xdp_frame, maybe via param to xdp_do_redirect). Well, not sure if it is too much work to transfer this info, for this use-case.

I was thinking about that but I guess point (1) is tricky since "cpu length" can be changed even in the middle by devmaps or cpumaps (not just in the driver rx napi loop). I guess we can try to address this point in a subsequent series. Agree?

Regards,
Lorenzo

Quoted 23 lines
Quoted 14 lines
net/core/page_pool.c +12 -0
+
+		page_ring[len++] = page;
 
+	}
+
+	page_pool_ring_lock(pool);
+	for (i = 0; i < len; i++) {
+		if (__ptr_ring_produce(&pool->ring, page_ring[i]))
+			page_pool_return_page(pool, page_ring[i]);
+	}
+	page_pool_ring_unlock(pool);
+}
+EXPORT_SYMBOL(page_pool_put_page_bulk);
Signature · 4 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
#18 lore raw

Re: [PATCH net-next 2/4] net: page_pool: add bulk support for ptr_ring

Quoted 40 lines · On Thu, 29 Oct 2020 11:31:48 +0100 Lorenzo Bianconi…
Quoted 10 lines

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 416

Quoted 4 lines · On Tue, 27 Oct 2020 20:04:08 +0100 Lorenzo Bianconi <[email protected]> wrote:
net/core/page_pool.c +4 -0
+void page_pool_put_page_bulk(struct page_pool *pool, void **data,
+			     int count)
+{
+	struct page *page_ring[XDP_BULK_QUEUE_SIZE];

Maybe we could reuse the 'data' array instead of creating a new array (2 cache-lines long) for the array of pages?

I agree, I will try to reuse the data array for that

Quoted 18 lines
Quoted 5 lines
net/core/page_pool.c +4 -0
+	int i, len = 0;
+
+	for (i = 0; i < count; i++) {
+		struct page *page = virt_to_head_page(data[i]);

net/core/page_pool.c void page_pool_put_page(struct page_pool *pool, struct page *page, · line 430

Quoted 10 lines
net/core/page_pool.c +10 -0
+
+		if (unlikely(page_ref_count(page) != 1 ||
+			     !pool_page_reusable(pool, page))) {
+			page_pool_release_page(pool, page);
+			put_page(page);
+			continue;
+		}
+
+		if (pool->p.flags & PP_FLAG_DMA_SYNC_DEV)
+			page_pool_dma_sync_for_device(pool, page, -1);

Here we sync the entire DMA area (-1), which have a *huge* cost for mvneta (especially on EspressoBin HW). For this xdp_frame->len is

Quoted 8 lines

unfortunately not enough. We will need the *maximum* length touch by
(1) CPU and (2) remote device DMA engine. DMA-TX completion knows the length for (2). The CPU length (1) is max of original xdp_buff size and xdp_frame->len, because BPF-helpers could have shrinked the size. (tricky part is that xdp_frame->len isn't correct in-case of header adjustments, thus like mvneta_run_xdp we to calc dma_sync size, and store this in xdp_frame, maybe via param to xdp_do_redirect). Well, not sure if it is too much work to transfer this info, for this use-case.

Quoted 4 lines

I was thinking about that but I guess point (1) is tricky since "cpu length" can be changed even in the middle by devmaps or cpumaps (not just in the driver rx napi loop). I guess we can try to address this point in a subsequent series. Agree?

I agree, that this change request goes beyond this series. But it becomes harder and harder to add later when this API is getting used in more and more drivers. Looking at 1/4 is can be extended later, as you just pass down xdpf in API driver use (and then queue xdpf->data).

Signature · 4 lines
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer
author
#19 lore raw

PATCH 3/4 net: mvpp2: add xdp tx return bulking support

In reply to Lorenzo Bianconi

Convert mvpp2 driver to xdp_return_frame_bulk APIs.

XDP_REDIRECT (upstream codepath): 1.79Mpps XDP_REDIRECT (upstream codepath + bulking APIs): 1.93Mpps

Tested-by Matteo Croce <[email protected]>
Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +4 -1
index f6616c8933ca..04f24d1d72ab 100644
@@ -2440,8 +2440,10 @@ static void mvpp2_txq_bufs_free(struct mvpp2_port *port,
 				struct mvpp2_tx_queue *txq,
 				struct mvpp2_txq_pcpu *txq_pcpu, int num)
 {
+	struct xdp_frame_bulk bq;
 	int i;
 
+	bq.xa = NULL;
 	for (i = 0; i < num; i++) {
 		struct mvpp2_txq_pcpu_buf *tx_buf =
 			txq_pcpu->buffs + txq_pcpu->txq_get_index;

@@ -2454,10 +2456,11 @@ static void mvpp2_txq_bufs_free(struct mvpp2_port *port,
 			dev_kfree_skb_any(tx_buf->skb);
 		else if (tx_buf->type == MVPP2_TYPE_XDP_TX ||
 			 tx_buf->type == MVPP2_TYPE_XDP_NDO)
-			xdp_return_frame(tx_buf->xdpf);
+			xdp_return_frame_bulk(tx_buf->xdpf, &bq);
 
 		mvpp2_txq_inc_get(txq_pcpu);
 	}
+	xdp_flush_frame_bulk(&bq);
 }
 
 static inline struct mvpp2_rx_queue *mvpp2_get_rx_queue(struct mvpp2_port *port,
Signature · 1 line
2.26.2
author
#20 lore raw

PATCH 4/4 net: mlx5: add xdp tx return bulking support

In reply to Lorenzo Bianconi

Convert mlx5 driver to xdp_return_frame_bulk APIs.

XDP_REDIRECT (upstream codepath): 8.5Mpps XDP_REDIRECT (upstream codepath + bulking APIs): 10.1Mpps

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +4 -1
index ae90d533a350..5fdfbf390d5c 100644
@@ -369,8 +369,10 @@ static void mlx5e_free_xdpsq_desc(struct mlx5e_xdpsq *sq,
 				  bool recycle)
 {
 	struct mlx5e_xdp_info_fifo *xdpi_fifo = &sq->db.xdpi_fifo;
+	struct xdp_frame_bulk bq;
 	u16 i;
 
+	bq.xa = NULL;
 	for (i = 0; i < wi->num_pkts; i++) {
 		struct mlx5e_xdp_info xdpi = mlx5e_xdpi_fifo_pop(xdpi_fifo);
 

@@ -379,7 +381,7 @@ static void mlx5e_free_xdpsq_desc(struct mlx5e_xdpsq *sq,
 			/* XDP_TX from the XSK RQ and XDP_REDIRECT */
 			dma_unmap_single(sq->pdev, xdpi.frame.dma_addr,
 					 xdpi.frame.xdpf->len, DMA_TO_DEVICE);
-			xdp_return_frame(xdpi.frame.xdpf);
+			xdp_return_frame_bulk(xdpi.frame.xdpf, &bq);
 			break;
 		case MLX5E_XDP_XMIT_MODE_PAGE:
 			/* XDP_TX from the regular RQ */

@@ -393,6 +395,7 @@ static void mlx5e_free_xdpsq_desc(struct mlx5e_xdpsq *sq,
 			WARN_ON_ONCE(true);
 		}
 	}
+	xdp_flush_frame_bulk(&bq);
 }
 
 bool mlx5e_poll_xdpsq_cq(struct mlx5e_cq *cq)
Signature · 1 line
2.26.2
#21 lore raw

Re: [PATCH net-next 4/4] net: mlx5: add xdp tx return bulking support

Quoted 42 lines · Lorenzo Bianconi

Convert mlx5 driver to xdp_return_frame_bulk APIs.

XDP_REDIRECT (upstream codepath): 8.5Mpps XDP_REDIRECT (upstream codepath + bulking APIs): 10.1Mpps

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +2 -0
index ae90d533a350..5fdfbf390d5c 100644
@@ -369,8 +369,10 @@ static void mlx5e_free_xdpsq_desc(struct 
 mlx5e_xdpsq *sq,
 				  bool recycle)
 {
 	struct mlx5e_xdp_info_fifo *xdpi_fifo = &sq->db.xdpi_fifo;
+	struct xdp_frame_bulk bq;
 	u16 i;
 
+	bq.xa = NULL;
 	for (i = 0; i < wi->num_pkts; i++) {
 		struct mlx5e_xdp_info xdpi =

mlx5e_xdpi_fifo_pop(xdpi_fifo);

drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +1 -1
@@ -379,7 +381,7 @@ static void mlx5e_free_xdpsq_desc(struct 
 mlx5e_xdpsq *sq,
 			/* XDP_TX from the XSK RQ and XDP_REDIRECT 
 */
 			dma_unmap_single(sq->pdev, 
 xdpi.frame.dma_addr,
 					 xdpi.frame.xdpf->len, 
 DMA_TO_DEVICE);
-			xdp_return_frame(xdpi.frame.xdpf);
+			xdp_return_frame_bulk(xdpi.frame.xdpf,

&bq);

drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +0 -0
 			break;
 		case MLX5E_XDP_XMIT_MODE_PAGE:
 			/* XDP_TX from the regular RQ */

drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c static void mlx5e_free_xdpsq_desc(struct · line 399

Quoted 5 lines
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +1 -0
@@ -393,6 +395,7 @@ static void mlx5e_free_xdpsq_desc(struct 
 mlx5e_xdpsq *sq,
 			WARN_ON_ONCE(true);
 		}
 	}
+	xdp_flush_frame_bulk(&bq);

While I understand the rational behind this patchset, using an intermediate buffer

	void *q[XDP_BULK_QUEUE_SIZE];

means more pressure on the data cache.

At the time I ran performance tests on mlx5 to see whether batching skbs before passing them to GRO would improve performance. On some flows I got worse performance. This function seems to have less Dcache contention than RX flow, but maybe some performance testing are needed here.

Quoted 3 lines
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +0 -0
 }

bool mlx5e_poll_xdpsq_cq(struct mlx5e_cq *cq)

#22 lore raw

Re: [PATCH net-next 4/4] net: mlx5: add xdp tx return bulking support

Quoted 47 lines
Quoted 36 lines · Lorenzo Bianconi

Convert mlx5 driver to xdp_return_frame_bulk APIs.

XDP_REDIRECT (upstream codepath): 8.5Mpps XDP_REDIRECT (upstream codepath + bulking APIs): 10.1Mpps

Signed-off-by Lorenzo Bianconi <[email protected]>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +2 -0
index ae90d533a350..5fdfbf390d5c 100644
@@ -369,8 +369,10 @@ static void mlx5e_free_xdpsq_desc(struct
 mlx5e_xdpsq *sq,
 				  bool recycle)
 {
 	struct mlx5e_xdp_info_fifo *xdpi_fifo = &sq->db.xdpi_fifo;
+	struct xdp_frame_bulk bq;
 	u16 i;
+	bq.xa = NULL;
 	for (i = 0; i < wi->num_pkts; i++) {
 		struct mlx5e_xdp_info xdpi =  mlx5e_xdpi_fifo_pop(xdpi_fifo);
  @@ -379,7 +381,7 @@ static void mlx5e_free_xdpsq_desc(struct

mlx5e_xdpsq *sq,

drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +1 -1
 			/* XDP_TX from the XSK RQ and XDP_REDIRECT  */
 			dma_unmap_single(sq->pdev,  xdpi.frame.dma_addr,
 					 xdpi.frame.xdpf->len,  DMA_TO_DEVICE);
-			xdp_return_frame(xdpi.frame.xdpf);
+			xdp_return_frame_bulk(xdpi.frame.xdpf, &bq);
 			break;
 		case MLX5E_XDP_XMIT_MODE_PAGE:
 			/* XDP_TX from the regular RQ */

drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c static void mlx5e_free_xdpsq_desc(struct mlx5e_xdpsq · line 399

Quoted 5 lines
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +1 -0
@@ -393,6 +395,7 @@ static void mlx5e_free_xdpsq_desc(struct mlx5e_xdpsq
 *sq,
 			WARN_ON_ONCE(true);
 		}
 	}
+	xdp_flush_frame_bulk(&bq);

While I understand the rational behind this patchset, using an intermediate buffer

Quoted 8 lines
	void *q[XDP_BULK_QUEUE_SIZE];

means more pressure on the data cache.

At the time I ran performance tests on mlx5 to see whether batching skbs before passing them to GRO would improve performance. On some flows I got worse performance.
This function seems to have less Dcache contention than RX flow, but maybe some performance testing are needed here.

Hi Shay,

this codepath is only activated for "redirected" frames (not for packets forwarded to networking stack). We run performance comparisons with the upstream code for this particular use-case and we reported a nice improvements (8.5Mpps vs 10.1Mpps).
Do you have in mind other possible performance tests to run?

Regards,
Lorenzo

Quoted 4 lines
Quoted 3 lines
drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c +0 -0
 }
 bool mlx5e_poll_xdpsq_cq(struct mlx5e_cq *cq)