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

[Jesper Dangaard Brouer <jbrouer@redhat.com>]

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