linux-kernel - Re: [PATCH net-next v3 09/18] page_pool: allow mixing PPs within one bulk

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]

Message-ID: <87y11z7yv6.fsf@toke.dk>
Date: Mon, 04 Nov 2024 17:22:21 +0100
From: Toke Høiland-Jørgensen <toke@...hat.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
 <daniel@...earbox.net>, John Fastabend <john.fastabend@...il.com>, Andrii
 Nakryiko <andrii@...nel.org>, Maciej Fijalkowski
 <maciej.fijalkowski@...el.com>, 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 v3 09/18] page_pool: allow mixing PPs within
 one bulk

Alexander Lobakin <aleksander.lobakin@...el.com> writes:

> From: Toke Høiland-Jørgensen <toke@...hat.com>
> Date: Fri, 01 Nov 2024 14:09:59 +0100
>
>> Alexander Lobakin <aleksander.lobakin@...el.com> writes:
>> 
>>> The main reason for this change was to allow mixing pages from different
>>> &page_pools within one &xdp_buff/&xdp_frame. Why not?
>>> Adjust xdp_return_frame_bulk() and page_pool_put_page_bulk(), so that
>>> they won't be tied to a particular pool. Let the latter create a
>>> separate bulk of pages which's PP is different and flush it recursively.
>>> This greatly optimizes xdp_return_frame_bulk(): no more hashtable
>>> lookups. Also make xdp_flush_frame_bulk() inline, as it's just one if +
>>> function call + one u32 read, not worth extending the call ladder.
>>>
>>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>
>> 
>> Neat idea, but one comment, see below:
>
> [...]
>
>>> +	if (sub.count)
>>> +		page_pool_put_page_bulk(sub.q, sub.count, true);
>>> +
>> 
>> In the worst case here, this function can recursively call itself
>> XDP_BULK_QUEUE_SIZE (=16) times. Which will blow ~2.5k of stack size,
>> and lots of function call overhead. I'm not saying this level of
>> recursion is likely to happen today, but who knows about future uses? So
>> why not make it iterative instead of recursive (same basic idea, but
>> some kind of 'goto begin', or loop, instead of the recursive call)?
>
> Oh, great idea!
> I was also unsure about the recursion here. Initially, I wanted header
> split frames, which usually have linear/header part from one PP and
> frag/payload part from second PP, to be efficiently recycled in bulks.
> Currently, it's not possible, as a bulk will look like [1, 2, 1, 2, ...]
> IOW will be flush every frame.
> But I realize the recursion is not really optimal here, just the first
> that came to my mind. I'll give you Suggested-by here (or
> Co-developed-by?), really liked your approach :>

Sure, co-developed-by SGTM :)

-Toke