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
