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

Commit Message

Lorenzo Bianconi April 22, 2023, 6:54 p.m. UTC

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 <[e-mail]>
Signed-off-by: Lorenzo Bianconi <[e-mail]>
---
 drivers/net/Kconfig |  1 +
 drivers/net/veth.c  | 48 +++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 45 insertions(+), 4 deletions(-)

Comments

Yunsheng Lin April 23, 2023, 12:17 p.m. UTC | #1

On 2023/4/23 2:54, Lorenzo Bianconi wrote:
>  struct veth_priv {
> @@ -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.


>  		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;
> @@ -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 April 23, 2023, 2:20 p.m. UTC | #2

> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
> >  struct veth_priv {
> > @@ -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.

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

> 
> 
> >  		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;
> > @@ -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 April 24, 2023, 2:29 a.m. UTC | #3

On 2023/4/23 22:20, Lorenzo Bianconi wrote:
>> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
>>>  struct veth_priv {
>>> @@ -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.
> 
> 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?

> - 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 April 24, 2023, 9:17 a.m. UTC | #4

> On 2023/4/23 22:20, Lorenzo Bianconi wrote:
> >> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
> >>>  struct veth_priv {
> >>> @@ -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.
> > 
> > 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?

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

> 
> > - 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 April 24, 2023, 11:58 a.m. UTC | #5

On 2023/4/24 17:17, Lorenzo Bianconi wrote:
>> On 2023/4/23 22:20, Lorenzo Bianconi wrote:
>>>> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
>>>>>  struct veth_priv {
>>>>> @@ -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.
>>>
>>> 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?
> 
> 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 April 24, 2023, 1:04 p.m. UTC | #6

On 24/04/2023 13.58, Yunsheng Lin wrote:
> On 2023/4/24 17:17, Lorenzo Bianconi wrote:
>>> On 2023/4/23 22:20, Lorenzo Bianconi wrote:
>>>>> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
>>>>>>   struct veth_priv {
>>>>>> @@ -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.
>>>>
>>>> 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?
>>
>> 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.
> 

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

> 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 April 24, 2023, 1:06 p.m. UTC | #7

> On 2023/4/24 17:17, Lorenzo Bianconi wrote:
> >> On 2023/4/23 22:20, Lorenzo Bianconi wrote:
> >>>> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
> >>>>>  struct veth_priv {
> >>>>> @@ -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.
> >>>
> >>> 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?
> > 
> > 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.

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

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

Fijalkowski, Maciej April 24, 2023, 1:10 p.m. UTC | #8

On Mon, Apr 24, 2023 at 07:58:07PM +0800, Yunsheng Lin wrote:
> On 2023/4/24 17:17, Lorenzo Bianconi wrote:
> >> On 2023/4/23 22:20, Lorenzo Bianconi wrote:
> >>>> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
> >>>>>  struct veth_priv {
> >>>>> @@ -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.
> >>>
> >>> 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?
> > 
> > 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.

> 
> 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 April 24, 2023, 1:41 p.m. UTC | #9

> On Mon, Apr 24, 2023 at 07:58:07PM +0800, Yunsheng Lin wrote:
> > On 2023/4/24 17:17, Lorenzo Bianconi wrote:
> > >> On 2023/4/23 22:20, Lorenzo Bianconi wrote:
> > >>>> On 2023/4/23 2:54, Lorenzo Bianconi wrote:
> > >>>>>  struct veth_priv {
> > >>>>> @@ -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.
> > >>>
> > >>> 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?
> > > 
> > > 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.
> 
> > 
> > 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.

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

>

Yunsheng Lin April 25, 2023, 11:19 a.m. UTC | #10

On 2023/4/24 21:10, Maciej Fijalkowski wrote:
>>> 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?

> 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 April 25, 2023, 2:13 p.m. UTC | #11

> On 2023/4/24 21:10, Maciej Fijalkowski wrote:
> >>> 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?
> 
> > 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

Patch

diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index c34bd432da27..368c6f5b327e 100644
--- a/drivers/net/Kconfig
+++ b/drivers/net/Kconfig
@@ -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
diff --git a/drivers/net/veth.c b/drivers/net/veth.c
index 4b3c6647edc6..35d2285dec25 100644
--- a/drivers/net/veth.c
+++ b/drivers/net/veth.c
@@ -26,6 +26,7 @@ 
 #include <linux/ptr_ring.h>
 #include <linux/bpf_trace.h>
 #include <linux/net_tstamp.h>
+#include <net/page_pool.h>
 
 #define DRV_NAME	"veth"
 #define DRV_VERSION	"1.0"
@@ -65,6 +66,7 @@  struct veth_rq {
 	bool			rx_notify_masked;
 	struct ptr_ring		xdp_ring;
 	struct xdp_rxq_info	xdp_rxq;
+	struct page_pool	*page_pool;
 };
 
 struct veth_priv {
@@ -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 (!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;
@@ -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,
+		.nid = NUMA_NO_NODE,
+		.dev = &rq->dev->dev,
+	};
+
+	rq->page_pool = page_pool_create(&pp_params);
+	if (IS_ERR(rq->page_pool)) {
+		int err = PTR_ERR(rq->page_pool);
+
+		rq->page_pool = NULL;
+		return err;
+	}
+
+	return 0;
+}
+
 static int __veth_napi_enable_range(struct net_device *dev, int start, int end)
 {
 	struct veth_priv *priv = netdev_priv(dev);
 	int err, i;
 
+	for (i = start; i < end; i++) {
+		err = veth_create_page_pool(&priv->rq[i]);
+		if (err)
+			goto err_page_pool;
+	}
+
 	for (i = start; i < end; i++) {
 		struct veth_rq *rq = &priv->rq[i];
 
@@ -1027,6 +1057,11 @@  static int __veth_napi_enable_range(struct net_device *dev, int start, int end)
 err_xdp_ring:
 	for (i--; i >= start; i--)
 		ptr_ring_cleanup(&priv->rq[i].xdp_ring, veth_ptr_free);
+err_page_pool:
+	for (i = start; i < end; i++) {
+		page_pool_destroy(priv->rq[i].page_pool);
+		priv->rq[i].page_pool = NULL;
+	}
 
 	return err;
 }
@@ -1056,6 +1091,11 @@  static void veth_napi_del_range(struct net_device *dev, int start, int end)
 		rq->rx_notify_masked = false;
 		ptr_ring_cleanup(&rq->xdp_ring, veth_ptr_free);
 	}
+
+	for (i = start; i < end; i++) {
+		page_pool_destroy(priv->rq[i].page_pool);
+		priv->rq[i].page_pool = NULL;
+	}
 }
 
 static void veth_napi_del(struct net_device *dev)