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

["Toke Høiland-Jørgensen" <toke@redhat.com>]

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