[PATCH bpf-next 0/2] multi-buffer support for XDP_REDIRECT samples

[PATCH bpf-next 0/2] multi-buffer support for XDP_REDIRECT samples

[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

[PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[Tariq Toukan]

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

[PATCH bpf-next 2/2] samples/bpf: fixup xdp_redirect_map tool to be able to support xdp multibuffer

[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

Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[Jesper Dangaard Brouer]

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;

Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[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;
> 

Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[Jesper Dangaard Brouer]

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;
>>
> 

Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[Tariq Toukan]

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;
>>>
>>
> 

Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[Toke Høiland-Jørgensen]

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

Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[Martin KaFai Lau]

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/

Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be able to support xdp multibuffer

[Tariq Toukan]

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.