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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 30 May 2023 17:05:00 +0200
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Tariq Toukan <ttoukan.linux@...il.com>, Jesper Dangaard Brouer
 <jbrouer@...hat.com>, Alexei Starovoitov <ast@...nel.org>, John Fastabend
 <john.fastabend@...il.com>, Jakub Kicinski <kuba@...nel.org>
Cc: brouer@...hat.com, Daniel Borkmann <daniel@...earbox.net>, Jesper
 Dangaard Brouer <hawk@...nel.org>, bpf@...r.kernel.org, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 netdev@...r.kernel.org, Gal Pressman <gal@...dia.com>, Nimrod Oren
 <noren@...dia.com>, Tariq Toukan <tariqt@...dia.com>, Lorenzo Bianconi
 <lorenzo.bianconi@...hat.com>, drosen@...gle.com, Joanne Koong
 <joannelkoong@...il.com>, henning.fehrmann@....mpg.de,
 oliver.behnke@....mpg.de
Subject: Re: [PATCH bpf-next 1/2] samples/bpf: fixup xdp_redirect tool to be
 able to support xdp multibuffer

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