Alexander Lobakin <aleksander.lobakin@xxxxxxxxx> writes: > 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 :> Sure, co-developed-by SGTM :) -Toke
