bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH bpf-next 0/2] multi-buffer support for XDP_REDIRECT samples
@ 2023-05-29 11:06 Tariq Toukan
  2023-05-29 11:06 ` [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer Tariq Toukan
  2023-05-29 11:06 ` [PATCH bpf-next 2/2] samples/bpf: fixup xdp_redirect_map " Tariq Toukan
  0 siblings, 2 replies; 10+ messages in thread
From: Tariq Toukan @ 2023-05-29 11:06 UTC (permalink / raw)
  To: Alexei Starovoitov, John Fastabend, Jakub Kicinski
  Cc: Daniel Borkmann, Jesper Dangaard Brouer, bpf, David S. Miller,
	Eric Dumazet, netdev, Gal Pressman, Tariq Toukan

Hi,

This series adds multi-buffer support for two XDP_REDIRECT sample programs.
It follows the pattern from xdp1 and xdp2.

Series generated against bpf-next commit:
4266f41feaee bpf: Fix bad unlock balance on freeze_mutex

Regards,
Tariq

Nimrod Oren (1):
  samples/bpf: fixup xdp_redirect_map tool to be able to support xdp
    multibuffer

Tariq Toukan (1):
  samples/bpf: fixup xdp_redirect tool to be able to support xdp
    multibuffer

 samples/bpf/xdp_redirect.bpf.c     | 16 +++++++++++----
 samples/bpf/xdp_redirect_map.bpf.c | 31 ++++++++++++++++++++++--------
 2 files changed, 35 insertions(+), 12 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-29 11:06 [PATCH bpf-next 0/2] multi-buffer support for XDP_REDIRECT samples Tariq Toukan
