add page_pool support for page recycling in veth driver

PATCH Applied Lorenzo Bianconi 6 participants 15 messages Apr 22, 2023 – Apr 25, 2023

Subsystems (3)

Networking Network drivers BPF

lore

Share & tools

lore

Lorenzo Bianconi

author

2023-04-22 18:54 UTC #0 lore raw

v2 cover add page_pool support for page recycling in veth driver

Introduce page_pool support in veth driver in order to recycle pages in veth_convert_skb_to_xdp_buff routine and avoid reallocating the skb through the page allocator when we run a xdp program on the device and we receive skbs from the stack.

Change since v1:
- remove page_pool checks in veth_convert_skb_to_xdp_buff() before allocating the pages
- recycle pages in the hot cache if build_skb fails in veth_convert_skb_to_xdp_buff()

Lorenzo Bianconi (2):
net: veth: add page_pool for page recycling
net: veth: add page_pool stats

 drivers/net/Kconfig |  2 ++
 drivers/net/veth.c  | 68 ++++++++++++++++++++++++++++++++++++++++-----
 2 files changed, 63 insertions(+), 7 deletions(-)

Signature · 1 line

2.40.0

Lorenzo Bianconi

author

2023-04-22 18:54 UTC #1 lore raw

PATCH v2 1/2 net: veth: add page_pool for page recycling

Introduce page_pool support in veth driver in order to recycle pages in veth_convert_skb_to_xdp_buff routine and avoid reallocating the skb through the page allocator.
The patch has been tested sending tcp traffic to a veth pair where the remote peer is running a simple xdp program just returning xdp_pass:

veth upstream codebase:
MTU 1500B: ~ 8Gbps
MTU 8000B: ~ 13.9Gbps

veth upstream codebase + pp support:
MTU 1500B: ~ 9.2Gbps
MTU 8000B: ~ 16.2Gbps

Tested-by Maryam Tahhan <[email protected]>

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

---
 drivers/net/Kconfig |  1 +
 drivers/net/veth.c  | 48 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)

drivers/net/Kconfig +1 -0

index c34bd432da27..368c6f5b327e 100644
@@ -402,6 +402,7 @@ config TUN_VNET_CROSS_LE
 
 config VETH
 	tristate "Virtual ethernet pair device"
+	select PAGE_POOL
 	help
 	  This device is a local ethernet tunnel. Devices are created in pairs.
 	  When one end receives the packet it appears on its pair and vice

drivers/net/veth.c +44 -4

110 lines not on this page · raw message

Signature · 1 line

2.40.0

Yunsheng Lin

2023-04-23 12:17 UTC #2 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736

Quoted 11 lines · Lorenzo Bianconi

struct veth_priv {

drivers/net/veth.c +1 -1

@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;
 
 		nskb = build_skb(page_address(page), PAGE_SIZE);

If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.

Quoted 31 lines

drivers/net/veth.c +5 -3

 		if (!nskb) {
-			put_page(page);
+			page_pool_put_full_page(rq->page_pool, page, true);
 			goto drop;
 		}
 
 		skb_reserve(nskb, VETH_XDP_HEADROOM);
+		skb_copy_header(nskb, skb);
+		skb_mark_for_recycle(nskb);
+
 		size = min_t(u32, skb->len, max_head_size);
 		if (skb_copy_bits(skb, 0, nskb->data, size)) {
 			consume_skb(nskb);

@@ -745,7 +750,6 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		}
 		skb_put(nskb, size);
 
-		skb_copy_header(nskb, skb);
 		head_off = skb_headroom(nskb) - skb_headroom(skb);
 		skb_headers_offset_update(nskb, head_off);

@@ -754,7 +758,7 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		len = skb->len - off;
 
