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

[Alexander Lobakin]

From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx>
Date: Fri, 01 Nov 2024 14:09:59 +0100

> Alexander Lobakin <aleksander.lobakin@xxxxxxxxx> 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@xxxxxxxxx>
> 
> 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 :>

Thanks,
Olek