Tariq Toukan <ttoukan.linux@xxxxxxxxx> 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