@ 2023-05-29 11:06 ` Tariq Toukan
  2023-05-30 11:33   ` Jesper Dangaard Brouer
  2023-05-29 11:06 ` [PATCH bpf-next 2/2] samples/bpf: fixup xdp_redirect_map " Tariq Toukan
  1 sibling, 1 reply; 10+ messages in thread
From: Tariq Toukan @ 2023-05-29 11:06 UTC (permalink / raw)
  To: Alexei Starovoitov, John Fastabend, Jakub Kicinski
  Cc: Daniel Borkmann, Jesper Dangaard Brouer, bpf, David S. Miller,
	Eric Dumazet, netdev, Gal Pressman, Tariq Toukan, Nimrod Oren

Expand the xdp multi-buffer support to xdp_redirect tool.
Similar to what's done in commit
772251742262 ("samples/bpf: fixup some tools to be able to support xdp multibuffer")
and its fix commit
7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern").

Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
Reviewed-by: Nimrod Oren <noren@nvidia.com>
---
 samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/samples/bpf/xdp_redirect.bpf.c b/samples/bpf/xdp_redirect.bpf.c
index 7c02bacfe96b..620163eb7e19 100644
--- a/samples/bpf/xdp_redirect.bpf.c
+++ b/samples/bpf/xdp_redirect.bpf.c
@@ -16,16 +16,21 @@
 
 const volatile int ifindex_out;
 
-SEC("xdp")
+#define XDPBUFSIZE	64
+SEC("xdp.frags")
 int xdp_redirect_prog(struct xdp_md *ctx)
 {
-	void *data_end = (void *)(long)ctx->data_end;
-	void *data = (void *)(long)ctx->data;
+	__u8 pkt[XDPBUFSIZE] = {};
+	void *data_end = &pkt[XDPBUFSIZE-1];
+	void *data = pkt;
 	u32 key = bpf_get_smp_processor_id();
 	struct ethhdr *eth = data;
 	struct datarec *rec;
 	u64 nh_off;
 
+	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return XDP_DROP;
+
 	nh_off = sizeof(*eth);
 	if (data + nh_off > data_end)
 		return XDP_DROP;
@@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx)
 	NO_TEAR_INC(rec->processed);
 
 	swap_src_dst_mac(data);
+	if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return XDP_DROP;
+
 	return bpf_redirect(ifindex_out, 0);
 }
 
 /* Redirect require an XDP bpf_prog loaded on the TX device */
-SEC("xdp")
+SEC("xdp.frags")
 int xdp_redirect_dummy_prog(struct xdp_md *ctx)
 {
 	return XDP_PASS;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH bpf-next 2/2] samples/bpf: fixup xdp_redirect_map tool to be able to support xdp multibuffer
  2023-05-29 11:06 [PATCH bpf-next 0/2] multi-buffer support for XDP_REDIRECT samples Tariq Toukan
  2023-05-29 11:06 ` [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer Tariq Toukan
@ 2023-05-29 11:06 ` Tariq Toukan
  1 sibling, 0 replies; 10+ messages in thread
From: Tariq Toukan @ 2023-05-29 11:06 UTC (permalink / raw)
  To: Alexei Starovoitov, John Fastabend, Jakub Kicinski
  Cc: Daniel Borkmann, Jesper Dangaard Brouer, bpf, David S. Miller,
	Eric Dumazet, netdev, Gal Pressman, Nimrod Oren, Tariq Toukan

From: Nimrod Oren <noren@nvidia.com>

Expand the xdp multi-buffer support to xdp_redirect_map tool.
Similar to what's done in commit
772251742262 ("samples/bpf: fixup some tools to be able to support xdp multibuffer")
and its fix commit
7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern").

Signed-off-by: Nimrod Oren <noren@nvidia.com>
Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
---
 samples/bpf/xdp_redirect_map.bpf.c | 31 ++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/samples/bpf/xdp_redirect_map.bpf.c b/samples/bpf/xdp_redirect_map.bpf.c
index 8557c278df77..dd034fdff1a9 100644
--- a/samples/bpf/xdp_redirect_map.bpf.c
+++ b/samples/bpf/xdp_redirect_map.bpf.c
@@ -35,15 +35,20 @@ struct {
 /* store egress interface mac address */
 const volatile __u8 tx_mac_addr[ETH_ALEN];
 
+#define XDPBUFSIZE	64
 static __always_inline int xdp_redirect_map(struct xdp_md *ctx, void *redirect_map)
 {
-	void *data_end = (void *)(long)ctx->data_end;
-	void *data = (void *)(long)ctx->data;
+	__u8 pkt[XDPBUFSIZE] = {};
+	void *data_end = &pkt[XDPBUFSIZE-1];
+	void *data = pkt;
 	u32 key = bpf_get_smp_processor_id();
 	struct ethhdr *eth = data;
 	struct datarec *rec;
 	u64 nh_off;
 
+	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return XDP_DROP;
+
 	nh_off = sizeof(*eth);
 	if (data + nh_off > data_end)
 		return XDP_DROP;
@@ -53,30 +58,37 @@ static __always_inline int xdp_redirect_map(struct xdp_md *ctx, void *redirect_m
 		return XDP_PASS;
 	NO_TEAR_INC(rec->processed);
 	swap_src_dst_mac(data);
+	if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return XDP_DROP;
+
 	return bpf_redirect_map(redirect_map, 0, 0);
 }
 
-SEC("xdp")
+SEC("xdp.frags")
 int xdp_redirect_map_general(struct xdp_md *ctx)
 {
 	return xdp_redirect_map(ctx, &tx_port_general);
 }
 
-SEC("xdp")
+SEC("xdp.frags")
 int xdp_redirect_map_native(struct xdp_md *ctx)
 {
 	return xdp_redirect_map(ctx, &tx_port_native);
 }
 
-SEC("xdp/devmap")
+SEC("xdp.frags/devmap")
 int xdp_redirect_map_egress(struct xdp_md *ctx)
 {
-	void *data_end = (void *)(long)ctx->data_end;
-	void *data = (void *)(long)ctx->data;
+	__u8 pkt[XDPBUFSIZE] = {};
+	void *data_end = &pkt[XDPBUFSIZE-1];
+	void *data = pkt;
 	u8 *mac_addr = (u8 *) tx_mac_addr;
 	struct ethhdr *eth = data;
 	u64 nh_off;
 
+	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return XDP_DROP;
+
 	nh_off = sizeof(*eth);
 	if (data + nh_off > data_end)
 		return XDP_DROP;
@@ -84,11 +96,14 @@ int xdp_redirect_map_egress(struct xdp_md *ctx)
 	barrier_var(mac_addr); /* prevent optimizing out memcpy */
 	__builtin_memcpy(eth->h_source, mac_addr, ETH_ALEN);
 
+	if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt)))
+		return XDP_DROP;
+
 	return XDP_PASS;
 }
 
 /* Redirect require an XDP bpf_prog loaded on the TX device */
-SEC("xdp")
+SEC("xdp.frags")
 int xdp_redirect_dummy_prog(struct xdp_md *ctx)
 {
 	return XDP_PASS;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-29 11:06 ` [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer Tariq Toukan
@ 2023-05-30 11:33   ` Jesper Dangaard Brouer
  2023-05-30 12:17     ` Tariq Toukan
  0 siblings, 1 reply; 10+ messages in thread
From: Jesper Dangaard Brouer @ 2023-05-30 11:33 UTC (permalink / raw)
  To: Tariq Toukan, Alexei Starovoitov, John Fastabend, Jakub Kicinski
  Cc: brouer, Daniel Borkmann, Jesper Dangaard Brouer, bpf,
	David S. Miller, Eric Dumazet, netdev, Gal Pressman, Nimrod Oren



On 29/05/2023 13.06, Tariq Toukan wrote:
> Expand the xdp multi-buffer support to xdp_redirect tool.
> Similar to what's done in commit
> 772251742262 ("samples/bpf: fixup some tools to be able to support xdp multibuffer")
> and its fix commit
> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern").
> 

Have you tested if this cause a performance degradation?

(Also found possible bug below)

> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
> Reviewed-by: Nimrod Oren <noren@nvidia.com>
> ---
>   samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++----
>   1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/samples/bpf/xdp_redirect.bpf.c b/samples/bpf/xdp_redirect.bpf.c
> index 7c02bacfe96b..620163eb7e19 100644
> --- a/samples/bpf/xdp_redirect.bpf.c
> +++ b/samples/bpf/xdp_redirect.bpf.c
> @@ -16,16 +16,21 @@
>   
>   const volatile int ifindex_out;
>   
> -SEC("xdp")
> +#define XDPBUFSIZE	64

Pktgen sample scripts will default send with 60 pkt length, because the
4 bytes FCS (end-frame checksum) is added by hardware.

Will this result in an error when bpf_xdp_load_bytes() tries to copy 64
bytes from a 60 bytes packet?

> +SEC("xdp.frags")
>   int xdp_redirect_prog(struct xdp_md *ctx)
>   {
> -	void *data_end = (void *)(long)ctx->data_end;
> -	void *data = (void *)(long)ctx->data;
> +	__u8 pkt[XDPBUFSIZE] = {};
> +	void *data_end = &pkt[XDPBUFSIZE-1];
> +	void *data = pkt;
>   	u32 key = bpf_get_smp_processor_id();
>   	struct ethhdr *eth = data;
>   	struct datarec *rec;
>   	u64 nh_off;
>   
> +	if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
> +		return XDP_DROP;

E.g. sizeof(pkt) = 64 bytes here.

> +
>   	nh_off = sizeof(*eth);
>   	if (data + nh_off > data_end)
>   		return XDP_DROP;
> @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx)
>   	NO_TEAR_INC(rec->processed);
>   
>   	swap_src_dst_mac(data);
> +	if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt)))
> +		return XDP_DROP;
> +
>   	return bpf_redirect(ifindex_out, 0);
>   }
>   
>   /* Redirect require an XDP bpf_prog loaded on the TX device */
> -SEC("xdp")
> +SEC("xdp.frags")
>   int xdp_redirect_dummy_prog(struct xdp_md *ctx)
>   {
>   	return XDP_PASS;


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-30 11:33   ` Jesper Dangaard Brouer
@ 2023-05-30 12:17     ` Tariq Toukan
  2023-05-30 12:40       ` Jesper Dangaard Brouer
  0 siblings, 1 reply; 10+ messages in thread
From: Tariq Toukan @ 2023-05-30 12:17 UTC (permalink / raw)
  To: Jesper Dangaard Brouer, Alexei Starovoitov, John Fastabend,
	Jakub Kicinski
  Cc: brouer, Daniel Borkmann, Jesper Dangaard Brouer, bpf,
	David S. Miller, Eric Dumazet, netdev, Gal Pressman, Nimrod Oren,
	Tariq Toukan



On 30/05/2023 14:33, Jesper Dangaard Brouer wrote:
> 
> 
> On 29/05/2023 13.06, Tariq Toukan wrote:
>> Expand the xdp multi-buffer support to xdp_redirect tool.
>> Similar to what's done in commit
>> 772251742262 ("samples/bpf: fixup some tools to be able to support xdp 
>> multibuffer")
>> and its fix commit
>> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern").
>>
> 
> Have you tested if this cause a performance degradation?
> 
> (Also found possible bug below)
> 

Hi Jesper,

This introduces the same known perf degradation we already have in xdp1 
and xdp2.
Unfortunately, this is the API we have today to safely support multi-buffer.
Note that both perf and functional (noted below) degradation should be 
eliminated once replacing the load/store operations with dynptr logic 
that returns a pointer to the scatter entry instead of copying it.

I initiated a discussion on this topic a few months ago. dynptr was 
accepted since then, but I'm not aware of any in-progress followup work 
that addresses this.

>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>> Reviewed-by: Nimrod Oren <noren@nvidia.com>
>> ---
>>   samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++----
>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>
>> diff --git a/samples/bpf/xdp_redirect.bpf.c 
>> b/samples/bpf/xdp_redirect.bpf.c
>> index 7c02bacfe96b..620163eb7e19 100644
>> --- a/samples/bpf/xdp_redirect.bpf.c
>> +++ b/samples/bpf/xdp_redirect.bpf.c
>> @@ -16,16 +16,21 @@
>>   const volatile int ifindex_out;
>> -SEC("xdp")
>> +#define XDPBUFSIZE    64
> 
> Pktgen sample scripts will default send with 60 pkt length, because the
> 4 bytes FCS (end-frame checksum) is added by hardware.
> 
> Will this result in an error when bpf_xdp_load_bytes() tries to copy 64
> bytes from a 60 bytes packet?
> 

Yes.

This can be resolved by reducing XDPBUFSIZE to 60.
Need to check if it's OK to disregard these last 4 bytes without hurting 
the XDP program logic.

If so, do you suggest changing xdp1 and xdp2 as well?

>> +SEC("xdp.frags")
>>   int xdp_redirect_prog(struct xdp_md *ctx)
>>   {
>> -    void *data_end = (void *)(long)ctx->data_end;
>> -    void *data = (void *)(long)ctx->data;
>> +    __u8 pkt[XDPBUFSIZE] = {};
>> +    void *data_end = &pkt[XDPBUFSIZE-1];
>> +    void *data = pkt;
>>       u32 key = bpf_get_smp_processor_id();
>>       struct ethhdr *eth = data;
>>       struct datarec *rec;
>>       u64 nh_off;
>> +    if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
>> +        return XDP_DROP;
> 
> E.g. sizeof(pkt) = 64 bytes here.
> 
>> +
>>       nh_off = sizeof(*eth);
>>       if (data + nh_off > data_end)
>>           return XDP_DROP;
>> @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx)
>>       NO_TEAR_INC(rec->processed);
>>       swap_src_dst_mac(data);
>> +    if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt)))
>> +        return XDP_DROP;
>> +
>>       return bpf_redirect(ifindex_out, 0);
>>   }
>>   /* Redirect require an XDP bpf_prog loaded on the TX device */
>> -SEC("xdp")
>> +SEC("xdp.frags")
>>   int xdp_redirect_dummy_prog(struct xdp_md *ctx)
>>   {
>>       return XDP_PASS;
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-30 12:17     ` Tariq Toukan
@ 2023-05-30 12:40       ` Jesper Dangaard Brouer
  2023-05-30 13:40         ` Tariq Toukan
  0 siblings, 1 reply; 10+ messages in thread
From: Jesper Dangaard Brouer @ 2023-05-30 12:40 UTC (permalink / raw)
  To: Tariq Toukan, Jesper Dangaard Brouer, Alexei Starovoitov,
	John Fastabend, Jakub Kicinski
  Cc: brouer, Daniel Borkmann, Jesper Dangaard Brouer, bpf,
	David S. Miller, Eric Dumazet, netdev, Gal Pressman, Nimrod Oren,
	Tariq Toukan, Lorenzo Bianconi, drosen, Joanne Koong



On 30/05/2023 14.17, Tariq Toukan wrote:
> 
> On 30/05/2023 14:33, Jesper Dangaard Brouer wrote:
>>
>>
>> On 29/05/2023 13.06, Tariq Toukan wrote:
>>> Expand the xdp multi-buffer support to xdp_redirect tool.
>>> Similar to what's done in commit
>>> 772251742262 ("samples/bpf: fixup some tools to be able to support 
>>> xdp multibuffer")
>>> and its fix commit
>>> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern").
>>>
>>
>> Have you tested if this cause a performance degradation?
>>
>> (Also found possible bug below)
>>
> 
> Hi Jesper,
> 
> This introduces the same known perf degradation we already have in xdp1 
> and xdp2.

Did a quick test with xdp1, the performance degradation is around 18%.

  Before: 22,917,961 pps
  After:  18,798,336 pps

  (1-(18798336/22917961))*100 = 17.97%


> Unfortunately, this is the API we have today to safely support 
> multi-buffer.
> Note that both perf and functional (noted below) degradation should be 
> eliminated once replacing the load/store operations with dynptr logic 
> that returns a pointer to the scatter entry instead of copying it.
> 

Well, should we use dynptr logic in this patch then?

Does it make sense to add sample code that does thing in a way that is 
sub-optimal and we want to replace?
... (I fear people will copy paste the sample code).

> I initiated a discussion on this topic a few months ago. dynptr was 
> accepted since then, but I'm not aware of any in-progress followup work 
> that addresses this.
> 

Are you saying some more work is needed on dynptr?

>>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>>> Reviewed-by: Nimrod Oren <noren@nvidia.com>
>>> ---
>>>   samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++----
>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/samples/bpf/xdp_redirect.bpf.c 
>>> b/samples/bpf/xdp_redirect.bpf.c
>>> index 7c02bacfe96b..620163eb7e19 100644
>>> --- a/samples/bpf/xdp_redirect.bpf.c
>>> +++ b/samples/bpf/xdp_redirect.bpf.c
>>> @@ -16,16 +16,21 @@
>>>   const volatile int ifindex_out;
>>> -SEC("xdp")
>>> +#define XDPBUFSIZE    64
>>
>> Pktgen sample scripts will default send with 60 pkt length, because the
>> 4 bytes FCS (end-frame checksum) is added by hardware.
>>
>> Will this result in an error when bpf_xdp_load_bytes() tries to copy 64
>> bytes from a 60 bytes packet?
>>
> 
> Yes.
> 
> This can be resolved by reducing XDPBUFSIZE to 60.
> Need to check if it's OK to disregard these last 4 bytes without hurting 
> the XDP program logic.
> 
> If so, do you suggest changing xdp1 and xdp2 as well?
> 

I can take care of reducing XDPBUFSIZE to 60 on xpd1 and xdp2, as I
already had to make these changes for the above quick bench work ;-)
I'll send out patches shortly.


>>> +SEC("xdp.frags")
>>>   int xdp_redirect_prog(struct xdp_md *ctx)
>>>   {
>>> -    void *data_end = (void *)(long)ctx->data_end;
>>> -    void *data = (void *)(long)ctx->data;
>>> +    __u8 pkt[XDPBUFSIZE] = {};
>>> +    void *data_end = &pkt[XDPBUFSIZE-1];
>>> +    void *data = pkt;
>>>       u32 key = bpf_get_smp_processor_id();
>>>       struct ethhdr *eth = data;
>>>       struct datarec *rec;
>>>       u64 nh_off;
>>> +    if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
>>> +        return XDP_DROP;
>>
>> E.g. sizeof(pkt) = 64 bytes here.
>>
>>> +
>>>       nh_off = sizeof(*eth);
>>>       if (data + nh_off > data_end)
>>>           return XDP_DROP;
>>> @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx)
>>>       NO_TEAR_INC(rec->processed);
>>>       swap_src_dst_mac(data);
>>> +    if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt)))
>>> +        return XDP_DROP;
>>> +
>>>       return bpf_redirect(ifindex_out, 0);
>>>   }
>>>   /* Redirect require an XDP bpf_prog loaded on the TX device */
>>> -SEC("xdp")
>>> +SEC("xdp.frags")
>>>   int xdp_redirect_dummy_prog(struct xdp_md *ctx)
>>>   {
>>>       return XDP_PASS;
>>
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-30 12:40       ` Jesper Dangaard Brouer
@ 2023-05-30 13:40         ` Tariq Toukan
  2023-05-30 15:05           ` Toke Høiland-Jørgensen
  2023-05-30 17:22           ` Martin KaFai Lau
  0 siblings, 2 replies; 10+ messages in thread
From: Tariq Toukan @ 2023-05-30 13:40 UTC (permalink / raw)
  To: Jesper Dangaard Brouer, Alexei Starovoitov, John Fastabend,
	Jakub Kicinski
  Cc: brouer, Daniel Borkmann, Jesper Dangaard Brouer, bpf,
	David S. Miller, Eric Dumazet, netdev, Gal Pressman, Nimrod Oren,
	Tariq Toukan, Lorenzo Bianconi, drosen, Joanne Koong,
	henning.fehrmann, oliver.behnke



On 30/05/2023 15:40, Jesper Dangaard Brouer wrote:
> 
> 
> On 30/05/2023 14.17, Tariq Toukan wrote:
>>
>> On 30/05/2023 14:33, Jesper Dangaard Brouer wrote:
>>>
>>>
>>> On 29/05/2023 13.06, Tariq Toukan wrote:
>>>> Expand the xdp multi-buffer support to xdp_redirect tool.
>>>> Similar to what's done in commit
>>>> 772251742262 ("samples/bpf: fixup some tools to be able to support 
>>>> xdp multibuffer")
>>>> and its fix commit
>>>> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern").
>>>>
>>>
>>> Have you tested if this cause a performance degradation?
>>>
>>> (Also found possible bug below)
>>>
>>
>> Hi Jesper,
>>
>> This introduces the same known perf degradation we already have in 
>> xdp1 and xdp2.
> 
> Did a quick test with xdp1, the performance degradation is around 18%.
> 
>   Before: 22,917,961 pps
>   After:  18,798,336 pps
> 
>   (1-(18798336/22917961))*100 = 17.97%
> 
> 
>> Unfortunately, this is the API we have today to safely support 
>> multi-buffer.
>> Note that both perf and functional (noted below) degradation should be 
>> eliminated once replacing the load/store operations with dynptr logic 
>> that returns a pointer to the scatter entry instead of copying it.
>>
> 
> Well, should we use dynptr logic in this patch then?
> 

AFAIU it's not there ready to be used...
Not sure what parts are missing, I'll need to review it a bit deeper.

> Does it make sense to add sample code that does thing in a way that is 
> sub-optimal and we want to replace?
> ... (I fear people will copy paste the sample code).
> 

I get your point.
As xdp1 and xdp2 are already there, I thought that we'd want to expose 
multi-buffer samples in XDP_REDIRECT as well. We use these samples for 
internal testing.

>> I initiated a discussion on this topic a few months ago. dynptr was 
>> accepted since then, but I'm not aware of any in-progress followup 
>> work that addresses this.
>>
> 
> Are you saying some more work is needed on dynptr?
> 

AFAIU yes.
But I might be wrong... I need to revisit this.
Do you think/know that dynptr can be used immediately?

>>>> Signed-off-by: Tariq Toukan <tariqt@nvidia.com>
>>>> Reviewed-by: Nimrod Oren <noren@nvidia.com>
>>>> ---
>>>>   samples/bpf/xdp_redirect.bpf.c | 16 ++++++++++++----
>>>>   1 file changed, 12 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/samples/bpf/xdp_redirect.bpf.c 
>>>> b/samples/bpf/xdp_redirect.bpf.c
>>>> index 7c02bacfe96b..620163eb7e19 100644
>>>> --- a/samples/bpf/xdp_redirect.bpf.c
>>>> +++ b/samples/bpf/xdp_redirect.bpf.c
>>>> @@ -16,16 +16,21 @@
>>>>   const volatile int ifindex_out;
>>>> -SEC("xdp")
>>>> +#define XDPBUFSIZE    64
>>>
>>> Pktgen sample scripts will default send with 60 pkt length, because the
>>> 4 bytes FCS (end-frame checksum) is added by hardware.
>>>
>>> Will this result in an error when bpf_xdp_load_bytes() tries to copy 64
>>> bytes from a 60 bytes packet?
>>>
>>
>> Yes.
>>
>> This can be resolved by reducing XDPBUFSIZE to 60.
>> Need to check if it's OK to disregard these last 4 bytes without 
>> hurting the XDP program logic.
>>
>> If so, do you suggest changing xdp1 and xdp2 as well?
>>
> 
> I can take care of reducing XDPBUFSIZE to 60 on xpd1 and xdp2, as I
> already had to make these changes for the above quick bench work ;-)
> I'll send out patches shortly.
> 
> 
Thanks.

Are we fine with the above?
Should we just change the array size to 60 and re-spin?

>>>> +SEC("xdp.frags")
>>>>   int xdp_redirect_prog(struct xdp_md *ctx)
>>>>   {
>>>> -    void *data_end = (void *)(long)ctx->data_end;
>>>> -    void *data = (void *)(long)ctx->data;
>>>> +    __u8 pkt[XDPBUFSIZE] = {};
>>>> +    void *data_end = &pkt[XDPBUFSIZE-1];
>>>> +    void *data = pkt;
>>>>       u32 key = bpf_get_smp_processor_id();
>>>>       struct ethhdr *eth = data;
>>>>       struct datarec *rec;
>>>>       u64 nh_off;
>>>> +    if (bpf_xdp_load_bytes(ctx, 0, pkt, sizeof(pkt)))
>>>> +        return XDP_DROP;
>>>
>>> E.g. sizeof(pkt) = 64 bytes here.
>>>
>>>> +
>>>>       nh_off = sizeof(*eth);
>>>>       if (data + nh_off > data_end)
>>>>           return XDP_DROP;
>>>> @@ -36,11 +41,14 @@ int xdp_redirect_prog(struct xdp_md *ctx)
>>>>       NO_TEAR_INC(rec->processed);
>>>>       swap_src_dst_mac(data);
>>>> +    if (bpf_xdp_store_bytes(ctx, 0, pkt, sizeof(pkt)))
>>>> +        return XDP_DROP;
>>>> +
>>>>       return bpf_redirect(ifindex_out, 0);
>>>>   }
>>>>   /* Redirect require an XDP bpf_prog loaded on the TX device */
>>>> -SEC("xdp")
>>>> +SEC("xdp.frags")
>>>>   int xdp_redirect_dummy_prog(struct xdp_md *ctx)
>>>>   {
>>>>       return XDP_PASS;
>>>
>>
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-30 13:40         ` Tariq Toukan
@ 2023-05-30 15:05           ` Toke Høiland-Jørgensen
  2023-05-30 17:22           ` Martin KaFai Lau
  1 sibling, 0 replies; 10+ messages in thread
From: Toke Høiland-Jørgensen @ 2023-05-30 15:05 UTC (permalink / raw)
  To: Tariq Toukan, Jesper Dangaard Brouer, Alexei Starovoitov,
	John Fastabend, Jakub Kicinski
  Cc: brouer, Daniel Borkmann, Jesper Dangaard Brouer, bpf,
	David S. Miller, Eric Dumazet, netdev, Gal Pressman, Nimrod Oren,
	Tariq Toukan, Lorenzo Bianconi, drosen, Joanne Koong,
	henning.fehrmann, oliver.behnke

Tariq Toukan <ttoukan.linux@gmail.com> writes:

> On 30/05/2023 15:40, Jesper Dangaard Brouer wrote:
>> 
>> 
>> On 30/05/2023 14.17, Tariq Toukan wrote:
>>>
>>> On 30/05/2023 14:33, Jesper Dangaard Brouer wrote:
>>>>
>>>>
>>>> On 29/05/2023 13.06, Tariq Toukan wrote:
>>>>> Expand the xdp multi-buffer support to xdp_redirect tool.
>>>>> Similar to what's done in commit
>>>>> 772251742262 ("samples/bpf: fixup some tools to be able to support 
>>>>> xdp multibuffer")
>>>>> and its fix commit
>>>>> 7a698edf954c ("samples/bpf: Fix MAC address swapping in xdp2_kern").
>>>>>
>>>>
>>>> Have you tested if this cause a performance degradation?
>>>>
>>>> (Also found possible bug below)
>>>>
>>>
>>> Hi Jesper,
>>>
>>> This introduces the same known perf degradation we already have in 
>>> xdp1 and xdp2.
>> 
>> Did a quick test with xdp1, the performance degradation is around 18%.
>> 
>>   Before: 22,917,961 pps
>>   After:  18,798,336 pps
>> 
>>   (1-(18798336/22917961))*100 = 17.97%
>> 
>> 
>>> Unfortunately, this is the API we have today to safely support 
>>> multi-buffer.
>>> Note that both perf and functional (noted below) degradation should be 
>>> eliminated once replacing the load/store operations with dynptr logic 
>>> that returns a pointer to the scatter entry instead of copying it.
>>>
>> 
>> Well, should we use dynptr logic in this patch then?
>> 
>
> AFAIU it's not there ready to be used...
> Not sure what parts are missing, I'll need to review it a bit deeper.
>
>> Does it make sense to add sample code that does thing in a way that is 
>> sub-optimal and we want to replace?
>> ... (I fear people will copy paste the sample code).
>> 
>
> I get your point.
> As xdp1 and xdp2 are already there, I thought that we'd want to expose 
> multi-buffer samples in XDP_REDIRECT as well. We use these samples for 
> internal testing.

Note that I am planning to send a patch to remove these utilities in the
not-so distant future. We have merged the xdp-bench utility into
xdp-tools as of v1.3.0, and that should contain all functionality of
both the xdp1/2 utilities and the xdp_redirect* utilities, without being
dependent on the (slowly bitrotting) samples/bpf directory.

The only reason I haven't sent the patch to remove the utilities yet is
that I haven't yet merged the multibuf support (WiP PR here:
https://github.com/xdp-project/xdp-tools/pull/314).

I'll try to move that up on my list of things to get done, but in the
meantime I'd advice against expending too much effort on improving these
tools :)

-Toke


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-30 13:40         ` Tariq Toukan
  2023-05-30 15:05           ` Toke Høiland-Jørgensen
@ 2023-05-30 17:22           ` Martin KaFai Lau
  2023-05-31 12:02             ` Tariq Toukan
  1 sibling, 1 reply; 10+ messages in thread
From: Martin KaFai Lau @ 2023-05-30 17:22 UTC (permalink / raw)
  To: Tariq Toukan
  Cc: brouer, Daniel Borkmann, Jesper Dangaard Brouer, bpf,
	David S. Miller, Eric Dumazet, netdev, Gal Pressman, Nimrod Oren,
	Tariq Toukan, Lorenzo Bianconi, drosen, Joanne Koong,
	henning.fehrmann, oliver.behnke, Jesper Dangaard Brouer,
	Alexei Starovoitov, John Fastabend, Jakub Kicinski

On 5/30/23 6:40 AM, Tariq Toukan wrote:
>>> I initiated a discussion on this topic a few months ago. dynptr was accepted 
>>> since then, but I'm not aware of any in-progress followup work that addresses 
>>> this.
>>>
>>
>> Are you saying some more work is needed on dynptr?
>>
> 
> AFAIU yes.
> But I might be wrong... I need to revisit this.
> Do you think/know that dynptr can be used immediately?

Not sure if you are aware of the bpf_dynptr_slice[_rdwr]() which could be useful 
here. It only does a copy when the requested slice is across different frags:
https://lore.kernel.org/all/20230301154953.641654-10-joannelkoong@gmail.com/


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer
  2023-05-30 17:22           ` Martin KaFai Lau
@ 2023-05-31 12:02             ` Tariq Toukan
  0 siblings, 0 replies; 10+ messages in thread
From: Tariq Toukan @ 2023-05-31 12:02 UTC (permalink / raw)
  To: Martin KaFai Lau
  Cc: brouer, Daniel Borkmann, Jesper Dangaard Brouer, bpf,
	David S. Miller, Eric Dumazet, netdev, Gal Pressman, Nimrod Oren,
	Tariq Toukan, Lorenzo Bianconi, drosen, Joanne Koong,
	henning.fehrmann, oliver.behnke, Jesper Dangaard Brouer,
	Alexei Starovoitov, John Fastabend, Jakub Kicinski



On 30/05/2023 20:22, Martin KaFai Lau wrote:
> On 5/30/23 6:40 AM, Tariq Toukan wrote:
>>>> I initiated a discussion on this topic a few months ago. dynptr was 
>>>> accepted since then, but I'm not aware of any in-progress followup 
>>>> work that addresses this.
>>>>
>>>
>>> Are you saying some more work is needed on dynptr?
>>>
>>
>> AFAIU yes.
>> But I might be wrong... I need to revisit this.
>> Do you think/know that dynptr can be used immediately?
> 
> Not sure if you are aware of the bpf_dynptr_slice[_rdwr]() which could 
> be useful here. It only does a copy when the requested slice is across 
> different frags:
> https://lore.kernel.org/all/20230301154953.641654-10-joannelkoong@gmail.com/
> 

Thanks for the pointer. I missed it. Looks promising!
I'll take a deeper look soon and do some perf tests.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-05-31 12:02 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-05-29 11:06 [PATCH bpf-next 0/2] multi-buffer support for XDP_REDIRECT samples Tariq Toukan
2023-05-29 11:06 ` [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer Tariq Toukan
2023-05-30 11:33   ` Jesper Dangaard Brouer
2023-05-30 12:17     ` Tariq Toukan
2023-05-30 12:40       ` Jesper Dangaard Brouer
2023-05-30 13:40         ` Tariq Toukan
2023-05-30 15:05           ` Toke Høiland-Jørgensen
2023-05-30 17:22           ` Martin KaFai Lau
2023-05-31 12:02             ` Tariq Toukan
2023-05-29 11:06 ` [PATCH bpf-next 2/2] samples/bpf: fixup xdp_redirect_map " Tariq Toukan

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).