On 1/18/2025 12:56 AM, Jesper Dangaard Brouer wrote:
...
I am not really sure about that, as using the PAGE_SIZE block to hold the
item seems like a implementation detail, which might change in the
future,
renaming other function to something like that doesn't seem right to
me IMHO.
Also the next patch will add page_pool_item_blk_add() to support
unlimited
inflight pages, it seems a better name is needed for that too, perheps
rename
page_pool_item_blk_add() to page_pool_dynamic_item_add()?
Hmmm... not sure about this.
I think I prefer page_pool_item_blk_add() over
page_pool_dynamic_item_add().
For __page_pool_item_init(), perhaps just inline it back to
page_pool_item_init()
as __page_pool_item_init() is only used by page_pool_item_init(), and
both of them
are not really large function.
I like that you had a helper function. So, don't merge
__page_pool_item_init() into page_pool_item_init() just to avoid naming
it differently.
Any particular reason for the above suggestion?
After reusing the page_pool_item_uninit() to fix the similar
use-after-freed problem，it seems reasonable to not expose the
item_blcok as much as possible as item_blcok is really an
implementation detail that should be hidden as much as possible
IMHO.
If it is able to reused for supporting the unlimited item case,
then I am agreed that it might be better to refactor it out,
but it is not really reusable.
static int page_pool_item_init(struct page_pool *pool)
{
#define PAGE_POOL_MIN_INFLIGHT_ITEMS 512
struct page_pool_item_block *block;
int item_cnt;
INIT_LIST_HEAD(&pool->item_blocks);
init_llist_head(&pool->hold_items);
init_llist_head(&pool->release_items);
item_cnt = pool->p.pool_size * 2 + PP_ALLOC_CACHE_SIZE +
PAGE_POOL_MIN_INFLIGHT_ITEMS;
for (; item_cnt > 0; item_cnt -= ITEMS_PER_PAGE) {
struct page *page;
unsigned int i;
page = alloc_pages_node(pool->p.nid, GFP_KERNEL, 0);
if (!page) {
page_pool_item_uninit(pool);
return -ENOMEM;
}
block = page_address(page);
block->pp = pool;
list_add(&block->list, &pool->item_blocks);
for (i = 0; i < ITEMS_PER_PAGE; i++) {
page_pool_item_init_state(&block->items[i]);
__llist_add(&block->items[i].lentry,
&pool->hold_items);
}
}
return 0;
}
Let me be more explicit what I'm asking for:
IMHO you should rename:
- __page_pool_item_init() to __page_pool_item_block_init()
and rename:
- page_pool_item_init() to page_pool_item_block_init()
I hope this make it more clear what I'm saying.
> > --Jesper
