linux-kernel - Re: [PATCH net-next v2 16/18] xsk: add helper to get &xdp

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] [day] [month] [year] [list]

Message-ID: <e24250e5-3525-4fdf-8ac4-2fb8e33bca9e@intel.com>
Date: Wed, 23 Oct 2024 16:50:32 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
CC: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
	<edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
	<pabeni@...hat.com>, Toke Høiland-Jørgensen
	<toke@...hat.com>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
	<daniel@...earbox.net>, John Fastabend <john.fastabend@...il.com>, "Andrii
 Nakryiko" <andrii@...nel.org>, Stanislav Fomichev <sdf@...ichev.me>, "Magnus
 Karlsson" <magnus.karlsson@...el.com>,
	<nex.sw.ncis.osdt.itp.upstreaming@...el.com>, <bpf@...r.kernel.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next v2 16/18] xsk: add helper to get &xdp_desc's DMA
 and meta pointer in one go

From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
Date: Tue, 22 Oct 2024 17:42:13 +0200

> On Tue, Oct 15, 2024 at 04:53:48PM +0200, Alexander Lobakin wrote:
>> Currently, when you send an XSk frame without metadata, you need to do
> 
> you meant *with* metadata?

Eeeeh... Maybe, I forgot already what I wanted to say =\

> 
>> the following:
>>
>> * call external xsk_buff_raw_get_dma();
>> * call inline xsk_buff_get_metadata(), which calls external
>>   xsk_buff_raw_get_data() and then do some inline checks.
>>
>> This effectively means that the following piece:
>>
>> addr = pool->unaligned ? xp_unaligned_add_offset_to_addr(addr) : addr;
>>
>> is done twice per frame, plus you have 2 external calls per frame, plus
>> this:
>>
>> 	meta = pool->addrs + addr - pool->tx_metadata_len;
>> 	if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
>>
>> is always inlined, even if there's no meta or it's invalid.
> 
> when there is no meta you bail out early in xsk_buff_get_metadata() as
> tx_metadata_len was not set, no?

Yes, but this code is still inlined.
See below (at the end of the reply).

> 
>>
>> Add xsk_buff_raw_get_ctx() (xp_raw_get_ctx() to be precise) to do that
>> in one go. It returns a small structure with 2 fields: DMA address,
>> filled unconditionally, and metadata pointer, valid only if it's
>> present. The address correction is performed only once and you also
>> have only 1 external call per XSk frame, which does all the calculations
>> and checks outside of your hotpath. You only need to check
>> `if (ctx.meta)` for the metadata presence.
> 
> IMHO adding this might confuse future users which approach should be
> preferred.

It's a regular practice in the kernel that we have several functions to
do +/- the same. It's up to the developer which one to pick, he reads
the code and decides himself.

> 
> Thinking out loud...couldn't we export address correction logic and pass
> the corrected addr to xsk_buff_get_metadata and then add it to
> pool->addrs. But that would require modifying existing callsites +
> addressing xp_raw_get_dma() as well :<

Yes, modifying current API requires touching the users.
+ keeping xsk_buff_get_metadata negates most the main purpose of this
patch, see below.

> 
> Standard question - any perf improvement when micro benchmarking? :P

TBH I didn't test before/after with the meta enabled, but it was enough
for me that using this function instead of the get_dma + get_meta pair
reduces the object code size by 1 Kb when unrolling by 8.

Thanks,
Olek