Re: [PATCH net-next v7 3/8] page_pool: fix IOMMU crash when driver has already unbound

On 1/18/2025 12:56 AM, Jesper Dangaard Brouer wrote:

...

quoted

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().

quoted

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