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

[Yunsheng Lin]

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
>