 		for (i = 0; i < MAX_SKB_FRAGS && off < skb->len; i++) {
-			page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+			page = page_pool_dev_alloc_pages(rq->page_pool);
 			if (!page) {
 				consume_skb(nskb);
 				goto drop;

drivers/net/veth.c static int veth_poll(struct napi_struct *napi, int budget) · line 1013

Quoted 8 lines

drivers/net/veth.c +5 -0

@@ -1002,11 +1006,37 @@ static int veth_poll(struct napi_struct *napi, int budget)
 	return done;
 }
 
+static int veth_create_page_pool(struct veth_rq *rq)
+{
+	struct page_pool_params pp_params = {
+		.order = 0,
+		.pool_size = VETH_RING_SIZE,

It seems better to allocate different poo_size according to the mtu, so that the best proformance is achiced using the least memory?

Lorenzo Bianconi

2023-04-23 14:21 UTC #3 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

Quoted 11 lines · Lorenzo Bianconi

Quoted 10 lines

struct veth_priv {

drivers/net/veth.c +1 -1

@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;

Quoted 6 lines

drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
drivers/net/veth.c +0 -0
 		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.

please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:

- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258
- max_headsize = SKB_WITH_OVERHEAD(VETH_BUF_SIZE - VETH_XDP_HEADROOM) = 1470

Even in this case we will need the consume a full page. In fact, performances are a little bit worse:

MTU 1500: tcp throughput ~ 8.3Gbps

Do you agree or am I missing something?

Regards,
Lorenzo

Quoted 46 lines

Quoted 33 lines

drivers/net/veth.c +5 -3

 		if (!nskb) {
-			put_page(page);
+			page_pool_put_full_page(rq->page_pool, page, true);
 			goto drop;
 		}
 
 		skb_reserve(nskb, VETH_XDP_HEADROOM);
+		skb_copy_header(nskb, skb);
+		skb_mark_for_recycle(nskb);
+
 		size = min_t(u32, skb->len, max_head_size);
 		if (skb_copy_bits(skb, 0, nskb->data, size)) {
 			consume_skb(nskb);

@@ -745,7 +750,6 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		}
 		skb_put(nskb, size);
 
-		skb_copy_header(nskb, skb);
 		head_off = skb_headroom(nskb) - skb_headroom(skb);
 		skb_headers_offset_update(nskb, head_off);

@@ -754,7 +758,7 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 		len = skb->len - off;
 
 		for (i = 0; i < MAX_SKB_FRAGS && off < skb->len; i++) {
-			page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+			page = page_pool_dev_alloc_pages(rq->page_pool);
 			if (!page) {
 				consume_skb(nskb);
 				goto drop;

drivers/net/veth.c static int veth_poll(struct napi_struct *napi, int budget) · line 1013

Quoted 8 lines

drivers/net/veth.c +5 -0

@@ -1002,11 +1006,37 @@ static int veth_poll(struct napi_struct *napi, int budget)
 	return done;
 }
 
+static int veth_create_page_pool(struct veth_rq *rq)
+{
+	struct page_pool_params pp_params = {
+		.order = 0,
+		.pool_size = VETH_RING_SIZE,

It seems better to allocate different poo_size according to the mtu, so that the best proformance is achiced using the least memory?

Yunsheng Lin

2023-04-24 02:30 UTC #4 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

Quoted 18 lines · Lorenzo Bianconi

Quoted 11 lines · Lorenzo Bianconi
Quoted 10 lines
struct veth_priv {
drivers/net/veth.c +1 -1
@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;
 
Quoted 6 lines
drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
drivers/net/veth.c +0 -0
 		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.

Quoted 6 lines

please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:
- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258

On some arch the NET_IP_ALIGN is zero.

I suppose XDP_PACKET_HEADROOM are for xdp_frame and data_meta, it seems xdp_frame is only 40 bytes for 64 bit arch and max size of metalen is 32 as xdp_metalen_invalid() suggest, is there any other reason why we need 256 bytes here?

Quoted 11 lines

- max_headsize = SKB_WITH_OVERHEAD(VETH_BUF_SIZE - VETH_XDP_HEADROOM) = 1470
Even in this case we will need the consume a full page. In fact, performances are a little bit worse:
MTU 1500: tcp throughput ~ 8.3Gbps
Do you agree or am I missing something?
Regards,
Lorenzo

Lorenzo Bianconi

2023-04-24 09:17 UTC #5 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

Quoted 26 lines · Lorenzo Bianconi

Quoted 18 lines
Quoted 11 lines · Lorenzo Bianconi
Quoted 10 lines
struct veth_priv {
drivers/net/veth.c +1 -1
@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;
 
Quoted 6 lines
drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
drivers/net/veth.c +0 -0
 		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.
Quoted 6 lines
please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:
- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258

Quoted 6 lines

On some arch the NET_IP_ALIGN is zero.
I suppose XDP_PACKET_HEADROOM are for xdp_frame and data_meta, it seems xdp_frame is only 40 bytes for 64 bit arch and max size of metalen is 32 as xdp_metalen_invalid() suggest, is there any other reason why we need 256 bytes here?

XDP_PACKET_HEADROOM must be greater than (40 + 32)B because you may want push new data at the beginning of the xdp_buffer/xdp_frame running bpf_xdp_adjust_head() helper.
I think 256B has been selected for XDP_PACKET_HEADROOM since it is 4 cachelines (but I can be wrong).
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.

Regards,
Lorenzo

Quoted 13 lines

Quoted 12 lines
- max_headsize = SKB_WITH_OVERHEAD(VETH_BUF_SIZE - VETH_XDP_HEADROOM) = 1470
Even in this case we will need the consume a full page. In fact, performances are a little bit worse:
MTU 1500: tcp throughput ~ 8.3Gbps
Do you agree or am I missing something?
Regards,
Lorenzo

Yunsheng Lin

2023-04-24 11:58 UTC #6 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

Quoted 33 lines · Lorenzo Bianconi

Quoted 26 lines · Lorenzo Bianconi
Quoted 18 lines
Quoted 11 lines · Lorenzo Bianconi
Quoted 10 lines
struct veth_priv {
drivers/net/veth.c +1 -1
@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;
 
Quoted 6 lines
drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
drivers/net/veth.c +0 -0
 		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.
Quoted 6 lines
please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:
- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258
Quoted 6 lines
On some arch the NET_IP_ALIGN is zero.
I suppose XDP_PACKET_HEADROOM are for xdp_frame and data_meta, it seems xdp_frame is only 40 bytes for 64 bit arch and max size of metalen is 32 as xdp_metalen_invalid() suggest, is there any other reason why we need 256 bytes here?

Quoted 8 lines

XDP_PACKET_HEADROOM must be greater than (40 + 32)B because you may want push new data at the beginning of the xdp_buffer/xdp_frame running bpf_xdp_adjust_head() helper.
I think 256B has been selected for XDP_PACKET_HEADROOM since it is 4 cachelines (but I can be wrong).
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.

It worth mentioning that the performance gain in this patch is at the cost of more memory usage, at most of VETH_RING_SIZE(256) + PP_ALLOC_CACHE_SIZE(128) pages is used.

IMHO, it seems better to limit the memory usage as much as possible, or provide a way to disable/enable page pool for user.

Jesper Dangaard Brouer

2023-04-24 13:05 UTC #7 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

Quoted 43 lines · Yunsheng Lin

Quoted 33 lines · Lorenzo Bianconi
Quoted 26 lines · Lorenzo Bianconi
Quoted 18 lines
Quoted 11 lines · Lorenzo Bianconi
Quoted 10 lines
  struct veth_priv {
drivers/net/veth.c +1 -1
@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
  			goto drop;
 
  		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
  		if (!page)
  			goto drop;
 
Quoted 6 lines
drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
drivers/net/veth.c +0 -0
  		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.
Quoted 6 lines
please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:
- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258
Quoted 6 lines
On some arch the NET_IP_ALIGN is zero.
I suppose XDP_PACKET_HEADROOM are for xdp_frame and data_meta, it seems xdp_frame is only 40 bytes for 64 bit arch and max size of metalen is 32 as xdp_metalen_invalid() suggest, is there any other reason why we need 256 bytes here?
Quoted 8 lines
XDP_PACKET_HEADROOM must be greater than (40 + 32)B because you may want push new data at the beginning of the xdp_buffer/xdp_frame running bpf_xdp_adjust_head() helper.
I think 256B has been selected for XDP_PACKET_HEADROOM since it is 4 cachelines (but I can be wrong).
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.

Quoted 4 lines

It worth mentioning that the performance gain in this patch is at the cost of more memory usage, at most of VETH_RING_SIZE(256) + PP_ALLOC_CACHE_SIZE(128) pages is used.

The general scheme with XDP is trading memory for speed up.

Quoted 3 lines

IMHO, it seems better to limit the memory usage as much as possible, or provide a way to disable/enable page pool for user.

Well, that sort of it exists right... If you disable XDP, or actually NAPI (looking at patches), it will also disable the page pool.

I want to high-light that Lorenzo is "just" replacing allocating a full page via alloc_page() to a faster api, that happens to cache some of these pages.
In that sense, I think this patch makes sense ... isolated seen.

My concern beyond this patch is that netif_receive_generic_xdp() and veth_convert_skb_to_xdp_buff() are both dealing with SKB-to-XDP conversion, but they are diverting in how they do this. (Is the challenge that veth will also see "TX" SKBs?)

Kind changing the direction, but I'm thinking why the beep are we allocating+copying the entire contents of the SKB.
There must be a better way? (especially after XDP got frags support).

--Jesper

Lorenzo Bianconi

2023-04-24 13:07 UTC #8 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

In reply to Yunsheng Lin

Quoted 43 lines · Lorenzo Bianconi

Quoted 33 lines
Quoted 26 lines · Lorenzo Bianconi
Quoted 18 lines
Quoted 11 lines · Lorenzo Bianconi
Quoted 10 lines
struct veth_priv {
drivers/net/veth.c +1 -1
@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;
 
Quoted 6 lines
drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
drivers/net/veth.c +0 -0
 		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.
Quoted 6 lines
please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:
- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258
Quoted 6 lines
On some arch the NET_IP_ALIGN is zero.
I suppose XDP_PACKET_HEADROOM are for xdp_frame and data_meta, it seems xdp_frame is only 40 bytes for 64 bit arch and max size of metalen is 32 as xdp_metalen_invalid() suggest, is there any other reason why we need 256 bytes here?
Quoted 8 lines
XDP_PACKET_HEADROOM must be greater than (40 + 32)B because you may want push new data at the beginning of the xdp_buffer/xdp_frame running bpf_xdp_adjust_head() helper.
I think 256B has been selected for XDP_PACKET_HEADROOM since it is 4 cachelines (but I can be wrong).
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.

Quoted 3 lines

It worth mentioning that the performance gain in this patch is at the cost of more memory usage, at most of VETH_RING_SIZE(256) + PP_ALLOC_CACHE_SIZE(128) pages is used.

I would say the memory footprint is not so significative compared to the performance improvement (>= 15%) in this particular case. In particular I think in most of the cases we will recycle into ptr_ring:
- 4K pages: 256*4KB ~ 1MB
- 64K pages: 256*64KB ~ 16MB

Regards,
Lorenzo

Quoted 4 lines

IMHO, it seems better to limit the memory usage as much as possible, or provide a way to disable/enable page pool for user.

Maciej Fijalkowski

2023-04-24 13:11 UTC #9 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

In reply to Yunsheng Lin

Quoted 34 lines · Yunsheng Lin

Quoted 33 lines · Lorenzo Bianconi
Quoted 32 lines · Lorenzo Bianconi
Quoted 18 lines
Quoted 11 lines · Lorenzo Bianconi
Quoted 10 lines
struct veth_priv {
drivers/net/veth.c +1 -1
@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;
 
Quoted 6 lines
drivers/net/veth.c static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
drivers/net/veth.c +0 -0
 		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.
Quoted 6 lines
please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:
- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258
On some arch the NET_IP_ALIGN is zero.
I suppose XDP_PACKET_HEADROOM are for xdp_frame and data_meta, it seems xdp_frame is only 40 bytes for 64 bit arch and max size of metalen is 32 as xdp_metalen_invalid() suggest, is there any other reason why we need 256 bytes here?

Quoted 8 lines

Quoted 8 lines
XDP_PACKET_HEADROOM must be greater than (40 + 32)B because you may want push new data at the beginning of the xdp_buffer/xdp_frame running bpf_xdp_adjust_head() helper.
I think 256B has been selected for XDP_PACKET_HEADROOM since it is 4 cachelines (but I can be wrong).
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.

Intel drivers still work just fine at 192 headroom and split the page but it makes it problematic for BIG TCP where MAX_SKB_FRAGS from shinfo needs to be increased. So it's the tailroom that becomes the bottleneck, not the headroom. I believe at some point we will convert our drivers to page_pool with full 4k page dedicated for a single frame.

Quoted 7 lines

It worth mentioning that the performance gain in this patch is at the cost of more memory usage, at most of VETH_RING_SIZE(256) + PP_ALLOC_CACHE_SIZE(128) pages is used.
IMHO, it seems better to limit the memory usage as much as possible, or provide a way to disable/enable page pool for user.

I think that this argument is valuable due to the purpose that veth can serve, IMHO it wouldn't be an argument for a standard PF driver. It would be interesting to see how veth would work with PP_FLAG_PAGE_FRAG.

Lorenzo Bianconi

2023-04-24 13:41 UTC #10 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

Quoted 55 lines · Yunsheng Lin

Quoted 34 lines · Lorenzo Bianconi
Quoted 33 lines
Quoted 32 lines · Lorenzo Bianconi
Quoted 18 lines
Quoted 11 lines · Lorenzo Bianconi
Quoted 10 lines
struct veth_priv {
(quoted diff) +1 -1
@@ -727,17 +729,20 @@ static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq,
 			goto drop;
 
 		/* Allocate skb head */
-		page = alloc_page(GFP_ATOMIC | __GFP_NOWARN);
+		page = page_pool_dev_alloc_pages(rq->page_pool);
 		if (!page)
 			goto drop;
 
Quoted 6 lines
Review static int veth_convert_skb_to_xdp_buff(struct veth_rq *rq, · line 736
Quoted 1 line
(quoted diff) +0 -0
 		nskb = build_skb(page_address(page), PAGE_SIZE);
If page pool is used with PP_FLAG_PAGE_FRAG, maybe there is some additional improvement for the MTU 1500B case, it seem a 4K page is able to hold two skb. And we can reduce the memory usage too, which is a significant saving if page size is 64K.
Quoted 6 lines
please correct if I am wrong but I think the 1500B MTU case does not fit in the half-page buffer size since we need to take into account VETH_XDP_HEADROOM. In particular:
- VETH_BUF_SIZE = 2048
- VETH_XDP_HEADROOM = 256 + 2 = 258
On some arch the NET_IP_ALIGN is zero.
I suppose XDP_PACKET_HEADROOM are for xdp_frame and data_meta, it seems xdp_frame is only 40 bytes for 64 bit arch and max size of metalen is 32 as xdp_metalen_invalid() suggest, is there any other reason why we need 256 bytes here?
Quoted 8 lines
Quoted 8 lines
XDP_PACKET_HEADROOM must be greater than (40 + 32)B because you may want push new data at the beginning of the xdp_buffer/xdp_frame running bpf_xdp_adjust_head() helper.
I think 256B has been selected for XDP_PACKET_HEADROOM since it is 4 cachelines (but I can be wrong).
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.
Intel drivers still work just fine at 192 headroom and split the page but it makes it problematic for BIG TCP where MAX_SKB_FRAGS from shinfo needs to be increased. So it's the tailroom that becomes the bottleneck, not the headroom. I believe at some point we will convert our drivers to page_pool with full 4k page dedicated for a single frame.
Quoted 5 lines
It worth mentioning that the performance gain in this patch is at the cost of more memory usage, at most of VETH_RING_SIZE(256) + PP_ALLOC_CACHE_SIZE(128) pages is used.

Quoted 6 lines

Quoted 2 lines
IMHO, it seems better to limit the memory usage as much as possible, or provide a way to disable/enable page pool for user.
I think that this argument is valuable due to the purpose that veth can serve, IMHO it wouldn't be an argument for a standard PF driver. It would be interesting to see how veth would work with PP_FLAG_PAGE_FRAG.

actually I already have a PoC for using page_pool with PP_FLAG_PAGE_FRAG flag in veth driver but if we do not reduce XDP_PACKET_HEADROOM size there will not be any difference in the memory footprint since we will need two fragments (so a full-page) for a 1500B frame. Moreover, we will have lower performance since we will need to spread the data on two skb buffers (linear area and frag0 in this case).

Regards,
Lorenzo

Quoted 1 line

Yunsheng Lin

2023-04-25 11:19 UTC #11 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

In reply to Maciej Fijalkowski

Quoted 6 lines · Maciej Fijalkowski

Quoted 3 lines
Quoted 3 lines
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.
Intel drivers still work just fine at 192 headroom and split the page but it makes it problematic for BIG TCP where MAX_SKB_FRAGS from shinfo needs

I am not sure why we are not enabling skb_shinfo(skb)->frag_list to support BIG TCP instead of increasing MAX_SKB_FRAGS, perhaps there was some disscution about this in the past I am not aware of?

Quoted 3 lines

to be increased. So it's the tailroom that becomes the bottleneck, not the headroom. I believe at some point we will convert our drivers to page_pool with full 4k page dedicated for a single frame.

Can we use header splitting to ensure there is enough tailroom for napi_build_skb() or xdp_frame with shinfo?

Lorenzo Bianconi

2023-04-25 14:19 UTC #12 lore raw

Re: [PATCH v2 net-next 1/2] net: veth: add page_pool for page recycling

Quoted 12 lines · Maciej Fijalkowski

Quoted 6 lines
Quoted 3 lines
Quoted 3 lines
There was a discussion in the past to reduce XDP_PACKET_HEADROOM to 192B but this is not merged yet and it is not related to this series. We can address your comments in a follow-up patch when XDP_PACKET_HEADROOM series is merged.
Intel drivers still work just fine at 192 headroom and split the page but it makes it problematic for BIG TCP where MAX_SKB_FRAGS from shinfo needs
I am not sure why we are not enabling skb_shinfo(skb)->frag_list to support BIG TCP instead of increasing MAX_SKB_FRAGS, perhaps there was some disscution about this in the past I am not aware of?

Quoted 7 lines

Quoted 3 lines
to be increased. So it's the tailroom that becomes the bottleneck, not the headroom. I believe at some point we will convert our drivers to page_pool with full 4k page dedicated for a single frame.
Can we use header splitting to ensure there is enough tailroom for napi_build_skb() or xdp_frame with shinfo?

since veth_convert_skb_to_xdp_buff() runs in veth_poll() I think we can use napi_build_skb(). I tested it and we get an improvement (9.65Gbps vs 9.2Gbps for 1500B frames).

Regards,
Lorenzo

Lorenzo Bianconi

author

2023-04-22 18:54 UTC #13 lore raw

PATCH v2 2/2 net: veth: add page_pool stats

In reply to Lorenzo Bianconi

Introduce page_pool stats support to report info about local page_pool through ethtool

Tested-by Maryam Tahhan <[email protected]>

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

---
 drivers/net/Kconfig |  1 +
 drivers/net/veth.c  | 20 +++++++++++++++++---
 2 files changed, 18 insertions(+), 3 deletions(-)

drivers/net/Kconfig +1 -0

index 368c6f5b327e..d0a1ed216d15 100644
@@ -403,6 +403,7 @@ config TUN_VNET_CROSS_LE
 config VETH
 	tristate "Virtual ethernet pair device"
 	select PAGE_POOL
+	select PAGE_POOL_STATS
 	help
 	  This device is a local ethernet tunnel. Devices are created in pairs.
 	  When one end receives the packet it appears on its pair and vice

drivers/net/veth.c +17 -3

index 35d2285dec25..dce9f9d63e04 100644
@@ -157,6 +157,8 @@ static void veth_get_strings(struct net_device *dev, u32 stringset, u8 *buf)
 			for (j = 0; j < VETH_TQ_STATS_LEN; j++)
 				ethtool_sprintf(&p, "tx_queue_%u_%.18s",
 						i, veth_tq_stats_desc[j].desc);
+
+		page_pool_ethtool_stats_get_strings(p);
 		break;
 	}
 }

@@ -167,7 +169,8 @@ static int veth_get_sset_count(struct net_device *dev, int sset)
 	case ETH_SS_STATS:
 		return ARRAY_SIZE(ethtool_stats_keys) +
 		       VETH_RQ_STATS_LEN * dev->real_num_rx_queues +
-		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues;
+		       VETH_TQ_STATS_LEN * dev->real_num_tx_queues +
+		       page_pool_ethtool_stats_get_count();
 	default:
 		return -EOPNOTSUPP;
 	}

@@ -178,7 +181,8 @@ static void veth_get_ethtool_stats(struct net_device *dev,
 {
 	struct veth_priv *rcv_priv, *priv = netdev_priv(dev);
 	struct net_device *peer = rtnl_dereference(priv->peer);
-	int i, j, idx;
+	struct page_pool_stats pp_stats = {};
+	int i, j, idx, pp_idx;
 
 	data[0] = peer ? peer->ifindex : 0;
 	idx = 1;

@@ -197,9 +201,10 @@ static void veth_get_ethtool_stats(struct net_device *dev,
 		} while (u64_stats_fetch_retry(&rq_stats->syncp, start));
 		idx += VETH_RQ_STATS_LEN;
 	}
+	pp_idx = idx;
 
 	if (!peer)
-		return;
+		goto page_pool_stats;
 
 	rcv_priv = netdev_priv(peer);
 	for (i = 0; i < peer->real_num_rx_queues; i++) {

@@ -216,7 +221,16 @@ static void veth_get_ethtool_stats(struct net_device *dev,
 				data[tx_idx + j] += *(u64 *)(base + offset);
 			}
 		} while (u64_stats_fetch_retry(&rq_stats->syncp, start));
+		pp_idx = tx_idx + VETH_TQ_STATS_LEN;
+	}
+
+page_pool_stats:
+	for (i = 0; i < dev->real_num_rx_queues; i++) {
+		if (!priv->rq[i].page_pool)
+			continue;
+		page_pool_get_stats(priv->rq[i].page_pool, &pp_stats);
 	}
+	page_pool_ethtool_stats_get(&data[pp_idx], &pp_stats);
 }
 
 static void veth_get_channels(struct net_device *dev,

Signature · 1 line

2.40.0

[email protected]

CI Applied

2023-04-25 01:10 UTC #14 lore raw

In reply to Lorenzo Bianconi

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 5 lines · On Sat, 22 Apr 2023 20:54:31 +0200 you wrote:

Introduce page_pool support in veth driver in order to recycle pages in veth_convert_skb_to_xdp_buff routine and avoid reallocating the skb through the page allocator when we run a xdp program on the device and we receive skbs from the stack.

Quoted 7 lines

Change since v1:
- remove page_pool checks in veth_convert_skb_to_xdp_buff() before allocating the pages
- recycle pages in the hot cache if build_skb fails in veth_convert_skb_to_xdp_buff()
[...]

Here is the summary with links:
- [v2,net-next,1/2] net: veth: add page_pool for page recycling
https://git.kernel.org/netdev/net-next/c/0ebab78cbcbf
- [v2,net-next,2/2] net: veth: add page_pool stats

    https://git.kernel.org/netdev/net-next/c/4fc418053ec7

You are awesome, thank you!

Signature · 2 lines

Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html