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

[Yunsheng Lin <yunshenglin0825@gmail.com>]

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
>